<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/113066/">http://git.reviewboard.kde.org/r/113066/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks pretty good to me.
first topic needs discussion, once we agree on something consider this a ship it.
(would be good if someone else has a look too, it's a big change)</pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/113066/diff/1/?file=193637#file193637line407" style="color: black; font-weight: bold; text-decoration: underline;">config/telepathy-kded-config.ui</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">407</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <string>Enable "Now playing..." presence</string></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">407</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <string>Enable "Now playing..." presence<span class="hl"> on Login</span></string></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is an i18n change and we're in string freeze.
We can either:
- not push patch this in 0.7
- push it without this one line change and do that in a new commit in master
- email i18n teams and beg forgiveness
- not have this string, restore now playing if it was running
- or simply don't restore it at login at all, it's quite easy to activate.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/113066/diff/1/?file=193639#file193639line165" style="color: black; font-weight: bold; text-decoration: underline;">telepathy-mpris.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void TelepathyMPRIS::serviceNameFetchFinished(QDBusPendingCallWatcher *callWatcher)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">158</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">isEnabled</span><span class="p">()</span> <span class="o">&&</span> <span class="n">pluginEnabled</span><span class="p">)</span> <span class="p">{</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">138</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// reenable/disable nowplaying if user just change some other unrelated settings in kcm</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">super pedantic nitpick
reenable -> re-enable
change->changed</pre>
</div>
<br />
<p>- David Edmundson</p>
<br />
<p>On October 2nd, 2013, 7:47 p.m. UTC, Xuetian Weng 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 Xuetian Weng.</div>
<p style="color: grey;"><i>Updated Oct. 2, 2013, 7:47 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=307582">307582</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=313126">313126</a>
</div>
<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;">This patch contains following changes:
1. disconnect for PropertiesChanges when knownPlayer is reset, the disappered service will not automatically disconnect from dbus.
2. change the "Enable" option to "Enable On Login" (bug 307582) and hence we can simplifies the logic (no more presenceActivated)
3. Changes now playing status at contact list will be remembered only for current login.
4. Fix the wrong behavior of handling PropertiesChanges (invalidate properties is not considered).
5. use requestedPresence to evaluate the current status (part of bug 313126)
6. since it changes to "Enable On Login", always enable the nowplaying text lineedit in kcm.
Current logic:
1. If the enabled option in config changes, set the plugin enable state (this must be caused by user change in kcm).
2. receive onActivate / onDeactivate from contact list, set the plugin enable state.
3. when enable state change to true, start detecting player. (activatePlugin(bool))
4. when enable state change to false, unwatch all player, reset all other state. (activatePlugin(bool))
5. when new player appears, request playbackStatus and metadata by GetAll, and then watch on the PropertiesChange for this player.
Some improvement:
1. not always listening on expensive serviceOwnerChanged.
2. be able to enable nowplaying on login.
3. fix leak connection of dbus signal.
4. code should now be clearer instead of having too much boolean check.
</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;">Everything seems to work as expected.</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>config/telepathy-kded-config.cpp <span style="color: grey">(953a637)</span></li>
<li>config/telepathy-kded-config.ui <span style="color: grey">(09729e7)</span></li>
<li>telepathy-mpris.h <span style="color: grey">(e3378d4)</span></li>
<li>telepathy-mpris.cpp <span style="color: grey">(6cd3e20)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/113066/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>