Single-update undo/redo

[apedroferreira]

Closes #845
Closes #1192
Preview URL: https://toolpad-pr-1374.onrender.com/

Update the DOM only a single time for each drag & drop operation/resizing/delete, so that undo/redo works without throttling.

[apedroferreira]

@Janpot I've removed some methods from domApi in the last commit, the ones that seemed most likely to be used in updates with more than one change. Let me know if you think I should remove more or less methods.

[bytasv]

If dom changes are done in atomic update why do we still need throttle? Wouldn't the goal of this PR be to get rid of throttle entirely?
You mentioned in other PR that throttle might still be needed when undoing changes in a text input, though our undo/redo implementation should not be even triggered inside text inputs, so I'm wondering if it's related to that?

[Janpot]

The undo would happen natively in the text input, but the toolpad undo stack won't reflect this, it will just add another stack entry on top instead of popping one off. a subsequent redo will behave wrong.

example (this is already current behavior):

1. add a button
2. focus the content prop and add text at the end " foo", it should now read "Button Text foo"
3. Cmd+Z, it should now read "Button Text" again
4. remove focus from the input
5. Cmd+Z, it should now remove the button, but instead it adds back " foo", you'll have to Cmd+Z twice more to get the button removed.

We'll need to fix this and ☝️ can be an integration test

The problem will compound the more text you write and undo/redo in the text input.

As soon as we remove the throttling, every update to the textinput will get its own stack entry. So one entry for every letter typed, and then one entry for every time undo or redo is called within the textinput.

[bytasv]

Thanks for explanation Jan (I missed your reply in another PR), now its clear to me. I agree we need to fix that and cover with test

[apedroferreira]

Can I get a final review here in order to merge, please? All suggestions should have been addressed already.
Changing views is ready in #1417, but I can't get a preview live until I merge this part.

Edit: Ah nevermind this still needs the text input changes or it's a bit broken - feel free to pre-approve this PR anyway so I can do that in a separate PR

[bytasv]

This spec should still be relevant, specifically for text field changes

[apedroferreira]

We can have something more specific once we have specific logic for text inputs?
Anyway I won't merge this until I add a test that does the same that this one did.

[apedroferreira]

This test should work fine after all, I had the impression that it was testing more general changes but I was wrong!

[bytasv]

Apart from from spec comment all seems to be working great 👍

[apedroferreira]

I've merged text input undo/redo into this PR so that it works as a whole and can be merged (#1459).
In that PR I'm just using debouncing for text input changes + disabling the native browser undo/redo in text inputs (in the page view only - native undo/redo won't be blocked in other views).

Also re-added the batching test as looks like it's perfect for testing the text inputs after all!

Preview URL: https://toolpad-pr-1374.onrender.com/

[apedroferreira]

I'm gonna merge this PR later today if it's ok!
Feel free to check these changes (just 4 files) #1459, and the preview at https://toolpad-pr-1374.onrender.com/ (includes atomic updates + text input undo/redo) in case there's any more comments.
I shouldn't have requested for reviews yesterday until having added these extra changes, so sorry for that.

[Janpot]

🙂 I think this is why I suggested to use a function in domApi.update. This callback invalidates on every dom change, and it's also not 100% sure it will get the last dom. e.g. imagine a function:

const addConnection = React.useCallback(() => domApi.update(appDom.addConnection(dom)), [dom]);

// somewhere else, this function called twice
addConnection();
addConnection();

Those would get the same initial version of dom, meaning only one connection is added. While I would expect to have two connections. It would be different if

const addConnection = React.useCallback(
  () => domApi.update((draft) => appDom.addConnection(draft)),
  [],
);

[apedroferreira]

Would it work to restore and use methods such as domApi.setNodeNamespacedProp (which I had deleted) in cases like this?
And use domApi.update as I'm using it but only when a temporary DOM is needed? domApi.update replaces the whole DOM anyway so in those cases maybe it's fine?

[apedroferreira]

Still thinking of better alternatives, your proposal does seem more "atomic", it's just more difficult to use in RenderOverlay.

[apedroferreira]

I'll try supporting domApi.update((draft) => ... and use it as much as possible where I can, except in the few cases where it's hard to use, I think that should be possible.

[Janpot]

Still thinking of better alternatives, your proposal does seem more "atomic", it's just more difficult to use in RenderOverlay.

We can support both methods, like

update: (draft: React.SetStateAction<AppDom>) => void

Also, wouldn't necessarily recommend it, but nothing prevents you from doing:

const addConnection = React.useCallback(
  () => domApi.update(() => appDom.addConnection(dom)),
  [dom],
);

[apedroferreira]

Looks like I was able to use an updater function everywhere after all!
Commit: 1f7779e
After the refactoring I had made in RenderOverlay I guess it wasn't so difficult there.

[apedroferreira]

As this PR is using the updater function now I will merge this part already!