Fix various code editor bugs by robustifying text selection
Fixes #72797 Most likely Closes #71217
Probably not a good implementation, but a fine starting point. It works correctly so I haven't marked it as draft.
These bugs were caused because of the selections' "pivots". Selections have three properties: from, to, and a "pivot". from and to define the borders of the selection, while "pivot" defines which part of the selection to be immovable when you change the selection, with Shift+Up for example.
The problem was that select() can't change the pivot. It sort of assumes that the text can't change without selections being undone, which is usually true, but not for operations that change the line they are ran on (i.e. unindent_lines()). But those operations still use it. Because the pivot isn't changed, as the line shrinks down, said pivot might become out of bounds and throw an error when you click it (as clicking is considered dragging until you release the mouse button). It also resulted in weird behavior when using Shift+Arrows (but this is unreported afaik).
This PR adds a function called select_safe() to TextEdit, which is similar to select() but it also allows to pass new selection "pivots". This new function is now used instead of select() with appropriate arguments for all operations that affect the text content of the code editor. This seemed to be the source of the 3rd bug too, although I couldn't replicate it.
Why a new function? As it stands, the only other place that can modify these pivots is set_selection_mode() which isn't appropriate.
Leaving as draft then until we agree on an implementation. Redrawing bug moved to #73597
Take 2, but the implementation still feels wrong to me. Definitely not final (there's a bug with unindent anyway).
- Implements a
move_selection()method instead. It's formove_lines_up(),move_lines_down(),duplicate(),indent_lines()and it only takes a caret and the new pivot, adjusting the rest of the selection appropriately. - It can't be used for
unindent_lines()andtoggle_comment()as those can modify the selection, rather than just moving it.
Currently on my mind: Maybe select() itself can be modified without needing to pass new pivots to it?
So I chatted a little with Paulb, wasn't very exhaustive but I think we observed the following:
- Selection "pivots" are mishandled all over the place.
- This includes at least one method that doesn't modify the content (unreported bug in
select_next_occurence()) - The only thing in common with all the bugs is a
select()call not accounting for pivots.
And our assumptions for how selections should be, is the following:
- Selections have a
fromand ato - Maybe carets could be connected to their selections, maybe not?
- Either way, a caret should always be on one end of a selection, and the pivot should always be on the other.
Externally, the functions that returned them are documented to "return the place where the selection was initially made". The documentation is correct, but the concept itself is flawed. Not adjusting this position means it might end up outside of the text. So I think it's reasonable to change this function, even though it "works as documented".
Therefore, the fix I'm thinking of now is...
- Selection "pivots" should be removed internally.
- The methods that returned them should instead return the edge of the selection opposite to the caret (either the
fromor theto)
Or perhaps only keep the "caret" and the "origin" of a selection, it's enough information; the from and to given by the exposed methods would just return whichever is earlier.
I've lost interest. This PR went through like 3 overhauls and I prefer to start over at this point. This isn't really salvageable.
Or perhaps only keep the "caret" and the "origin" of a selection, it's enough information; the from and to given by the exposed methods would just return whichever is earlier.
I'm thinking of trying to do this first on a simpler playground such as LineEdit.