<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/112987/">http://git.reviewboard.kde.org/r/112987/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 29th, 2013, 3:24 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="http://git.reviewboard.kde.org/r/112987/diff/1/?file=193067#file193067line84" style="color: black; font-weight: bold; text-decoration: underline;">KTp/im-persons-data-source.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">83</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="n">QString</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span class="s">"ktp://"</span><span class="p">)</span> <span class="o">+</span> <span class="n">contactManager</span><span class="o">-></span><span class="n">accountForContact</span><span class="p">(</span><span class="n">contact</span><span class="p">)</span><span class="o">-></span><span class="n">uniqueIdentifier</span><span class="p">()</span> <span class="o">+</span> <span class="n">QLatin1Char</span><span class="p">(</span><span class="sc">'#'</span><span class="p">)</span> <span class="o">+</span> <span class="n">contact</span><span class="o">-></span><span class="n">id</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;">this can crash.
if the account is offline, accountForContact won't work, at best it will return a null accountptr, which will then crash.
This is important as it is called from onContactInvalidated.</pre>
</blockquote>
<p>On September 30th, 2013, 8:23 a.m. UTC, <b>Daniele E. Domenichelli</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'm not sure what to return here when the account is null... something like ktp://invalid_account#<contact_id> would work?
Can the contact be invalid as well?</pre>
</blockquote>
<p>On September 30th, 2013, 8:35 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;">no, you _need_ it to return the correct account ID otherwise you won't delete the contact from the hash map correctly.
It will return the wrong account because the code for matching contacts --> account is
foreach(account, allAccounts) {
if (account->connection == contact->contactManager->connection) {
return account;
}
}
This works whilst you're online, but as soon as you're offline both of those are null.
(EDIT: from the above snippit it makes it sound like that will work, but we have a check for nulls and just return a null AccountPtr)
It's pretty terrible.
I guess we need to rethink how we do that.
Long term a connection->account() method in TpQt will be ideal.
Short term, we either need to not do this call here, or we need a better hack for doing accountForContact. Maybe a hashmap inside GlobalContactManager?</pre>
</blockquote>
<p>On September 30th, 2013, 2:17 p.m. UTC, <b>Daniele E. Domenichelli</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;">Can we save the account unique identifier inside KTp::Contact? Or is the contact going to be null as well?
If that's the case, how was that not crashing before?</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;">It didn't crash because it only used the contactId before;
accountForContact was only used for channel starting which has guards around null accounts.
We could store the account ID in the contact, the question is finding a neat place to set it when the contact is created.
I don't want to have a public member in KTp::Contact that is only valid half the time, depending on whether the contact happens to have gone through GlobalcontactManager or not. </pre>
<br />
<p>- David</p>
<br />
<p>On September 29th, 2013, 12:35 a.m. UTC, Daniele E. Domenichelli 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, David Edmundson and Martin Klapetek.</div>
<div>By Daniele E. Domenichelli.</div>
<p style="color: grey;"><i>Updated Sept. 29, 2013, 12:35 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
ktp-common-internals
</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;">Use nco:contactUID instead of nco:imID to identify a contact
NOTE: Changes in this review request https://git.reviewboard.kde.org/r/112970/ are required for this to work</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;">Tested with contact list, changed presence a few times.
No more duplicates, presence works 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>KTp/im-persons-data-source.cpp <span style="color: grey">(ebd01bae2f029ba5335c99babedec31b88cd7620)</span></li>
<li>KTp/Models/kpeopletranslationproxy.cpp <span style="color: grey">(a9bce378271006a9003c7757e49ba5b76ea97223)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112987/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>