<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/113803/">http://git.reviewboard.kde.org/r/113803/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 12th, 2013, 7:41 a.m. UTC, <b>Alexandr Akulich</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
<br />
<p>- David</p>
<br />
<p>On November 12th, 2013, 6:59 a.m. UTC, Alexandr Akulich wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Telepathy.</div>
<div>By Alexandr Akulich.</div>
<p style="color: grey;"><i>Updated Nov. 12, 2013, 6:59 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
ktp-kded-module
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks like it works as well, as before refactor. (Tested with amarok and dragon player).</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>telepathy-mpris.h <span style="color: grey">(c223e94)</span></li>
<li>telepathy-mpris.cpp <span style="color: grey">(93875fe)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/113803/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>