<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="https://git.reviewboard.kde.org/r/115425/">https://git.reviewboard.kde.org/r/115425/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 15th, 2014, 2:38 p.m. CET, <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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="https://git.reviewboard.kde.org/r/115425/diff/3/?file=244160#file244160line372" style="color: black; font-weight: bold; text-decoration: underline;">global-presence-chooser.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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 GlobalPresenceChooser::onUserActivatedComboChange(int index)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">360</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QDBusMessage</span> <span class="n">message</span> <span class="o">=</span> <span class="n">QDBusMessage</span><span class="o">::</span><span class="n">createSignal</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span class="s">"/Telepathy"</span><span class="p">),</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">So the summary of this patch is that you're removing the activateNowPlaying signals and instead relying on changing the config and sending a configChanged method.
The slots still exist in the kded so this is very half finished.
It seems like all of these changes are just going round in circles changing things rather than coming up with a understanding of how things are meant to work and coming up with a solid design.
Martin: now playing is your area, can you please comment. There have been several people randomly hacking on this now and things are still broken. Can you please comment and fix this mess.
What is the purpose of the config and the nowPlaying signals? How is it /meant/ to work?</pre>
</blockquote>
<p>On February 16th, 2014, 5:17 a.m. CET, <b>James Smith</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;">As long as the kded dbus interface isn't used elsewhere it's probably redundant code originally there to trigger the first status update after enabling the nowplaying and essentially provide control over a semi-autonomous process thereafter.
The kconfig method (functionally) duplicates this transparently enough where nowplaying will enable when the config entry is updated and self-terminate when respectively disabled. The slots in the kded don't set the nowplaying config either, so its a fairly overlapping implementation. Can we remove the dbus logic or at least properly set the kconfig from it? We can have kconfig write the current enable status from activatenowplaying / deactivatenowplaying but that still requires kconfig also to a) check whether the plugin is enabled, and b) also to write out enabled status to kconfig, while still using dbus to activate / deactivate the plugin.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Originally there were two things - "enabled" and "active". Enabled was set/read from the config and when it was disabled, the nowplaying plugin would disconnect itself from dbus and would just sit there doing nothing. Active on the other hand was runtime only and was set by the contact list over dbus, it was meant to activate that plugin and "actively set the presence". At some point in history the enabled was actually sort of merged with active in the nowplaying plugin.
The original code - dbus activation signals - will continue working as the slots are still present in the nowplaying plugin and everything is still ok even without this patch afaics. But I think there's some merit in your patch. But as it now behaves as a checkbox and it's really unclear to the user that this is what will happen, I'd like the entry in the presence menu to have an actual checkbox and loose all the dialogs asking questions. Additionally as I stated before, the setting in the KCM should just hide the presence in the combobox altogether (yes I've given it some thought and reevaluated that bug (also note I've never disagreed nor actually posted anything in the bug comments)).</pre>
<br />
<p>- Martin</p>
<br />
<p>On February 15th, 2014, 2:25 p.m. CET, James Smith wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://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 and Martin Klapetek.</div>
<div>By James Smith.</div>
<p style="color: grey;"><i>Updated Feb. 15, 2014, 2:25 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
ktp-contact-list
</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;">Enables / disables Now Playing in systemsettings every time it is enabled / disabled in the contact list.
Fixes systemsettings kcm showing nowplaying enabled while the contact list has disabled its functionality.</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;">Compile, run.</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>global-presence-chooser.cpp <span style="color: grey">(2047473)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115425/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>