256353 2010-11-08 11:33:35 +0000 Selecting text by triple click and scolling up causes only the visible contents to be selected 2012-04-20 05:40:48 +0000 1 1 2 Applications konsole general 2.5.999 Mandriva RPMs Linux RESOLVED FIXED NOR normal --- 1 shlomif konsole-bugs-null adaptee shlomif 4.9.0 0 oldest_to_newest 1041672 0 shlomif 2010-11-08 11:33:35 +0000 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. 1164888 1 adaptee 2011-09-20 12:56:16 +0000 Yes, I can reproduce it in 2.7.999. The important condition is starting selection by triple click in step 2. 1212345 2 67643 shlomif 2012-01-10 08:58:11 +0000 Created attachment 67643 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. 1214714 3 shlomif 2012-01-15 15:04:57 +0000 (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)? 1214791 4 adaptee 2012-01-15 17:54:24 +0000 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. 1215921 5 shlomif 2012-01-18 08:59:42 +0000 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 1219874 6 khindenburg 2012-01-28 16:32:22 +0000 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 67643 2012-01-10 08:58:11 +0000 2012-01-10 08:58:11 +0000 A tentative patch that seems to fix the problem here. konsole-bug-256353-tentative-fix.diff text/plain 3017 shlomif ZGlmZiAtLWdpdCBhL3NyYy9TY3JlZW4uY3BwIGIvc3JjL1NjcmVlbi5jcHAKaW5kZXggM2Q4OTQ0 Yy4uZTQxMzFhYyAxMDA2NDQKLS0tIGEvc3JjL1NjcmVlbi5jcHAKKysrIGIvc3JjL1NjcmVlbi5j cHAKQEAgLTM1LDYgKzM1LDcgQEAKICNpbmNsdWRlIDxRdENvcmUvUVRleHRTdHJlYW0+CiAjaW5j bHVkZSA8UXRDb3JlL1FEYXRlPgogCisjaW5jbHVkZSA8S0RlYnVnPgogLy8gS29uc29sZQogI2lu Y2x1ZGUgImtvbnNvbGVfd2N3aWR0aC5oIgogI2luY2x1ZGUgIlRlcm1pbmFsQ2hhcmFjdGVyRGVj b2Rlci5oIgpAQCAtMTAyNiw2ICsxMDI3LDcgQEAgdm9pZCBTY3JlZW46OmdldFNlbGVjdGlvbkVu ZChpbnQmIGNvbHVtbiAsIGludCYgbGluZSkgY29uc3QKIH0KIHZvaWQgU2NyZWVuOjpzZXRTZWxl Y3Rpb25TdGFydChjb25zdCBpbnQgeCwgY29uc3QgaW50IHksIGNvbnN0IGJvb2wgbW9kZSkKIHsK KyAgICBrRGVidWcoKSA8PCAic2V0U2VsZWN0aW9uU3RhcnQgKCIgPDwgeCA8PCAiLCIgPDwgeSA8 PCAiKSI7CiAgICAgc2VsQmVnaW4gPSBsb2MoeCwgeSk7CiAgICAgLyogRklYTUUsIEhBQ0sgdG8g Y29ycmVjdCBmb3IgeCB0b28gZmFyIHRvIHRoZSByaWdodC4uLiAqLwogICAgIGlmICh4ID09IGNv bHVtbnMpIHNlbEJlZ2luLS07CkBAIC0xMDM3LDYgKzEwMzksNyBAQCB2b2lkIFNjcmVlbjo6c2V0 U2VsZWN0aW9uU3RhcnQoY29uc3QgaW50IHgsIGNvbnN0IGludCB5LCBjb25zdCBib29sIG1vZGUp CiAKIHZvaWQgU2NyZWVuOjpzZXRTZWxlY3Rpb25FbmQoY29uc3QgaW50IHgsIGNvbnN0IGludCB5 KQogeworICAgIGtEZWJ1ZygpIDw8ICJzZXRTZWxlY3Rpb25FbmQgKCIgPDwgeCA8PCAiLCIgPDwg eSA8PCAiKSI7CiAgICAgaWYgKHNlbEJlZ2luID09IC0xKQogICAgICAgICByZXR1cm47CiAKZGlm ZiAtLWdpdCBhL3NyYy9TY3JlZW5XaW5kb3cuY3BwIGIvc3JjL1NjcmVlbldpbmRvdy5jcHAKaW5k ZXggNTI0OTk5My4uNDdjNDcyNyAxMDA2NDQKLS0tIGEvc3JjL1NjcmVlbldpbmRvdy5jcHAKKysr IGIvc3JjL1NjcmVlbldpbmRvdy5jcHAKQEAgLTEyNiwxNyArMTI2LDI1IEBAIHZvaWQgU2NyZWVu V2luZG93OjpnZXRTZWxlY3Rpb25FbmQoaW50JiBjb2x1bW4gLCBpbnQmIGxpbmUpCiAgICAgX3Nj cmVlbi0+Z2V0U2VsZWN0aW9uRW5kKGNvbHVtbiwgbGluZSk7CiAgICAgbGluZSAtPSBjdXJyZW50 TGluZSgpOwogfQorI2RlZmluZSBCVUcyNTYzNTNfRklYCiB2b2lkIFNjcmVlbldpbmRvdzo6c2V0 U2VsZWN0aW9uU3RhcnQoaW50IGNvbHVtbiAsIGludCBsaW5lICwgYm9vbCBjb2x1bW5Nb2RlKQog eworI2lmZGVmIEJVRzI1NjM1M19GSVgKKyAgICBfc2NyZWVuLT5zZXRTZWxlY3Rpb25TdGFydChj b2x1bW4gLCBsaW5lICsgY3VycmVudExpbmUoKSAsIGNvbHVtbk1vZGUpOworI2Vsc2UKICAgICBf c2NyZWVuLT5zZXRTZWxlY3Rpb25TdGFydChjb2x1bW4gLCBxTWluKGxpbmUgKyBjdXJyZW50TGlu ZSgpLCBlbmRXaW5kb3dMaW5lKCkpICAsIGNvbHVtbk1vZGUpOwotCisjZW5kaWYKICAgICBfYnVm ZmVyTmVlZHNVcGRhdGUgPSB0cnVlOwogICAgIGVtaXQgc2VsZWN0aW9uQ2hhbmdlZCgpOwogfQog CiB2b2lkIFNjcmVlbldpbmRvdzo6c2V0U2VsZWN0aW9uRW5kKGludCBjb2x1bW4gLCBpbnQgbGlu ZSkKIHsKKyNpZmRlZiBCVUcyNTYzNTNfRklYCisgICAgX3NjcmVlbi0+c2V0U2VsZWN0aW9uRW5k KGNvbHVtbiAsIGxpbmUgKyBjdXJyZW50TGluZSgpKTsKKyNlbHNlCiAgICAgX3NjcmVlbi0+c2V0 U2VsZWN0aW9uRW5kKGNvbHVtbiAsIHFNaW4obGluZSArIGN1cnJlbnRMaW5lKCksIGVuZFdpbmRv d0xpbmUoKSkpOworI2VuZGlmCiAKICAgICBfYnVmZmVyTmVlZHNVcGRhdGUgPSB0cnVlOwogICAg IGVtaXQgc2VsZWN0aW9uQ2hhbmdlZCgpOwpkaWZmIC0tZ2l0IGEvc3JjL1Rlcm1pbmFsRGlzcGxh eS5jcHAgYi9zcmMvVGVybWluYWxEaXNwbGF5LmNwcAppbmRleCA1MTZmNTQ2Li44YmJiNjNiIDEw MDY0NAotLS0gYS9zcmMvVGVybWluYWxEaXNwbGF5LmNwcAorKysgYi9zcmMvVGVybWluYWxEaXNw bGF5LmNwcApAQCAtMjA3Nyw2ICsyMDc3LDggQEAgdm9pZCBUZXJtaW5hbERpc3BsYXk6OmV4dGVu ZFNlbGVjdGlvbihjb25zdCBRUG9pbnQmIHBvc2l0aW9uKQogICAgICAgICBpZiAoX2NvbHVtblNl bGVjdGlvbk1vZGUgJiYgIV9saW5lU2VsZWN0aW9uTW9kZSAmJiAhX3dvcmRTZWxlY3Rpb25Nb2Rl KSB7CiAgICAgICAgICAgICBfc2NyZWVuV2luZG93LT5zZXRTZWxlY3Rpb25TdGFydChvaGVyZS54 KCkgLCBvaGVyZS55KCkgLCB0cnVlKTsKICAgICAgICAgfSBlbHNlIHsKKyAgICAgICAgICAgIC8v IFRPRE8gOiBEZWJ1ZyB0cmFjZSB0byByZW1vdmUuCisgICAgICAgICAgICBrRGVidWcoKSA8PCAi U3RhcnQgc2V0IHRvICgiIDw8IChvaGVyZS54KCkgLSAxIC0gb2Zmc2V0KSA8PCAiLCIgPDwgb2hl cmUueSgpIDw8ICIpIjsKICAgICAgICAgICAgIF9zY3JlZW5XaW5kb3ctPnNldFNlbGVjdGlvblN0 YXJ0KG9oZXJlLngoKSAtIDEgLSBvZmZzZXQgLCBvaGVyZS55KCkgLCBmYWxzZSk7CiAgICAgICAg IH0KIApAQCAtMjA4OSw2ICsyMDkxLDggQEAgdm9pZCBUZXJtaW5hbERpc3BsYXk6OmV4dGVuZFNl bGVjdGlvbihjb25zdCBRUG9pbnQmIHBvc2l0aW9uKQogICAgIGlmIChfY29sdW1uU2VsZWN0aW9u TW9kZSAmJiAhX2xpbmVTZWxlY3Rpb25Nb2RlICYmICFfd29yZFNlbGVjdGlvbk1vZGUpIHsKICAg ICAgICAgX3NjcmVlbldpbmRvdy0+c2V0U2VsZWN0aW9uRW5kKGhlcmUueCgpICwgaGVyZS55KCkp OwogICAgIH0gZWxzZSB7CisgICAgICAgIC8vIFRPRE8gOiBEZWJ1ZyB0cmFjZSB0byByZW1vdmUu CisgICAgICAgIGtEZWJ1ZygpIDw8ICJFbmQgc2V0IHRvICgiIDw8IChoZXJlLngoKSArIG9mZnNl dCkgPDwgIiwiIDw8IGhlcmUueSgpIDw8ICIpIjsKICAgICAgICAgX3NjcmVlbldpbmRvdy0+c2V0 U2VsZWN0aW9uRW5kKGhlcmUueCgpICsgb2Zmc2V0ICwgaGVyZS55KCkpOwogICAgIH0KIAo=