> On Nov. 12, 2013, 1:41 p.m., Alexandr Akulich wrote:
> > 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.
>
> David Edmundson wrote:
> I personally would not add "GetAll" and such as constant strings.
> It becomes harder to read; you'd only read these code when trying to
> debug DBus things, so you'd end up having to look up every string to work out
> what it's actually doing.
>
> As for those methods I tend to have my slots named as:
>
> public:
> doSomething(); //method explains what the slot does for other people to
> connect to
> doSomethingElse();
> private:
> onSomeEvent(); //private handling when signals do something for us to
> connect to
> onAnotherEvent();
>
> having "onSettingsChanged" public is a bit wrong. "reload()" could be
> public.
>
> That said, for non-library code, a lot of this public/private doesn't
> matter greatly especially for an app so small.
>
>
>
I agree. I introduced dbusInterfaceProperties because of
DBUS_INTERFACE_PROPERTIES existance in dbus header (which I don't want to
include). I hope it have good-enough name to reflect it's content.
At same time I think that generally speaking there should be header (not in
KDE, but in system headers) for each standard (e.g. MPRIS), to allow
"one-place" updating and convenient development.
I want to leave this review-request in current state, but go to refactor
onSettingsChanged.
Shit it, once it's acceptable. I haven't commit access.
- Alexandr
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113803/#review43489
-----------------------------------------------------------
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
