<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/101380/">http://git.reviewboard.kde.org/r/101380/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 18th, 2011, 11:34 a.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/101380/diff/3/?file=16628#file16628line277" style="color: black; font-weight: bold; text-decoration: underline;">groups-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; ">void GroupsModel::removeContactFromGroup(ProxyTreeNode* proxyNode, const QString& group)</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">277</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">beginRemoveRows</span><span class="p">(</span><span class="n">index</span><span class="p">(</span><span class="n">proxyNode</span><span class="o">-></span><span class="n">parent</span><span class="p">()),</span> <span class="n">proxyNode</span><span class="o">-></span><span class="n">parent</span><span class="p">()</span><span class="o">-></span><span class="n">indexOf</span><span class="p">(</span><span class="n">proxyNode</span><span class="p">),</span> <span class="n">proxyNode</span><span class="o">-></span><span class="n">parent</span><span class="p">()</span><span class="o">-></span><span class="n">indexOf</span><span class="p">(</span><span class="n">proxyNode</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;">beginRemoveRows code should NOT be here.
removing the proxyNode will emit "itemsRemoved" and it will all hit your "onItemsRemovedCode" which does it.</pre>
</blockquote>
<p>On May 18th, 2011, 12:29 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;">Actually it won't. The signal is never emitted and the node is removed directly. Could use some rethinking though.</pre>
</blockquote>
<p>On May 18th, 2011, 1:05 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;">yes it will, the next line is proxyNode->remove();
proxyNode->remove will call TreeNode::remove which will call deleteLater()
this will call proxyNode::onRemoved, which will emit itemRemoved, this propogates down the tree and hits groupsModel::onItemsRemoved.</pre>
</blockquote>
<p>On May 18th, 2011, 1:14 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;">Now that you wrote that, it should indeed work like that, but GroupsModel::onItemsRemoved was never called when I tested this. Let me check again.</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;">Ok, so no, it's not like that. proxyNode::onRemoved is called only on sourceNode (ContactModelItem) being destroyed, which does not happen in this case, because the source contact is not really touched, therefore the itemRemoved will never be emitted. So instead of calling remove() directly, it should call the onRemoved() slot to get the signal emitted as expected.
So I propose to rename the onRemoved() slot to something better and then to call it directly instead of the code above. Is that ok?</pre>
<br />
<p>- Martin</p>
<br />
<p>On May 18th, 2011, 8:43 a.m., Martin Klapetek 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 Martin Klapetek.</div>
<p style="color: grey;"><i>Updated May 18, 2011, 8:43 a.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;">This patch adds cross-account groups to contact list.</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 several accounts with groups, also tried moving contacts between groups from another client. </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>CMakeLists.txt <span style="color: grey">(919df77)</span></li>
<li>account-filter-model.h <span style="color: grey">(6591355)</span></li>
<li>account-filter-model.cpp <span style="color: grey">(75f2126)</span></li>
<li>contact-delegate.h <span style="color: grey">(46fea76)</span></li>
<li>contact-delegate.cpp <span style="color: grey">(209e715)</span></li>
<li>groups-model-item.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>groups-model-item.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>groups-model.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>groups-model.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>main-widget.h <span style="color: grey">(7a5e417)</span></li>
<li>main-widget.cpp <span style="color: grey">(20e8003)</span></li>
<li>proxy-tree-node.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>proxy-tree-node.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>telepathy-kde-contactlist.notifyrc <span style="color: grey">(918736e)</span></li>
<li>tree-node.h <span style="color: grey">(adeefa4)</span></li>
<li>tree-node.cpp <span style="color: grey">(f892d5a)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/101380/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>