----------------------------------------------------------- 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 > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
