<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>
<p>On February 13th, 2013, 12:03 a.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;">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>
</blockquote>
<p>On February 13th, 2013, 12:36 a.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;">Personally, I'd like to keep it simple for the moment, since we don't have that many users yet and we're still seeing where this goes, or at least I am. I fear that using this ConversationQueue manager won't help there.
OTOH, if you commit yourself to maintaining it this can go in (that's why I pushed you into merging it some time ago).
When will you be able to work on it and adapt it to the current codebase? Maybe I should push this patch until you're done?
</pre>
</blockquote>
<p>On February 13th, 2013, 1:07 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;">@Lasath. You've still not explained how it's possible to have two plasmoids where the chats are in a different order and that the global manager has any use.
</pre>
</blockquote>
<p>On February 13th, 2013, 3:32 a.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;">The use of the global manager is that it keeps active conversations in a queue and thus will make them pop out in the order that messages were received in. I personally think it's much more natural that way than them popping up in the order they are in the model (which is what apol's patch does). The multiple plasmoid use case was a mere hypothetical (which may or may not be relevant).
And I won't be able to do any development till I get home at the end of the month at least so merging this patch for now isn't a bad idea - I just wanted to voice the need for some parts of the plasmoid to be refactored in the long run. </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;">Can I get a "ship it" then? Otherwise I can't move forward to new adventures... :/</pre>
<br />
<p>- Aleix</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>