> 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. > > Jan Gerrit Marker wrote: > 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.
Ok, I see what it's doing now. That makes a lot of sense. I would still rather see this as two slots (no matter which class this code is in), and then a sensible comment (just copy and paste what you just typed above) where you make the connections to explain it. It makes things a /lot/ simpler, and is less prone to break if anyone edits either the sendMessageBox or the search. - David ----------------------------------------------------------- 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
