Bug 403117 - REGRESSION: Konsole text selections are now limited by the size of the window
Summary: REGRESSION: Konsole text selections are now limited by the size of the window
Status: RESOLVED FIXED
Alias: None
Product: konsole
Classification: Applications
Component: copy-paste (show other bugs)
Version: 18.12.1
Platform: Other Linux
: VHI critical
Target Milestone: ---
Assignee: Konsole Developer
URL:
Keywords:
: 403462 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-01-11 22:32 UTC by Nate Graham
Modified: 2019-01-23 23:07 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In: 18.12.2


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Graham 2019-01-11 22:32:37 UTC
STEPS TO REPRODUCE
1. Produce some very long output (e.g. build some software)
2. Start a selection at the bottom of the text in the window by clicking-and-dragging
3. Scroll to the top and shift-click to extend the selection

OBSERVED RESULT
The selection terminates at the bottom of the window, not at the bottom of the text where I started it.


Regression in master. Cannot reproduce in 18.12.x (thankfully).
Comment 1 Krešimir Čohar 2019-01-12 08:08:39 UTC
(In reply to Nate Graham from comment #0)
> STEPS TO REPRODUCE
> 1. Produce some very long output (e.g. build some software)
> 2. Start a selection at the bottom of the text in the window by
> clicking-and-dragging
> 3. Scroll to the top and shift-click to extend the selection
> 
> OBSERVED RESULT
> The selection terminates at the bottom of the window, not at the bottom of
> the text where I started it.
> 
> 
> Regression in master. Cannot reproduce in 18.12.x (thankfully).

Yeah I can't reproduce it using 18.12.0 either.
Comment 2 Facundo Batista 2019-01-21 17:46:31 UTC
*** Bug 403462 has been marked as a duplicate of this bug. ***
Comment 3 Nate Graham 2019-01-21 20:00:36 UTC
This is broken in the stable branch. Works in 18.12.0, broken in 18.12.1.

Issue was introduced with https://cgit.kde.org/konsole.git/commit/?h=Applications/18.12&id=ef3773b8753b7553fc09c8cb020925388b05bc73.

Emergency revert time. :(
Comment 4 Nate Graham 2019-01-21 20:00:57 UTC
Git commit 0236ef3bd3e19c591b166f59608f59cb9c31a105 by Nate Graham.
Committed on 21/01/2019 at 19:59.
Pushed by ngraham into branch 'Applications/18.12'.

Revert "Fix crash in extendSelection"

This reverts commit ef3773b8753b7553fc09c8cb020925388b05bc73.

This change caused a critical regression in the stable branch: https://bugs.kde.org/show_bug.cgi?id=403117
FIXED-IN: 18.12.2

CCMAIL: pas.anddev@gmail.com
CCMAIL: kurt.hindenburg@gmail.com

M  +1    -7    src/TerminalDisplay.cpp

https://commits.kde.org/konsole/0236ef3bd3e19c591b166f59608f59cb9c31a105
Comment 5 Nate Graham 2019-01-21 20:03:24 UTC
Packagers of rolling release distros (CCd) can fix this by applying the patch at https://cgit.kde.org/konsole.git/commit/?id=0236ef3bd3e19c591b166f59608f59cb9c31a105
Comment 6 Martin Sandsmark 2019-01-21 20:12:19 UTC
I'd say a crash is more critical than limiting selections, tbh.
Comment 7 Nate Graham 2019-01-21 20:15:00 UTC
Well, both are bad to be sure. But breaking a feature on a stable branch is always verboten. Also the crash does not seem to be 100% reproducible for everyone or it would have more dupes (I can't reproduce it for example) but the selection issue affects everyone.
Comment 8 Martin Sandsmark 2019-01-21 20:18:25 UTC
the crash is 100% reproducible with asserts turned on, without asserts it depends on the gods (and what random memory you hit).

who acked the revert?
Comment 9 Nate Graham 2019-01-21 20:31:21 UTC
I did, on the basis that the fix caused a highly visible 100% reproducible user-facing regression of the type that erodes trust in the software between minor versions of the stable branch (.0 -> .1). The whole reason why we have stable branches is to prevent that kind of thing.

Let's focus on fixing the crash the right way for 18.12.2.
Comment 10 Martin Sandsmark 2019-01-21 20:45:31 UTC
I think I have a proper fix in the martin/cleanupfindwordendstart branch.

But bypassing the maintainer and rushing out a patch is not the correct way to handle this, especially when we knowingly re-introduce a crash, IMHO. 

Personally the crash bothers me more (I reported that bug, you reported this, so I guess that's obvious :-), but I'm not the maintainer either, so I think it's best to leave it up to Kurt to decide what to do.
Comment 11 Nate Graham 2019-01-21 20:48:41 UTC
Yeah, I'll take my lumps if I made the wrong decision here. :) In the future I'll wait before reverting.

We should also make sure we exhaustively test everything that goes into the stable branch too. Feature regressions in bugfix releases destroy users' faith in software.
Comment 12 Martin Sandsmark 2019-01-21 20:57:07 UTC
I couldn't agree more. And +1 for an autotest for this, the selection code is ... above average in complexity, so it's not very comfortable to work on.
Comment 13 Kurt Hindenburg 2019-01-22 02:41:40 UTC
I agree stable should not caused regressions; however, a crash is far, far worse as it will take out all the tabs.  I can get it to crash at will.  Now for the user, which is more likely to happen and cause more problems?

We have 2 weeks until .2 is tagged - what's the plan?
Comment 14 Nate Graham 2019-01-23 23:07:36 UTC
The user won't experience it 100% reproducibly (or at all) because the user doesn't have asserts turned on.

I believe Martin said he was working on a change that fixes the assert without regressing selection.