Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix various code editor bugs by robustifying text selection #73471

Closed
wants to merge 1 commit into from

Conversation

MewPurPur
Copy link
Contributor

@MewPurPur MewPurPur commented Feb 17, 2023

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.

@MewPurPur MewPurPur requested review from a team as code owners February 17, 2023 00:34
@KoBeWi KoBeWi added this to the 4.0 milestone Feb 17, 2023
@MewPurPur MewPurPur force-pushed the fix-silly-textedit branch 4 times, most recently from 69929de to 26557ec Compare February 17, 2023 02:58
@MewPurPur MewPurPur force-pushed the fix-silly-textedit branch 3 times, most recently from bd6f03a to 1d38fbf Compare February 17, 2023 18:36
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth splitting out the queue_redraw for #71794 into it's own PR, so we can get it fixed for 4.0.

Would be nice to add some unit tests for the CodeEdit changes to ensure this behaviour is covered.

editor/code_editor.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
@MewPurPur MewPurPur marked this pull request as draft February 19, 2023 19:58
@MewPurPur
Copy link
Contributor Author

MewPurPur commented Feb 19, 2023

Leaving as draft then until we agree on an implementation. Redrawing bug moved to #73597

@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 20, 2023
@MewPurPur
Copy link
Contributor Author

MewPurPur commented Mar 2, 2023

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 for move_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() and toggle_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?

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Mar 7, 2023

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 from and a to
  • 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 from or the to)

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.

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Mar 26, 2023

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.

@MewPurPur MewPurPur closed this Mar 26, 2023
@MewPurPur MewPurPur deleted the fix-silly-textedit branch March 26, 2023 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants