Review Request 108896: Add a method to jump to know what's the next active conversation

[Aleix Pol Gonzalez]

> On Feb. 12, 2013, 3:56 p.m., Lasath Fernando wrote:
> > KTp/Declarative/conversations-model.h, line 55
> > <http://git.reviewboard.kde.org/r/108896/diff/1/?file=113528#file113528line55>
> >
> >     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).
> 
> Aleix Pol Gonzalez wrote:
>     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)
> 
> Lasath Fernando wrote:
>     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? :-)
> 
> David Edmundson wrote:
>     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?
> 
> Lasath Fernando wrote:
>     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.
> 
> Aleix Pol Gonzalez wrote:
>     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?
>     
>
> 
> David Edmundson wrote:
>     @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.
>     
>
> 
> Lasath Fernando wrote:
>     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.

Can I get a "ship it" then? Otherwise I can't move forward to new adventures... :/


- Aleix


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108896/#review27308
-----------------------------------------------------------


On Feb. 11, 2013, 12:57 a.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108896/
> -----------------------------------------------------------
> 
> (Updated Feb. 11, 2013, 12:57 a.m.)
> 
> 
> Review request for Telepathy and David Edmundson.
> 
> 
> Description
> -------
> 
> 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/
> 
> 
> Diffs
> -----
> 
>   KTp/Declarative/conversations-model.h 60bd6dc 
>   KTp/Declarative/conversations-model.cpp 2c6007f 
> 
> Diff: http://git.reviewboard.kde.org/r/108896/diff/
> 
> 
> Testing
> -------
> 
> manual testing :/
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130215/fd1d91d7/attachment-0001.html>