----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102193/#review5341 -----------------------------------------------------------
Good effort, especially for your second patch. I have quite a few comments. Watch for coding style. I really don't like "toggleSelection", if I'm wrong comment back and tell me why your solution is needed (I am wrong a lot, so feel free to question me) lib/chat-widget.cpp <http://git.reviewboard.kde.org/r/102193/#comment4825> Watch for coding style KDE policy is if (foo) { } See http://techbase.kde.org/Policies/Kdelibs_Coding_Style lib/chat-widget.cpp <http://git.reviewboard.kde.org/r/102193/#comment4830> As I read this, you're not really toggling a selection. Only removing it. So rename this. removeActiveSelection? Also this is all very weird: If I'm reading this correctly If the sender is the chatArea clear the sendMessageBox if the sender is the sendMessageBox clear the chatArea's search. Why not just connect the signal from the two sources to two different slots? For bonus points, make these slots in the relevant classes, rather than here. lib/chat-widget.cpp <http://git.reviewboard.kde.org/r/102193/#comment4827> coding standard: always use braces if (foo) { } never if (foo) bar lib/chat-widget.cpp <http://git.reviewboard.kde.org/r/102193/#comment4828> and here lib/chat-widget.cpp <http://git.reviewboard.kde.org/r/102193/#comment4829> and here lib/chat-widget.cpp <http://git.reviewboard.kde.org/r/102193/#comment4831> QEvents shouldn't really be proxied through signals/slots. It's what event handlers are for. We're completely going against the magic of event propagation. However - this was messed up before, so I'm happy to keep this, but I might change it in future. - David On Aug. 3, 2011, 10:45 a.m., Jan Gerrit Marker wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102193/ > ----------------------------------------------------------- > > (Updated Aug. 3, 2011, 10:45 a.m.) > > > Review request for Telepathy. > > > Summary > ------- > > The patch was created with the purpose to enable text copying by pressing > Ctrl+C. This was more complicated than I thought so I tried to implement a > new concept of handling the keys: > My goal was to let the line edit and the view behave like there were one > widget regarding text selection. If text in the line edit is marked there is > no text marked in the view and vice versa. In order to achieve this I let the > widget which contains both handle the key events by connecting to a signal in > the two classes which get's emitted when a key is pressed. > > > Diffs > ----- > > lib/adium-theme-view.h 0507128 > lib/adium-theme-view.cpp 0eb1090 > lib/chat-text-edit.h c086010 > lib/chat-text-edit.cpp ea96034 > lib/chat-widget.h 06e98a1 > lib/chat-widget.cpp 534ee1c > > Diff: http://git.reviewboard.kde.org/r/102193/diff > > > Testing > ------- > > I tested it for a day and there were no problems. > > > Thanks, > > Jan Gerrit > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
