--===============7531899123387519730== Content-Type: multipart/alternative; boundary="===============0224675491140478296==" --===============0224675491140478296== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote: > > core/Player.h, line 39 > > > > > > Should be addAudioOutput and addVideoOutput if you want to keep the= m separate. > = > Mat=C4=9Bj Laitl wrote: > Why? The alternative is an abstract Output class, who's polymorphic sub-classes = can be added. Since there is no intention just yet to make that class it's = better to keep these method names rather then 2 addOutput() with different = arguments. > On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote: > > core/Player.h, line 51 > > > > > > Metadata should go in the source! > > = > > Would be nice in Amarok that we can simply map Track to Phonon::Sou= rce in the metadata handling. > = > Mat=C4=9Bj Laitl wrote: > +1. I discovered on the train back we need operator<() and a global qHash(Phono= n::Source) for this. On for use as a key in a QMap, the other in a QHash. > On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote: > > core/Player.h, line 52 > > > > > > no. Total can be unknown and it's a one line calculation. > = > Mat=C4=9Bj Laitl wrote: > But Phonon may know the length better than we in Amarok. I would pers= onally like to use some kind fo remainingTime signals to implement bounded = playback in Amarok. Phonon would internally do the same calculation (remaining =3D total - elap= sed). If you add the convenience function, shouldn't you also add the notif= y signal, etc? > On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote: > > core/Queue.h, line 21 > > > > > > Provide some convenience functions for this. > > iterator, index, append, prepend, etc. > = > Mat=C4=9Bj Laitl wrote: > Do we really need em? We just use enqueue() and clear() in Amarok. Amarok uses it's own queue, other applications probably don't want to make = it as complex. Besides, I'm hoping that in Phonon 5 we won't need our own queue implementa= tion either. - Bart ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106566/#review19473 ----------------------------------------------------------- On Sept. 25, 2012, 11:06 a.m., Harald Sitter wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106566/ > ----------------------------------------------------------- > = > (Updated Sept. 25, 2012, 11:06 a.m.) > = > = > Review request for Amarok and Phonon. > = > = > Description > ------- > = > phonon phive core frontend api > = > = > Diffs > ----- > = > core/AudioDataOutput.h PRE-CREATION = > core/AudioDataOutput.cpp PRE-CREATION = > core/AudioOutput.h PRE-CREATION = > core/AudioOutput.cpp PRE-CREATION = > core/BackendCapabilities.h PRE-CREATION = > core/BackendCapabilities.cpp PRE-CREATION = > core/OutputEffect.h PRE-CREATION = > core/OutputEffect.cpp PRE-CREATION = > core/Player.h PRE-CREATION = > core/Player.cpp PRE-CREATION = > core/Queue.h PRE-CREATION = > core/Queue.cpp PRE-CREATION = > core/Source.h PRE-CREATION = > core/Source.cpp PRE-CREATION = > core/VideoDataOutput.h PRE-CREATION = > core/VideoDataOutput.cpp PRE-CREATION = > core/abstract/AbstractAudioOutput.h PRE-CREATION = > core/abstract/AbstractAudioOutput.cpp PRE-CREATION = > core/abstract/AbstractMediaStream.h PRE-CREATION = > core/abstract/AbstractMediaStream.cpp PRE-CREATION = > core/abstract/AbstractVideoOutput.h PRE-CREATION = > core/abstract/AbstractVideoOutput.cpp PRE-CREATION = > core/core.pro PRE-CREATION = > core/effects/SubtitleEffect.h PRE-CREATION = > core/effects/SubtitleEffect.cpp PRE-CREATION = > core/five_global.h PRE-CREATION = > = > Diff: http://git.reviewboard.kde.org/r/106566/diff/ > = > = > Testing > ------- > = > = > Thanks, > = > Harald Sitter > = > --===============0224675491140478296== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: http://git.revie= wboard.kde.org/r/106566/

On September 26th, 2012, 7:42 p.m., Bart Ce= rneels wrote:

= = =
core/Player.h (Diff revision 1) 

   
public:


  

 

  

    
    

    39
    
    bool addOutput(Abstract=
AudioOutput *audioOutput<=
/span>);

  


 




  
Should be=
 addAudioOutput and addVideoOutput if you want to keep them separate.

 




 
On September 29th, 2012, 12:16 p.m., Mat=C4=9Bj Laitl wrote:

 

  
Why?

 











The alternative is an abstract Output class, who's polymorphic s=
ub-classes can be added. Since there is no intention just yet to make that =
class it's better to keep these method names rather then 2 addOutput() =
with different arguments.






 
On September 26th, 2012, 7:42 p.m., Bart Ce=
rneels wrote:

 

  =









 =

 =




 

 
  

   
    core/Player.h
    

     (Diff revision 1)

    
   
  

 

 
  


   

   
public:


  

 

  

    
    

    51
    
