This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106566/

On September 26th, 2012, 7:42 p.m., Bart Cerneels wrote:

core/Player.h (Diff revision 1)
public:
39
    bool addOutput(AbstractAudioOutput *audioOutput);
Should be addAudioOutput and addVideoOutput if you want to keep them separate.
Why?

On September 26th, 2012, 7:42 p.m., Bart Cerneels 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 the metadata handling.
+1.

On September 26th, 2012, 7:42 p.m., Bart Cerneels wrote:

core/Player.h (Diff revision 1)
public:
52
#warning remainingTime interface?
no. Total can be unknown and it's a one line calculation.
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.

On September 26th, 2012, 7:42 p.m., Bart Cerneels wrote:

core/Player.h (Diff revision 1)
public:
53
#warning ticking missing
You want to know the required resolution. And no, it's not easy to calculate when it might change!
I'd like to ditch ticking from Amarok code in favor of some kind of remainingTime mechanism.

On September 26th, 2012, 7:42 p.m., Bart Cerneels wrote:

core/Queue.h (Diff revision 1)
class Queue {
21
        void enqueue(const Source &source, const Source &before=Source());
Provide some convenience functions for this.
iterator, index, append, prepend, etc.
Do we really need em? We just use enqueue() and clear() in Amarok.

On September 26th, 2012, 7:42 p.m., Bart Cerneels wrote:

core/Queue.h (Diff revision 1)
class Queue {
26
        Source dequeue(const Source &source = Source());
Couldn't I put the same source in multiple times? Which one will you delete?

Again, convenience functions needed.

Or will the Queue be set?
> Or will the Queue be [a] set?

Cannot be, set doesn't preserve order and we really need it for a .. queue.

On September 26th, 2012, 7:42 p.m., Bart Cerneels wrote:

core/abstract/AbstractVideoOutput.h (Diff revision 1)
private:
16
    QList<OutputEffect *> m_effects;
Add same effect (object), what happens? Ordering?

Could make this a QSet
> Could make this a QSet

No, effect order matters and QSet doesn't preserve order.

- Matěj


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.

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)

View Diff