-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113803/#review43489
-----------------------------------------------------------
I don't sure about static const QLatin1String variables naming and about
introducing
static const QLatin1String dbusInterfacePropertiesChanged("PropertiesChanged");
static const QLatin1String dbusInterfaceGetAll("GetAll");
It's seems like slot onSettingsChanged() is called from outside and should
remain public. I think there should be empty virtual slot in
TelepathyKDEDModulePlugin with appropriative comment.
I don't know about onActivateNowPlaying() and onDeactivateNowPlaying()
publicity. I have found only connections in TelepathyMPRIS constructor and,
probably private is better.
- Alexandr Akulich
On Nov. 12, 2013, 12:59 p.m., Alexandr Akulich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113803/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2013, 12:59 p.m.)
>
>
> Review request for Telepathy.
>
>
> Repository: ktp-kded-module
>
>
> Description
> -------
>
> Get rid unnecessary include.
>
>
>
> Extract few string constants to static const string.
>
>
>
> Replace QString::contains() by startsWith() in service prefix matching.
>
>
>
> Rename newMediaPlayer() to watchPlayer() (inspired by unwatchAllPlayers()).
>
>
>
> Rename m_knownPlayers to m_watchedPlayers.
>
>
>
> Move few public slots to private slots or methods.
>
>
> Diffs
> -----
>
> telepathy-mpris.h c223e94
> telepathy-mpris.cpp 93875fe
>
> Diff: http://git.reviewboard.kde.org/r/113803/diff/
>
>
> Testing
> -------
>
> Looks like it works as well, as before refactor. (Tested with amarok and
> dragon player).
>
>
> Thanks,
>
> Alexandr Akulich
>
>
_______________________________________________
KDE-Telepathy mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/kde-telepathy
