EverestAPI / CelesteTAS-EverestInterop

Everest interop for DevilSquirrel's CelesteTAS
https://github.com/EuniverseCat/CelesteTAS
MIT License
67 stars 28 forks source link

fix(Studio): fix broken redo #41

Closed jakobhellermann closed 1 year ago

jakobhellermann commented 1 year ago

Fixes a bug where you need to redo twice to actually redo one atomic change.

   1,R,U
# Ctrl Z
   1,R
# Ctrl Shift Z
   1u,R
# Ctrl Shift Z
   1,R,U

For background, when typing U on 1,R there will be three commands pushed to the undo buffer:

executing command InsertCharCommand('u', autoundo=True)
executing command ClearSelectedCommand('   1u,R', autoundo=True)
executing command InsertTextCommand('   1,R,U', autoundo=False)

autoundo means that when a command is undone, if the next one is autoundo then it will also be undone. This lets Ctrl-Z treat these three commands as one single undo operation.

# Ctrl Z pressed
  undoing command InsertTextCommand('   1,R,U', autoundo=False)
  - next command is ClearSelectedCommand('   1u,R', autoundo=True), undoing as well
  undoing command ClearSelectedCommand('   1u,R', autoundo=True)
  - next command is InsertCharCommand('u', autoundo=False), undoing as well
  undoing command InsertCharCommand('u', autoundo=False) # <- note the False here

This works for undoing, but notice the autoundo=False in the InsertCharCommand above, which was previously True.

This happened because ClearSelectedCommand triggers OnTextChanging which triggers Studio.UpdateLines which attempts to change the text with some commands. During an undo, commands are disabled, so nothing happens here, but while trying to run a command Studio.UpdateLines calls BeginAutoUndoCommands and EndAutoUndoCommands, which leads to the topmost command in the history setting autoUndo=false.

This is a problem because now the redo stack is

InsertTextCommand('1,R,U', autoundo=False)
ClearSelectedCommand('1u,R', autoundo=True)
InsertCharCommand('u', autoundo=False)

so when you redo, the autoundo chain is broken immediately at InsertCharCommand, so you need to redo again for the remaining two.

The solution is to do nothing in BeginAutoUndoCommands/EndAutoUndoCommands if commands are currently disabled.


The only reason this is not currently broken for undos is that there the logic does

if (cmd.autoUndo || cmd is InsertCharCommand) {

which I would guess was added to fix this bug, but it would also be needed at the Redo.

By fixing the root cause we can remove the || cmd is InsertCharCommand (as long as I didn't miss anything else it is important for).

jakobhellermann commented 1 year ago

I just noticed this also fixes the bug (?) where typing EndStunPause<Ctrl-Z> will remove the entire string instead of just leaving you with EndStunPaus.

If that is a bug and not a feature. If it's a feature then I can re-add the '|| cmd is InsertCharCommand`.