<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>
<p>On November 12th, 2013, 7:56 a.m. UTC, <b>David Edmundson</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 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>
</blockquote>
<p>On November 12th, 2013, 8:43 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 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.</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;">>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.
There's a command to automatically generate a header file from the dbus XML file.
Best example in our code is probably ktp-text-ui/filters/texttospeech
</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>