----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112802/#review40381 -----------------------------------------------------------
Thanks for the patch. I acknowledge a need for a way to edit track rating (or, even better would be a generic way to edit any metadata), but I think that a different approach should be used, similarly to what Alex Merry said. Please see below. src/dbus/mpris2/MediaPlayer2AmarokExtensions.cpp <http://git.reviewboard.kde.org/r/112802/#comment29779> This is not the code style we use in Amarok, please see HACKING folder. [this is just for information and future reference] src/dbus/mpris2/MediaPlayer2AmarokExtensions.cpp <http://git.reviewboard.kde.org/r/112802/#comment29780> I believe this is triggered automatically by EngineController signals and Osd listening. For sure we should not notify OSD every time we change the rating, that would be braindead. src/dbus/mpris2/org.kde.amarok.Mpris2Extensions.Player.xml <http://git.reviewboard.kde.org/r/112802/#comment29781> Hmm, I must say I don't like having a rating property for a "player" object, because it doesn't logically belong there. It logically belongs to a "track". For reading purposes, the only reasonable place is MediaPlayer2.TrackList.GetTracksMetadata, and we *already* fill in xesam:userRating, so please let's don't add unnecessary redundancy (yet another undocumented ad-hoc method). For writing let's wait for the outcome of the discussion on the MPRIS list. - Matěj Laitl On Sept. 18, 2013, 9:34 p.m., Alex Busenius wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/112802/ > ----------------------------------------------------------- > > (Updated Sept. 18, 2013, 9:34 p.m.) > > > Review request for Amarok and Alex Merry. > > > Description > ------- > > The D-Bus interface is currently missing convenient method for reading and > setting user rating (the stars). The proposed patch adds an Mpris2 extension > to /org/mpris/MediaPlayer2 for that: > > property readwrite int org.kde.amarok.Mpris2Extensions.Player.Rating > > > Diffs > ----- > > src/dbus/mpris2/MediaPlayer2AmarokExtensions.h 964c7b8 > src/dbus/mpris2/MediaPlayer2AmarokExtensions.cpp ba8b54e > src/dbus/mpris2/org.kde.amarok.Mpris2Extensions.Player.xml 3397df4 > > Diff: http://git.reviewboard.kde.org/r/112802/diff/ > > > Testing > ------- > > > Thanks, > > Alex Busenius > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel