<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/108896/">http://git.reviewboard.kde.org/r/108896/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 12th, 2013, 3:56 p.m. UTC, <b>Lasath Fernando</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/108896/diff/1/?file=113528#file113528line55" style="color: black; font-weight: bold; text-decoration: underline;">KTp/Declarative/conversations-model.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class ConversationsModel : public QAbstractListModel, public Tp::AbstractClientHandler</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">55</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">int</span> <span class="nf">nextActiveConversation</span><span class="p">(</span><span class="kt">int</span> <span class="n">first</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 don't think this is the right way to go.
If you look at https://git.reviewboard.kde.org/r/107833/diff/#1 , you'll see that I greatly simplified ConversationQueueManager.
I considered replacing it with this approach, but I kept it around for 2 reasons.
1. It helps get rid of the *horrible* binding loop with base.currentIndex.
2. I want to have the possibility of this working across multiple instances of this plasmoid (which should be possible, since K_GLOBAL_STATIC should ensure there's only one instance across all of plasma).</pre>
</blockquote>
<p>On February 12th, 2013, 6:04 p.m. UTC, <b>Aleix Pol Gonzalez</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;">1. What binding loop are you talking about?
2. this won't work, the activate shortcut is targeted to 1 plasmoid. If we want something more sophisticated, it should be considered KTp-wide. (as in it should focus a window, if there's a chat on a window as well)</pre>
</blockquote>
<p>On February 12th, 2013, 9:21 p.m. UTC, <b>Lasath Fernando</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;">1. I can't run the plasmoid atm so I can't tell you exactly. But IIRC it was something to do with the kludge that kept the currentIndex of the model, the visible property of the delegate (button) and the visible property of the Dialog as well as the visibleToUser property of the model in sync, trying (but failing) to propagate the changes in both directions. I've been wanting to redesign that for a long time and I don't think we should be using the 'index' of a conversation any more than we have to at this stage.
2. Having a KTp-wide solution would be nice, but a little out of scope (and I don't know how we could such a thing). But the approach in my patch should work because each plasmoid would have a ConversationQueueManager exposed to it, and since it's internally a global static anyway, they should all share the same queue. Then when any plasmoid calls dequeueNext(), the ConversationQueueManager would call a method in the MessagesModel which will emit a signal to the appropriate plasmoid which will make the conversation visible. Makes sense? :-)</pre>
</blockquote>
<p>On February 12th, 2013, 9:42 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;">In reply to 2.
How does this work better for Plasma. Chat plasmoid always shows all chats. So there's no way to have two chat plasmoids such that you would have an unread message in one and not in another. So what problem actually exists that your method solves, that Aleix's doesn't.
Also I like using Plasma's widget activation, using the shortcut there. Is there any way to combine that in here? Does your method actually work with a shortcut currently?</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;">My patch does make it use plasma's widget activation. (I ditched the useless hardcoded shortcut in ConversationQueueManager)
And the reason I'm asking you guys to reconsider my patch is that it cleans up some badly written code, makes the shortcuts work correctly, and undoes a couple of bad design decisions I made a year ago. (And fixes a couple of other minor issues) It's unfortunate that it couldn't be merged before I left and I think it's worth going in.
PS: Yes I know there isn't much point in having the shortcuts consistent across multiple plasmoids, but that's just a happy side effect of getting them to maintain the correct order.</pre>
<br />
<p>- Lasath</p>
<br />
<p>On February 11th, 2013, 12:57 a.m. UTC, Aleix Pol Gonzalez 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 and David Edmundson.</div>
<div>By Aleix Pol Gonzalez.</div>
<p style="color: grey;"><i>Updated Feb. 11, 2013, 12:57 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 way, we can have a key binding to jump to the next important conversation.
This new method is used in: https://git.reviewboard.kde.org/r/108897/</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;">manual testing :/</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/Declarative/conversations-model.h <span style="color: grey">(60bd6dc)</span></li>
<li>KTp/Declarative/conversations-model.cpp <span style="color: grey">(2c6007f)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108896/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>