Fixes for undo in text editor grouping unrelated actions

[TheSofox]

Fixes #77101

Code/Text editor had issues with undoing. You could insert text on multiple different lines and hitting undo would undo all of the operations. I added "_push_current_op();" into several places in the code that splits up the undo operations. Now when you left click to change the caret position or use Arrow Keys/Home/End to change the caret position, you will close off the current undo operation and prepare a new one for the next action. You add (or remove) text on three different lines quickly, it'll now be three undo operations to undo instead of one.

[KoBeWi]

I gave it a brief test and didn't encounter the problems from the linked issue anymore. It works great.

[Paulb23]

This looks like a deeper bug somewhere with the undo / redo system as TextEdit::_insert_text should check for and handle this already if (current_op.to_line != p_line || current_op.to_column != p_char) {

[TheSofox]

You're right, there is a deeper bug. For some reason, that conditional statement ( line 7699 ) isn't even being reached no matter what you do in the Text Editor.

[TheSofox]

This looks like a deeper bug somewhere with the undo / redo system as TextEdit::_insert_text should check for and handle this already if (current_op.to_line != p_line || current_op.to_column != p_char) {

Okay, first of all while there maybe deeper bugs, I've looked into things and determined the code segment you point out doesn't handle multi-carat input and probably never did. It's based on comparing the position of the last text insert with the current text insert. This is fine with a single carat: having a different position means the carat was moved. However, with multi-carat input, differing positions could mean that the previous text was inserted by a different carat, not that the position of any carat was manually moved.

My pull request as it stands can handle multi-carat input, as it's based on direct user input.

[kitbdev]

The if statement at 7699 in _insert_text() and the merge current op code can be hit if calling insert_line_at() twice with adjacent lines. In this case it merges the operations together, which is unwanted.
It looks like this is from before the Action system and used to fulfill a similar purpose, but now it should probably be removed.

Also the deeper problem seems to be because of how the action system works, there is always a complex op active and waiting to be ended. Because of this self-contained operations do not hit the code in end_complex_operation that they should, so multiple complex operations are chained together until the next time start_action(NONE) is called, usually in _push_current_op.

[TheSofox]

I agree so much. I seriously considered proposing to refactor the UndoRedo system. There were definitely parts I felt I could clean up (submitting undo actions), but others would have broken compatibility since they are bound functions (like removing the Action System as you suggest). There's the whole question of how to merge Undo operations, and the fact that currently every single character typed is a separate operation (leading to bugs like this: #80624 ).

There's really a lot to discuss and work out when it comes to the TextEdit undo system, however right now, the question is whether to accept this PR.

I propose yes. Given it's the most straighforward way to solve this problem as it exists now. It works, nobody's raised any issues with it, and any other fix involves a massive reworking of existing system that, at the very least, is unlikely to get backported to previous versions. I would like to be part of such a reworking, but in the meantime, this fix should go through to tide people over and for previous versions. This is key usability after all, nobody likes having their focus on coding broken by their editor not acting as they expect.

[Paulb23]

Appreciate the added details!

So this is indirectly calling end_complex_operation forcing the current action to end. Separating the undo step. Otherwise the input is counted as part of that action. Whereas quite a lot of other standalone actions i.e cut is wrapped by [start|end]_complex_ops calls, which would also indirectly end the action.

The end result here I guess, is needing to push up all decisions of when to separate the undo steps into to user handling code, rather then the lower level. As to support that would require a overhaul in how we track changes.

I agree, though with this approach I would like to see if the old code can be removed so it's clearer what's happening. And needs a pass of user actions to cleanly split the steps.

[MajorMcDoom]

Was about to start working on a similar PR, but found this! This fix (even if short-term) is so appreciated, because this issue is a constant source of frustration for me. Hope this gets merged soon! 👍

[TheSofox]

I agree, though with this approach I would like to see if the old code can be removed so it's clearer what's happening. And needs a pass of user actions to cleanly split the steps.

By "old code" I assume you're referring to everything after the if (current_op.type != op.type) statement in _insert_text and _remove_text functions? I believe that function always evaluates to true so having anything after it is unnecessary.

I'm a little unsure of what you mean by "a pass of user actions to cleanly split the steps". Are you suggesting I add new functions to text_edit.cpp?

[TheSofox]

I removed dead code to make it clearer what's happening, and rebased to the latest version.

[akien-mga]

CC @kitbdev who also reworked this in #86978.

This PR might be a good first step and one that can be cherry-picked to 4.2.x.

[kitbdev]

#86978 doesn't actually rework this or fix the issue, I just noticed the issue when working on unit tests.
Ideally the action system will be reworked and _push_current_op() won't need to be called at the start of everything. However, I think this is fine as a workaround until then, and this can be cherrypicked.
Also, removing the old code fixed the multiple insert_line_at undo merging issue I noticed.

[Paulb23]

I'm a little unsure of what you mean by "a pass of user actions to cleanly split the steps". Are you suggesting I add new functions to text_edit.cpp?

I meant a manual pass of actions to make sure they're handled correctly. But this looks like a good enough fix for now.

[akien-mga]

Thanks!