#warning metadata missing

  


 




  
Metadata =
should go in the source!

Would be nice in Amarok that we can simply map Track to Phonon::Source in t=
he metadata handling.

 




 
On September 29th, 2012, 12:16 p.m., Mat=C4=9Bj Laitl wrote:

 

  
+1.

 











I discovered on the train back we need operator<() and a global q=
Hash(Phonon::Source) for this. On for use as a key in a QMap, the other in =
a QHash.






 
On September 26th, 2012, 7:42 p.m., Bart Ce=
rneels wrote:

 

  =









 =

 =




 

 
  

   
    core/Player.h
    

     (Diff revision 1)

    
   
  

 

 
  


   

   
public:


  

 

  

    
    

    52
    
#warning remainingTime inte=
rface?

  


 




  
no. Total=
 can be unknown and it's a one line calculation.

 




 
On September 29th, 2012, 12:16 p.m., Mat=C4=9Bj Laitl wrote:

 

  
But Phono=
n may know the length better than we in Amarok. I would personally like to =
use some kind fo remainingTime signals to implement bounded playback in Ama=
rok.

 











Phonon would internally do the same calculation (remaining =3D total=
 - elapsed). If you add the convenience function, shouldn't you also ad=
d the notify signal, etc?






 
On September 26th, 2012, 7:42 p.m., Bart Ce=
rneels wrote:

 

  =









 =

 =




 

 
  

   
    core/Queue.h
    

     (Diff revision 1)

    
   
  

 

 
  


   

   
class Queue {


  

 

  

    
    

    21
    
        void enqueue(const<=
/span> Source &source, const=
 Source &before=3DSou=
rce());

  


 




  
Provide s=
ome convenience functions for this.
iterator, index, append, prepend, etc.

 




 
On September 29th, 2012, 12:16 p.m., Mat=C4=9Bj Laitl wrote:

 

  
Do we rea=
lly need em? We just use enqueue() and clear() in Amarok.

 











Amarok uses it's own queue, other applications probably don'=
t want to make it as complex.
Besides, I'm hoping that in Phonon 5 we won't need our own queue im=
plementation either.








- Bart






On September 25th, 2012, 11:06 a.m., Harald Sitter wrote:









 

  


Review request for Amarok and Phonon.


By Harald Sitter.




Updated Sept. 25, 2012, 11:06 a.m.








Descripti=
on 

 

 

  
   
phonon phive core frontend api

  		
 









Diffs=
 


    

 
  • core/AudioDataOutput.h (PRE-CREATION)
    • 

 
  • core/AudioDataOutput.cpp (PRE-CREATION)
    • 

 
  • core/AudioOutput.h (PRE-CREATION)

 
  • core/AudioOutput.cpp (PRE-CREATION)=

    • 

 
  • core/BackendCapabilities.h (PRE-CREATION)<=
/span>
    • 

 
  • core/BackendCapabilities.cpp (PRE-CREATION=
)
    • 

 
  • core/OutputEffect.h (PRE-CREATION)<=
/li>

 
  • core/OutputEffect.cpp (PRE-CREATION)
    • 

 
  • core/Player.h (PRE-CREATION)
    • 

 
  • core/Player.cpp (PRE-CREATION)
    • 

 
  • core/Queue.h (PRE-CREATION)
    • 

 
  • core/Queue.cpp (PRE-CREATION)
    • 

 
  • core/Source.h (PRE-CREATION)
    • 

 
  • core/Source.cpp (PRE-CREATION)
    • 

 
  • core/VideoDataOutput.h (PRE-CREATION)
    • 

 
  • core/VideoDataOutput.cpp (PRE-CREATION)
    • 

 
  • core/abstract/AbstractAudioOutput.h (PRE-C=
REATION)
    • 

 
  • core/abstract/AbstractAudioOutput.cpp (PRE=
-CREATION)
    • 

 
  • core/abstract/AbstractMediaStream.h (PRE-C=
REATION)
    • 

 
  • core/abstract/AbstractMediaStream.cpp (PRE=
-CREATION)
    • 

 
  • core/abstract/AbstractVideoOutput.h (PRE-C=
REATION)
    • 

 
  • core/abstract/AbstractVideoOutput.cpp (PRE=
-CREATION)
    • 

 
  • core/core.pro (PRE-CREATION)
    • 

 
  • core/effects/SubtitleEffect.h (PRE-CREATIO=
N)
    • 

 
  • core/effects/SubtitleEffect.cpp (PRE-CREAT=
ION)
    • 

 
  • core/five_global.h (PRE-CREATION)





View Diff





  		
 











  

 



--===============0224675491140478296==--

--===============7531899123387519730==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
kde-multimedia mailing list
kde-multimedia@kde.org
https://mail.kde.org/mailman/listinfo/kde-multimedia

--===============7531899123387519730==--