Review Request 113803: TelepathyMPRIS: Refactor.

[David Edmundson]

> On Nov. 12, 2013, 7:41 a.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.

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. 


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113803/#review43489
-----------------------------------------------------------


On Nov. 12, 2013, 6:59 a.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, 6:59 a.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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20131112/35388219/attachment-0001.html>