Review Request: Allow copy of text in the chat by pressing Ctrl+C and reorganize the key handling a bit.

[David Edmundson]

> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20110808/a75c4b09/attachment.html>