Review Request: Allow tabs to be dettached from window

[David Edmundson]

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


Code is generally really really good. You've done it pretty much how I was expecting.

I can see one outstanding quite large issue.
 Try doing this:
    open 2 chats
    detatch a tab
    wait till person B changes state.
    the new tab won't update the icon in the window - it may even still change the icon in the wrong window
    
Reason you only call setupTabSignals in createTab, not in addTab.
Also you don't ever disconnect the signals when you removeTab

I don't like ChatWindow having createTab and addTab - and it being structured in a way that createTab doesn't call addTab.
I would even consider moving createTab to TelepathyChatUi, but that's up to you.

I'd like to see another diff with Martin's comments and this issue addressed. Then I think we'll be good to ship.


app/chat-window.cpp
<http://git.reviewboard.kde.org/r/101862/#comment3767>

    For bonus points disable these actions if you can't move it left or right.



app/chat-window.cpp
<http://git.reviewboard.kde.org/r/101862/#comment3752>

    Why.. you delete it on the next line?



app/telepathy-chat-ui.cpp
<http://git.reviewboard.kde.org/r/101862/#comment3751>

    This is slightly dangerous - what if there are no chat Windows? You'll access out of bounds.
    
    In reality, you're probably safe as soon as last window closes the chat handler will close, though there is a very small risk of getting a new chat whilst you're closing the window at which point it could go wrong.
    
    Check window exists before you use it.
    
    In the next refactor that I can see happening I think we'll move creating windows to here.
    


- David


On July 6, 2011, 5:24 a.m., Lasath Fernando wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101862/
> -----------------------------------------------------------
> 
> (Updated July 6, 2011, 5:24 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> Okay, internet's really sketchy here - I don't have time to type a long description. 
> 
> I'm not sure I handled the result from KMenu::exec() right, but apart form that my changes *should* be fairly straightforward in the diff.
> 
> http://quickgit.kde.org/?p=clones%2Ftelepathy-chat-handler%2Ffernando%2FdetachableTabs.git&a=shortlog&h=refs/heads/refractored_tabs
> 
> 
> Diffs
> -----
> 
>   app/chat-tab.h 2175fe2 
>   app/chat-tab.cpp 23c912e 
>   app/chat-window.h 2b2b70d 
>   app/chat-window.cpp 517b694 
>   app/telepathy-chat-ui.h 306912d 
>   app/telepathy-chat-ui.cpp 1654a9d 
> 
> Diff: http://git.reviewboard.kde.org/r/101862/diff
> 
> 
> Testing
> -------
> 
> Detached a few conversations with friends. 
> 
> 
> Thanks,
> 
> Lasath
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110706/18045d6a/attachment.htm