Review Request: phonon phive core frontend api

[Bart Cerneels]

> On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote:
> > core/Player.h, line 39
> > <http://git.reviewboard.kde.org/r/106566/diff/1/?file=87102#file87102line39>
> >
> >     Should be addAudioOutput and addVideoOutput if you want to keep them separate.
> 
> Matěj 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
> > <http://git.reviewboard.kde.org/r/106566/diff/1/?file=87102#file87102line51>
> >
> >     Metadata should go in the source!
> >     
> >     Would be nice in Amarok that we can simply map Track to Phonon::Source in the metadata handling.
> 
> Matěj Laitl wrote:
>     +1.

I discovered on the train back we need operator<() and a global qHash(Phonon::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
> > <http://git.reviewboard.kde.org/r/106566/diff/1/?file=87102#file87102line52>
> >
> >     no. Total can be unknown and it's a one line calculation.
> 
> Matěj Laitl wrote:
>     But Phonon 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 Amarok.

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


> On Sept. 26, 2012, 7:42 p.m., Bart Cerneels wrote:
> > core/Queue.h, line 21
> > <http://git.reviewboard.kde.org/r/106566/diff/1/?file=87104#file87104line21>
> >
> >     Provide some convenience functions for this.
> >     iterator, index, append, prepend, etc.
> 
> Matěj 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 implementation 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-multimedia/attachments/20120930/e9b495d5/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-multimedia mailing list
kde-multimedia at kde.org
https://mail.kde.org/mailman/listinfo/kde-multimedia