> On March 26, 2011, 2:06 a.m., David Edmundson wrote: > > lib/chat-widget.cpp, line 686 > > <http://git.reviewboard.kde.org/r/100948/diff/1/?file=13030#file13030line686> > > > > Why do we take the text as an arugment to the method yet take the flags > > from the searchbar. > > > > I would suggest either both as arguments, or both from the search bar. > > > > Unless there's a good reason not to. > >
You have a point here. Making all as arguments passed by the ChatSearchBar class. > On March 26, 2011, 2:06 a.m., David Edmundson wrote: > > lib/chat-widget.h, line 120 > > <http://git.reviewboard.kde.org/r/100948/diff/1/?file=13029#file13029line120> > > > > It's a bit unusual to write the word 'slot' in the method name. > > > > I like using onSomeEvent for my private slots, and normal method names > > for my public slots. > > onFlagsChanged it is > On March 26, 2011, 2:06 a.m., David Edmundson wrote: > > lib/chat-search-bar.cpp, line 85 > > <http://git.reviewboard.kde.org/r/100948/diff/1/?file=13028#file13028line85> > > > > Don't hardcode colours. > > > > There's a whole set of colours you can get from KPalette. Right, set colors to respect user desktop settings with "KColorScheme" > On March 26, 2011, 2:06 a.m., David Edmundson wrote: > > app/chat-window.cpp, line 164 > > <http://git.reviewboard.kde.org/r/100948/diff/1/?file=13025#file13025line164> > > > > If you're not going to use these why bother having them in your slot > > arguments? > > > > A slot can have fewer parameters that the signal it connects to. Didn't know about this. Updated accordingly > On March 26, 2011, 2:06 a.m., David Edmundson wrote: > > lib/chat-widget.h, line 104 > > <http://git.reviewboard.kde.org/r/100948/diff/1/?file=13029#file13029line104> > > > > I would consider > > > > searchComplete(bool textFound); > > same for the two slots in the chat-search-bar, which are basically the > > same but with different colors. good point. Fixed > On March 26, 2011, 2:06 a.m., David Edmundson wrote: > > lib/chat-widget.cpp, line 708 > > <http://git.reviewboard.kde.org/r/100948/diff/1/?file=13030#file13030line708> > > > > If you turn on case-senstive you don't update the searchTextFound (or > > not). > > > > Why do you need to findText(QString()); ? > > > > I would highly consider merging this with > > findTextInChat as to me it doesn't seem to be anything different. > > > > > > > > And yet another good point - Francesco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100948/#review2181 ----------------------------------------------------------- On March 26, 2011, 1:43 a.m., Francesco Nwokeka wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/100948/ > ----------------------------------------------------------- > > (Updated March 26, 2011, 1:43 a.m.) > > > Review request for Telepathy. > > > Summary > ------- > > Implementation of "search" within the chat window. > The search widget is called via keyboard shortcut ( standard "ctrl+f ") and > can also be modified by the user from the chat settings. > > > This addresses bug 269052. > http://bugs.kde.org/show_bug.cgi?id=269052 > > > Diffs > ----- > > app/chat-window.h cde19e4 > app/chat-window.cpp b3407db > lib/CMakeLists.txt 1639cfd > lib/chat-search-bar.h PRE-CREATION > lib/chat-search-bar.cpp PRE-CREATION > lib/chat-widget.h dfdef64 > lib/chat-widget.cpp 5ddf3dd > lib/chat-widget.ui 2dce82f > > Diff: http://git.reviewboard.kde.org/r/100948/diff > > > Testing > ------- > > wrote in a chat and looked for random text. Please try the patch and tell me > if some functionality is missing > > > Thanks, > > Francesco > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
