Bug 256353

Summary: Selecting text by triple click and scolling up causes only the visible contents to be selected
Product: [Applications] konsole Reporter: Shlomi Fish <shlomif>
Component: generalAssignee: Konsole Developer <konsole-devel>
Status: RESOLVED FIXED    
Severity: normal CC: adaptee, shlomif
Priority: NOR    
Version: 2.5.999   
Target Milestone: ---   
Platform: Mandriva RPMs   
OS: Linux   
Latest Commit: Version Fixed In: 4.9.0
Attachments: A tentative patch that seems to fix the problem here.

Description Shlomi Fish 2010-11-08 11:33:35 UTC
Version:           2.5.999 (using Devel) 
OS:                Linux

When I line-wise-select the text of "ls -lR / | head -1000" (or any other long text) in Konsole (on Mandriva Linux Cooker from kdebase4-4.5.74-0.svn1190490.1mdv2011.0.src.rpm ) and start from the bottom and move upwards, then after the screen scrolls upwards, and I release the mouse button, then the section of the selection that is not visible is no longer selected and is not copied. Selecting a text downwards is working fine.

Reproducible: Always

Steps to Reproduce:
1. Type ls -lR / | head -1000

2. Do a triple click to select the last line in the output and hold the mouse button.

3. Move up with the mouse to the top of the screen to select more. Make sure the last lines are no longer visible.

4. Release the mouse.

5. Scroll downwards using the scrollbar.

Actual Results:  
The bottommost lines are not selected.

Expected Results:  
The bottommost lines should remain selected.
Comment 1 Jekyll Wu 2011-09-20 12:56:16 UTC
Yes, I can reproduce it in 2.7.999. 

The important condition is starting selection by triple click in step 2.
Comment 2 Shlomi Fish 2012-01-10 08:58:11 UTC
Created attachment 67643 [details]
A tentative patch that seems to fix the problem here.

This is a tentative fix to this bug. It is tentative because it contains some code I added in order to debug this. Nevertheless, the fix is only in the src/ScreenWindow.cpp and is toggled by the "#define BUG256353_FIX".

What happens there is that the original authors used qMin(...) to limit the code based on the current window's boundaries. Perhaps we need one qMin() and one qMax(), but, from my understanding, it could be that end < start and so it's hard to know when to use each one. I simply removed the boundings and everything seems to be OK.
Comment 3 Shlomi Fish 2012-01-15 15:04:57 UTC
(In reply to comment #2)
> Created an attachment (id=67643) [details]
> A tentative patch that seems to fix the problem here.
> 
> This is a tentative fix to this bug. It is tentative because it contains some
> code I added in order to debug this. Nevertheless, the fix is only in the
> src/ScreenWindow.cpp and is toggled by the "#define BUG256353_FIX".
> 
> What happens there is that the original authors used qMin(...) to limit the
> code based on the current window's boundaries. Perhaps we need one qMin() and
> one qMax(), but, from my understanding, it could be that end < start and so
> it's hard to know when to use each one. I simply removed the boundings and
> everything seems to be OK.

Hi, since I didn't get any comment, should I put this patch on the KDE reviewboard (after cleaning it up)?
Comment 4 Jekyll Wu 2012-01-15 17:54:24 UTC
Sorry for no response. 

Yes, submitting patch to review board is always the preferred way.

As for the patch itself, I actually made similar analysis after posting comment #1, and I thought the first and real problem was why ScreenWindow::setSelectionStart() was ever called, which should not happen IMHO, to reset the start point of selection during scrolling up.  My little hack was setting 'swapping' to false in force in the case of triple clicking and scrolling, and it seemed to work. Anyway, it was just hack and I didn't make further investigation.

In general, I feel the real problem is not in those setSelectionStart() methods themselves, but in how and when they are called. So changing the internal logic of those methods might be dangerous and cause new regressions.
Comment 5 Shlomi Fish 2012-01-18 08:59:42 UTC
Hi Jekyll,

(In reply to comment #4)
> Sorry for no response. 
> 
> Yes, submitting patch to review board is always the preferred way.
> 
> As for the patch itself, I actually made similar analysis after posting comment
> #1, and I thought the first and real problem was why
> ScreenWindow::setSelectionStart() was ever called, which should not happen
> IMHO, to reset the start point of selection during scrolling up.  

I think it is called because the selection start/end are calculated based on the current scroll position and so it needs to be updated whenver the user scrolls the viewport (don't know why this design decision was made, but that's how it is.)

> My little
> hack was setting 'swapping' to false in force in the case of triple clicking
> and scrolling, and it seemed to work. Anyway, it was just hack and I didn't
> make further investigation.
> 
> In general, I feel the real problem is not in those setSelectionStart() methods
> themselves, but in how and when they are called. So changing the internal logic
> of those methods might be dangerous and cause new regressions.

From my testing (which I admit was not very comprehensive), the scrolling and selection continued to work properly with this patch.

In any case, I'll submit this patch to the review board.

Regards,

-- Shlomi Fish
Comment 6 Kurt Hindenburg 2012-01-28 16:32:22 UTC
Git commit 6d9d49aafb358293326f4edca393c7f2dfc9602a by Kurt Hindenburg.
Committed on 28/01/2012 at 17:26.
Pushed by hindenburg into branch 'master'.

Correct issue of triple clicking and scrolling up.

Fixes selecting text by triple click and scrolling up causes only the
visible contents to be selected.

Thanks to Shlomi Fish (shlomif@iglu.org.il) for research and patch.
REVIEW: 103724
FIXED-IN: 4.9

M  +2    -2    src/ScreenWindow.cpp

http://commits.kde.org/konsole/6d9d49aafb358293326f4edca393c7f2dfc9602a