<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>
<p>On September 30th, 2013, 2:23 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;">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>
</blockquote>
<p>On September 30th, 2013, 2:49 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;">Something like KTp::Contact::Contact() -> manager->connection()->objectPath().remove(TP_QT_CONNECTION_OBJECT_PATH_BASE) could work?</pre>
</blockquote>
<p>On September 30th, 2013, 3:26 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;">\o/ YES!
I like it.
It still needs a minor tweak, but we can use it.
IIRC contact only has a weakptr to contactManager and vice versa (or maybe it's contactManager->connection) so it's _possible_ to get null here.
You'll have to check, I seem to remember it from somewhere.
So either:
we keep a reference to the connection somewhere
or
we set the accountID (or even a weakptr to the account) in the Contact constructor.</pre>
</blockquote>
<p>On September 30th, 2013, 3:40 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;">Hmm /o\ NO
I just realized that's a different string for gabble... I think the xmpp resource name is appended after a "_2f" :(
We could check for that and cut the string, but I'm not sure if it is safe...
An alternative is to use everywhere the _connection_ ID instead of the _account_ ID</pre>
</blockquote>
<p>On September 30th, 2013, 3:41 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;">But that means that if you change the resource name you will lose your metacontacts, so not a good option</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;">This in KTp::Contact constructor is a bit hackish, but works
QString connectionUid(manager->connection()->objectPath().remove(TP_QT_CONNECTION_OBJECT_PATH_BASE));
QString accountUid = connectionUid.left(connectionUid.indexOf(QLatin1String("_2f"))) + QLatin1String("0");
QString contactUid = QString(QLatin1String("ktp://") + d->accountUid + QLatin1Char('#') + id());
Unfortunately even if we assume that QLatin1String("_2f") is ok, there is the "+ QLatin1String("0")" that breaks everything if one configures 2 accounts of the same type with the same id, for the second account it should be "QLatin1String("1")"
So I think we'll have to find another way... suggestions?
</pre>
<br />
<p>- Daniele E.</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>