<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/101108/">http://git.reviewboard.kde.org/r/101108/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 12th, 2011, 10:55 p.m., <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/101108/diff/3/?file=14240#file14240line106" style="color: black; font-weight: bold; text-decoration: underline;">account-filter-model.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">bool AccountFilterModel::lessThan( const QModelIndex & left, const QModelIndex & right ) const</pre></td>
</tr>
</tbody>
<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">106</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">leftPresence</span> <span class="o">=</span> <span class="n">sourceModel</span><span class="p">()</span><span class="o">-></span><span class="n">data</span><span class="p">(</span><span class="n">left</span><span class="p">,</span> <span class="n">AccountsModel</span><span class="o">::</span><span class="n">PresenceTypeRole</span><span class="p">).</span><span class="n">toUInt</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'm really not convinced this works, if you're going to treat them like ints then there's no point overriding this method at all.
I'm pretty confident (though I may be wrong) this will put offline users at the top, then online, away, busy.
I think that we want to treat them as the enum values they are and need something more like:
if (leftPresence == Tp::PresenceOnline) {
return false;
}
if (rightPresence == Tp::PresenceOnline {
return true;
}
if (leftPresence == Tp::PresenceAway) {
return false;
etc etc.
</pre>
</blockquote>
<p>On April 13th, 2011, 12:06 p.m., <b>Martin Klapetek</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;">The purpose of lessThan(..) is to compare values to do the sort and it seems like a good way to compare the uints (if they are ordered like 0-online, 1-away...7-offline, otherwise it's no use). In your proposed way it would have to compare all combinations of presences, no? Like if(leftPresence == Online && rightPresence == Away) return true; if(leftPresence == Online && rightPresence == Busy) return true;
or am I getting this wrong? :)
Also, Rémy, try to test with offline users shown.</pre>
</blockquote>
<p>On April 13th, 2011, 3:46 p.m., <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;">Your statement contains a massive "if" there.
I'm pretty sure the statuses are in this order:
http://telepathy.freedesktop.org/spec/Connection_Interface_Simple_Presence.html#Enum:Connection_Presence_Type
Which isn't a sensible order for listing.
Also I don't think we need to do every possibly combo if it's written like suggested, if either of them is "online" then that side 'wins' and you return either true or false. Otherwise you continue onto the next presence, if either of them are 'away' that way you only need each status once - though it's still pretty lengthy.
You may be right that it is easiest to turn the presence type into a rank score, which does have 0 for online, 7 for offline, then compare those. I would be happy with that. I just don't think it works as-is.</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;">Ahh, now I'm getting your logic in the code above. Yes, this might be actually a way better solution...as always :P</pre>
<br />
<p>- Martin</p>
<br />
<p>On April 12th, 2011, 9:05 p.m., Rémy Greinhofer wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Telepathy.</div>
<div>By Rémy Greinhofer.</div>
<p style="color: grey;"><i>Updated April 12, 2011, 9:05 p.m.</i></p>
<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;">Sorts the contacts by name offers the possibility to sort it also by status.</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;">1. Open the contact list, the contacts are sorted by name.
2. Click on the "Sort by onlineness button", the contacts are sorted by onlineness and by name.</pre>
</td>
</tr>
</table>
<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=246223">246223</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>account-filter-model.h <span style="color: grey">(d33231d)</span></li>
<li>account-filter-model.cpp <span style="color: grey">(7182362)</span></li>
<li>main-widget.h <span style="color: grey">(18f97ee)</span></li>
<li>main-widget.cpp <span style="color: grey">(a24ccf0)</span></li>
<li>main-widget.ui <span style="color: grey">(d000b9b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/101108/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>