Review Request 112802: Extend dbus unterface for reading and writing the rating of the current song

[Matěj Laitl]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130920/46408d4f/attachment.html>