> On Aug. 3, 2011, 12:06 p.m., David Edmundson wrote: > > lib/chat-widget.cpp, line 441 > > <http://git.reviewboard.kde.org/r/102193/diff/1/?file=30664#file30664line441> > > > > 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.
I already thought of implement it using an event handler, but signal/slots seemed easier to me at first. If you want to I'll port it. > On Aug. 3, 2011, 12:06 p.m., David Edmundson wrote: > > lib/chat-widget.cpp, line 425 > > <http://git.reviewboard.kde.org/r/102193/diff/1/?file=30664#file30664line425> > > > > coding standard: > > > > always use braces > > > > if (foo) { > > > > } > > > > never > > > > if (foo) > > bar > > Sorry about that, I'm used to new lines. I'll change it. > On Aug. 3, 2011, 12:06 p.m., David Edmundson wrote: > > lib/chat-widget.cpp, line 423 > > <http://git.reviewboard.kde.org/r/102193/diff/1/?file=30664#file30664line423> > > > > 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. This is the main point of the idea to make the two widgets (chatArea and sendMessageBox) behave like one: If chatArea changes its selection then sendMessageBox needs to clear its selection in order to have only one selection. I named it toggleSelection() as it changes the widget which has a selection. I'm open to name suggestions... The slots are in the ChatWidget class as I thought that it would be better to let the ChatWidget control the chatArea and the sendMessageBox if I want to create the user experience of using one widget. - Jan Gerrit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102193/#review5341 ----------------------------------------------------------- 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
