----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116907/#review54208 -----------------------------------------------------------
I have general feedback around the code below. It will be nice if you can poke someone who has written MPRIS-related code before to review this before finalizing. libs/mpris2/mediaplayer2.cpp <https://git.reviewboard.kde.org/r/116907/#comment37904> This would look better- QDBusConnection::sessionBus().registerObject("/org/mpris/MediaPlayer2", this, QDBusConnection::ExportAllSlots | QDBusConnection::ExportAllProperties | QDBusConnection::ExportAllSignals); libs/mpris2/mediaplayer2player.h <https://git.reviewboard.kde.org/r/116907/#comment37906> Are these supposed to be called on this object from outside? If yes, why are these protected? If no, why Q_INVOKABLE? libs/mpris2/mediaplayer2player.cpp <https://git.reviewboard.kde.org/r/116907/#comment37907> How is 64 decided? libs/mpris2/mediaplayer2player.cpp <https://git.reviewboard.kde.org/r/116907/#comment37909> if and else need braces, even when single-line, see http://techbase.kde.org/Policies/Kdelibs_Coding_Style libs/mpris2/mediaplayer2player.cpp <https://git.reviewboard.kde.org/r/116907/#comment37910> const int shells/newshell/mainwindow.cpp <https://git.reviewboard.kde.org/r/116907/#comment37911> whitespace shells/newshell/package/contents/ui/mediacenter.qml <https://git.reviewboard.kde.org/r/116907/#comment37912> instead of this, can't you use onPlayRequested instead? Its better to avoid dependencies for components (In this case Playlist requires a mprisPlayer to work). - Shantanu Tushar On March 26, 2014, 6:46 a.m., Ashish Madeti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/116907/ > ----------------------------------------------------------- > > (Updated March 26, 2014, 6:46 a.m.) > > > Review request for Plasma, Shantanu Tushar, Sinny Kumari, and Sujith > Haridasan. > > > Repository: plasma-mediacenter > > > Description > ------- > > Implemented "Player" DBus adaptor of MPRIS specifications for Plasma > Mediacenter. > Specification reference: > http://specifications.freedesktop.org/mpris-spec/latest/Player_Interface.html > > Some more work needs to be done in the adaptor which I plan to do soon. > > > Diffs > ----- > > libs/mpris2/mpris2.cpp a8ad3ef > mediaelements/mediaplayer/MediaPlayer.qml 39ed617 > mediaelements/playlist/Playlist.qml 5dde297 > shells/newshell/mainwindow.cpp d2d71d4 > shells/newshell/package/contents/ui/mediacenter.qml bac33c2 > libs/mpris2/mediaplayer2.h e68bc5c > libs/mpris2/mediaplayer2.cpp ff96618 > libs/mpris2/mediaplayer2player.h 203d681 > libs/mpris2/mediaplayer2player.cpp 7871efa > > Diff: https://git.reviewboard.kde.org/r/116907/diff/ > > > Testing > ------- > > Tested with qdbusviewer, the properties and methods are working fine. > > > Thanks, > > Ashish Madeti > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
