<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/116940/">https://git.reviewboard.kde.org/r/116940/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 26th, 2014, 11:58 a.m. UTC, <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/116940/diff/2/?file=257308#file257308line142" style="color: black; font-weight: bold; text-decoration: underline;">telepathy-module.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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 TelepathyModule::onAccountManagerReady(Tp::PendingOperation *op)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">137</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</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">142</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="n">m_lastUserStatusMessage</span> <span class="o">!=</span> <span class="n">presence</span><span class="p">.</span><span class="n">statusMessage</span><span class="p">())</span> <span class="p">{</span></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;">I don't understand why we're splitting status messages and statuses.
You're not saving disk writes/anything as the next if statement is going to trigger anyway as the statusMessage in the struct will differ.
You're now actually doubling it. </pre>
</blockquote>
<p>On March 28th, 2014, 10:44 a.m. UTC, <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;">No, the variables are required separately to (for example) keep the status message returning to a custom message after nowPlaying terminates. Stuffing it into a new presence variable works just as well for saving and leave no changes for autoconnect, but adds 3 lines of code including a new global presence specifically for writing to disk.</pre>
</blockquote>
<p>On March 29th, 2014, 12:33 p.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;">What confuses me is in the original design this bug shouldn't exist. m_lastUserPresence should contain the last *user set presence* which is what we should return to after all plugins terminate.
Clearly if m_lastUserPresence doesn't contain the last user presence something is very wrong. Anything else is either a bodge _OR_ the original design is flawed and we need to move to a new design which includes renaming this variable (and/or variable type to Tp::ConnectionPresenceType)
</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;">The ability to engage nowPlaying from the contact list which coincidentally with a little work can override the custom-set presence message (I'm personally in favor of warning the user to start from an empty status) is probably why the variable gets ahead of itself as part of a pair (presence - status message). When there is a user-set presence with status message the last-known user set status message is still required for when the nowPlaying plugin disengages, and so the variable is full. The variable is also required when competing to become the next visible status message. So, what we do is try and keep it separate until its paired with the presence and written to disk, and probably in the same go re-set as the visible status message, thus keeping it useable enough to perform status message replacement operations from its compares.
if m_lastUserPresence.statusMessage().isEmpty() isn't true I can't properly compare for the next course of action, which may or may not include using m_lastUserPresence.statusMessage() in some role. m_lastUserPresence.statusMessage() can currently be overwritten somewhat unpredictably in onRequestedPresenceChanged().
In the current solution, for instance, if presence.statusMessage() is not empty then bail before the presence is reset on-the-go before the user knows there's a visible status change. This code got around a bug which slightly hid a falling-over (with non-separate) presence + status message variables statusMessageStack(), due to comparison issues where there's a custom message, in addition to a plugin-supplied presence message. The current code may be more "correct", but the contact list still allows engaging from custom messages, meaning that variable isn't always verified properly filled, or the plugin engage logic falls over because the presence status message isn't empty despite not being the top contender for visible status message. Seen in addition to the ability for the presence status message to change from an external event, with a non-empty m_lastUserPresence.statusMessage() the compare broke with (in the same session) a new status message from nowPlaying which showed a problem where previously a isEmpty() on the m_lastUserPreesence.statusMessage() worked to keep the proper status message visible.
Given status messages changes do necessarily occur with or without a change in presence, we don't know with certainty what the current status message is supposed to be, and what the user-set status message is supposed to be, or have any way to change between the two for the user. I think the bug is ultimately caused by the requirement to set a plugin-based status message when the user-set custom message must be also juggled in the switch to a plugin status message. When m_lastUserPresence.statusMessage() isn't empty the logic falls over trying to compare.
I like the current .8 code because it works well with the presence applet. Most of these changes are required to allow the contactlist to work with a custom message, where instead I would just warn the user not to engage the nowPlaying from a custom status message, and make sure it turns on and off properly.</pre>
<br />
<p>- James</p>
<br />
<p>On March 30th, 2014, 3:15 a.m. UTC, 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.</div>
<div>By James Smith.</div>
<p style="color: grey;"><i>Updated March 30, 2014, 3:15 a.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=332082">332082</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;">Fixes engage-nowPlaying-from-custom-status-message and lessens disk activity on presence change. Also simplifies logic used to keep telepathy-kded-module in a "fit" state during run-time.</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>telepathy-module.cpp <span style="color: grey">(030a0d9)</span></li>
<li>telepathy-module.h <span style="color: grey">(2213cdf)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/116940/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>