Review Request: Fix to avoid duplicate chat tabs

[David Edmundson]

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


I like what this is doing. It's definitely the right approach to this, but I have some comments below.


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

    comparing titles isn't safe.
    
    I quite often switch from talking to someone on MSN to Jabber, it's an entirely new channel, but they both have the same title.
    
    Suggestion: we expose access to the textChannel in lib/ChatWidget.cpp as a public method, then compare these.



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

    I don't like creating a widget only to delete it again immediately.
    
    This becomes a much bigger change, but can I suggest we change
    
    addTab(ChatWidget*){..}
    
    to startChat(Tp::TextChannel)
    {
     //look for tab
    
     if(!notExistingTabFound){
       //create the tab widget
     }
    }
    
    and make the change to telepathy-chat-ui.cpp as appropriate.



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

    coding standard: we don't have spaces near brackets.
    
    setCurrentIndex(index);
    
    not 
    setCurrentIndex(    index   );


- David


On March 9, 2011, 3:55 p.m., Francesco Nwokeka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100825/
> -----------------------------------------------------------
> 
> (Updated March 9, 2011, 3:55 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> Simple patch to avoid creating duplicate chat tabs
> 
> 
> Diffs
> -----
> 
>   app/chat-window.cpp 66f7c03 
> 
> Diff: http://git.reviewboard.kde.org/r/100825/diff
> 
> 
> Testing
> -------
> 
> Tested it by adding three tabs and then requesting the same three tabs again. As expected i was show the already opened chat tab and no new tab was created.
> 
> 
> Thanks,
> 
> Francesco
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110309/281e674d/attachment.htm