<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/107811/">http://git.reviewboard.kde.org/r/107811/</a>
</td>
</tr>
</table>
<br />
<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 like the concept.
I've got a big "no" on the review, for a TpQt reason that isn't obviously clear from just reading the docs.</pre>
<br />
<div>
<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/107811/diff/3/?file=103934#file103934line81" style="color: black; font-weight: bold; text-decoration: underline;">plasmoid/declarative-plugin/pinnedcontactsmodel.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; ">PinnedContactsModel::PinnedContactsModel(QObject* parent)</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">81</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">d</span><span class="o">-></span><span class="n">accountManager</span> <span class="o">=</span> <span class="n">Tp</span><span class="o">::</span><span class="n">AccountManager</span><span class="o">::</span><span class="n">create</span><span class="p">(</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Argh!
We now have two account factories in the same application. One from here, and one in the client registrar.
If you have two account factories, you can end up with different Tp::Account* objects representing the same account, such that account1 != account2 even though account1->uniqueIdentifier() == account2->uniqueIdentifier();
I guess you may have figured this out because you have the line:
"if p.account->uniqueIdentifer == account->uni..." rather than the more obvious "if p.account=account"
This isn't a situation I want to be in. Is there a way to share the factories (or the AccountManager) between here and the TelepathyTextObserver?
It may need a bit more moving code about, but it'll be worth it.
</pre>
</div>
<br />
<div>
<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/107811/diff/3/?file=103934#file103934line99" style="color: black; font-weight: bold; text-decoration: underline;">plasmoid/declarative-plugin/pinnedcontactsmodel.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; ">PinnedContactsModel::PinnedContactsModel(QObject* parent)</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">99</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KSharedConfig</span><span class="o">::</span><span class="n">Ptr</span> <span class="n">cfg</span> <span class="o">=</span> <span class="n">KSharedConfig</span><span class="o">::</span><span class="n">openConfig</span><span class="p">(</span><span class="n">KGlobal</span><span class="o">::</span><span class="n">mainComponent</span><span class="p">());</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">what file does this write into?
AFAIK plasma has a special config group you're meant to use.</pre>
</div>
<br />
<div>
<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/107811/diff/3/?file=103934#file103934line112" style="color: black; font-weight: bold; text-decoration: underline;">plasmoid/declarative-plugin/pinnedcontactsmodel.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; ">PinnedContactsModel::PinnedContactsModel(QObject* parent)</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">112</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">qDebug</span><span class="p">()</span> <span class="o"><<</span> <span class="s">"loading pinned...."</span> <span class="o"><<</span> <span class="n">pins</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">qdebug -> kdebug
(everywhere)</pre>
</div>
<br />
<div>
<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/107811/diff/3/?file=103934#file103934line126" style="color: black; font-weight: bold; text-decoration: underline;">plasmoid/declarative-plugin/pinnedcontactsmodel.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; ">PinnedContactsModel::PinnedContactsModel(QObject* parent)</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">126</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Tp</span><span class="o">::</span><span class="n">ContactPtr</span> <span class="n">contact</span> <span class="o">=</span> <span class="n">job</span><span class="o">-></span><span class="n">contacts</span><span class="p">().</span><span class="n">first</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">if the pending operation fails to find the contact (like if you've pinned a contact you've now removed) you get a crash.
</pre>
</div>
<br />
<div>
<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/107811/diff/3/?file=103934#file103934line139" style="color: black; font-weight: bold; text-decoration: underline;">plasmoid/declarative-plugin/pinnedcontactsmodel.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; ">PinnedContactsModel::PinnedContactsModel(QObject* parent)</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">139</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span><span class="n">i</span><span class="o"><</span><span class="n">d</span><span class="o">-></span><span class="n">m_pins</span><span class="p">.</span><span class="n">count</span><span class="p">())</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">if () {
}</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On January 2nd, 2013, 5:34 p.m., Aleix Pol Gonzalez 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 and David Edmundson.</div>
<div>By Aleix Pol Gonzalez.</div>
<p style="color: grey;"><i>Updated Jan. 2, 2013, 5:34 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;">I've wanted to use this plasmoid for longtime, I fear that the reason why I'm not using it yet is because I can't start chats from there. The plan is that this will improve the situation, to some extent.
The important part about the patch is that the root element is refactored into a grid that can have different elements. Now I added a button with a contactlist that probably should go, but eventually i'd like to have non-conversation buttons so that one can pin contacts.</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;">Very little, mostly sending this review for starting a discussion on where we'd like to go with this plasmoid.</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>plasmoid/declarative-plugin/CMakeLists.txt <span style="color: grey">(48ba8a7)</span></li>
<li>plasmoid/declarative-plugin/contactpin.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>plasmoid/declarative-plugin/contactpin.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>plasmoid/declarative-plugin/conversation-target.h <span style="color: grey">(cd45f2d)</span></li>
<li>plasmoid/declarative-plugin/conversation-target.cpp <span style="color: grey">(f9c285d)</span></li>
<li>plasmoid/declarative-plugin/conversation.h <span style="color: grey">(6eeab86)</span></li>
<li>plasmoid/declarative-plugin/conversation.cpp <span style="color: grey">(152d940)</span></li>
<li>plasmoid/declarative-plugin/conversations-model.h <span style="color: grey">(f9dc047)</span></li>
<li>plasmoid/declarative-plugin/conversations-model.cpp <span style="color: grey">(faaa60b)</span></li>
<li>plasmoid/declarative-plugin/ktp-metatypes.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>plasmoid/declarative-plugin/pinnedcontactsmodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>plasmoid/declarative-plugin/pinnedcontactsmodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>plasmoid/declarative-plugin/qml-plugins.cpp <span style="color: grey">(23a4291)</span></li>
<li>plasmoid/org.kde.ktp-chatplasmoid/contents/ui/ChatWidget.qml <span style="color: grey">(ea68f41)</span></li>
<li>plasmoid/org.kde.ktp-chatplasmoid/contents/ui/ContactList.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>plasmoid/org.kde.ktp-chatplasmoid/contents/ui/ConversationDelegate.qml <span style="color: grey">(8a8d851)</span></li>
<li>plasmoid/org.kde.ktp-chatplasmoid/contents/ui/ConversationDelegateButton.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>plasmoid/org.kde.ktp-chatplasmoid/contents/ui/main.qml <span style="color: grey">(feb766b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107811/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/107811/s/952/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2013/01/02/ktp-pinned_400x100.png" style="border: 1px black solid;" alt="i am pinned" /></a>
<a href="http://git.reviewboard.kde.org/r/107811/s/953/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2013/01/02/ktp-pinned1_400x100.png" style="border: 1px black solid;" alt="pin me" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>