Add undo & redo functionality #1225

bytasv · 2022-10-28T08:15:47Z

Partial of Undo/Redo action #226

Screen.Recording.2022-10-28.at.11.08.04.mov

FYI: if we agree on this approach, I'd like to proceed with integration tests in a separate PR as I suspect it might take a while to implement

render · 2022-10-28T08:15:53Z

Your Render PR Server URL is https://toolpad-pr-1225.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-cddovdla4995ha90evtg.

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentEditor.tsx

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/EditorCanvasHost.tsx

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/index.tsx

packages/toolpad-app/src/toolpad/DomLoader.tsx

packages/toolpad-app/src/utils/useShortcut.ts

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/RenderPanel/RenderOverlay.tsx

packages/toolpad-app/src/toolpad/DomLoader.tsx

render · 2022-10-31T12:33:01Z

Your Render PR Server URL is https://toolpad-pr-1225.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-cdfs0ukgqg438ugltuo0.

bytasv · 2022-10-31T13:01:40Z

@Janpot I've added undo/redo buttons as request. Are there any other changes that I should do before we can wrap up this work?

apedroferreira

Looks good! Just a couple of comments about getMaybeNode

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentEditor.tsx

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/RenderPanel/RenderOverlay.tsx

…tate

bytasv · 2022-11-02T16:43:09Z

Hey @gerdadesign I guess that my first 2 comments here were design related? #1225 (comment) Not sure if we should do something about them but let us know your thoughts!

Since this is done according to the latest design 👇 I'd suggest discussion whether grouping undo/redo actions with preview/deploy is the right way or not to take place separate from this PR (in other words not be a blocking factor)

packages/toolpad-app/src/toolpad/DomLoader.tsx

Janpot · 2022-11-03T09:34:18Z

packages/toolpad-app/src/toolpad/DomLoader.tsx

@@ -276,6 +343,9 @@ export interface DomLoader {
  saving: boolean;
  unsavedChanges: number;
  saveError: string | null;
+  pendingHistoryDom: appDom.AppDom;
+  undo: appDom.AppDom[];


Some nits:

Perhaps it makes sense to already anticipate that we will take selection in the undo stack (and maybe even other things) and foresee a type that can be extended with non-dom related things?

interface UndoStackEntry { dom: appDom.AppDom, // future: // selection: NodeId } undo: UndoStackEntry[];

very much a nit, but in terms of naming, undo and redo sound like they could be methods, I'd consider naming these undoStack and redoStack to better convey what they are.

Perhaps it makes sense to already anticipate that we will take selection in the undo stack (and maybe even other things) and foresee a type that can be extended with that?

Hmm, I'd then rather wait for an actual implementation. Initially I was thinking that we should do selection state part of the dom itself. With the approach you are proposing I assume selection and action would happen at the same time, where in other apps selection itself is considered a separate undoable action. If however selection was part of the dom, then we wouldn't need to do any extra handling for the undo/redo

very much a nit, but in terms of naming undo and redo sound like methods, I'd consider naming these undoStack and redoStack to better convey what they are

Makes sense 👍 just renamed them

Initially I was thinking that we should do selection state part of the dom itself.

The dom is what we persist on disk and what we regard as the shareable artifact containing a toolpad application. The selection is local state, it shouldn't be part of that.

With the approach you are proposing I assume selection and action would happen at the same time

Selection would add an entry in the undo stack with the same dom but a different selection.

Fine, regardless I'd leave this out of scope for this PR then as it's already quite big

Janpot · 2022-11-03T10:01:30Z

packages/toolpad-app/src/toolpad/DomLoader.tsx

@@ -147,6 +159,55 @@ export function domLoaderReducer(state: DomLoader, action: DomAction): DomLoader
  }

  switch (action.type) {
+    case 'DOM_UPDATE_HISTORY': {
+      const updatedUndoStack = [...state.undoStack, state.pendingHistoryDom];


Not sure I understand why we need pendingHistoryDom, can't we just use state.dom here?

That was my first idea, I did try using state.dom here, but by the time this is called state.dom has updated values, where what I want is values that were in the dom before the actions were called. Storing a reference to last state that hasn't been pushed to history stack resolved this issue.
Just to be 100% sure I tried passing state.dom here after all these changes with throttle/debounce, but it still doesn't work.
Basically if we store state.dom here, we loose 1 point of history, and by using reference to "pending" state it always works as expected

oh ok I see, it just means that the top of the stack contains the current state, and that doing an undo means first popping an item, and then resetting the dom to what's in the top of the stack instead of what we just popped off.

Hmm, I'm not sure I fully understand whether you meant that this is how it's working now or how it would work if we pushed state.dom instead of pendingHistoryDom but the idea is to keep undo stack populated only with previous states 🤔
Do you see a way that we could avoid using pendingHistoryDom ref? I was trying various things, like passing state to dispatch, etc... but none of them worked predictably and this (current) approach seemed to do the trick

Do you see a way that we could avoid using pendingHistoryDom ref?

I think it can be done if we slightly alter this philosophy: "the idea is to keep undo stack populated only with previous states" to "the idea is to keep undo stack populated with full history up until the current state". And initialize the undoStack with [dom] instead of []. And then when popping an item off the stack, not use that popped off item to restore the dom, but look at the top item of the trimmed stack.

Basically it would work the same way, only instead of adding an extra property for pendingHistoryDom, the top item in the stack would be pendingHistoryDom. I think there are some small benefits to this method:

One less property to keep in sync pendingHistoryDom

We will want to give an overview of the history of changes at some point. To do that we will have to add a human readable label to each entry in the stack. In the implementation with pendingHistoryDom there would be an entry missing for the last change, and we'd need to special case the last change. In the implementation without pendingHistoryDom, the undoStack would reflect the whole history up until the last change, and we can just flat render this array.

Then again, I may be wrong in my thinking, it's only based on reading the implementation and thinking about the consequences, I didn't try it out and I may be overlooking some other effects.

@Janpot you are correct, thanks, another great tip 💪 I changed the implementation according to your suggestion. I guess the main con of this new approach is that undo/redo methods work slightly differently, but that should be fine 🤔

In the implementation with pendingHistoryDom there would be an entry missing for the last change, and we'd need to special case the last change

I don't think that's true - in the case of pendingHistoryDom initially we would have simply empty array, which means there is no history to undo. At the time we would trigger action and push pendingHistoryDom we could add a label. IMO specifically right now we will have to add extra checks if we want to render history and omit top most entry, i.e. ```
if (undoStack.length < 2) {
// means there is no history to undo
}

We just need to remember in the future that top most item in the undo stack represents current state 🤔

Janpot · 2022-11-03T13:07:48Z

This took a lot of shaping, the result is great. We can absolutely build on this.

bytasv · 2022-11-03T13:14:21Z

@Janpot thanks! I also added to the parent issue todo for selection being part of undo/redo

@apedroferreira do you have any feedback that I should address merging this PR or do you think it's good to go?

apedroferreira

Looks good!

oliviertassinari

It's nice to have the first version of this feature 👍.

The app bar buttons work on my end in https://master--toolpad.mui.com/_toolpad/app/cl9pj1woq0001l59l5y3kztln/pages/7k195tr. However, I'm not able to use the keyboard on macOS. Is this expected?

oliviertassinari · 2022-11-03T17:00:49Z

packages/toolpad-app/src/utils/useShortcut.ts

@@ -16,13 +18,15 @@ export default function useShortcut(
    }

    const handleKeydown = (event: KeyboardEvent) => {
-      if (event.code === code && event.metaKey === metaKey) {
+      if (event.code === code && event.metaKey === metaKey && event.shiftKey === shiftKey) {


Does it work on Windows? I would expect that we need to use event.ctrlKey.

bytasv · 2022-11-03T22:06:17Z

It's nice to have the first version of this feature 👍.

The app bar buttons work on my end in https://master--toolpad.mui.com/_toolpad/app/cl9pj1woq0001l59l5y3kztln/pages/7k195tr. However, I'm not able to use the keyboard on macOS. Is this expected?

Could you share exact steps? I've just tried on the link and keyboard works for me 🤔

oliviertassinari · 2022-11-08T19:30:06Z

I've just tried on the link and keyboard works for me

I had a look, here is the root problem: #1274 (comment).

oliviertassinari · 2022-11-10T18:38:33Z

I have found a bug using the feature, I have reported it at #226 (comment)

bytasv added the feature: App Editor label Oct 28, 2022

bytasv requested a review from a team October 28, 2022 08:15

bytasv self-assigned this Oct 28, 2022

bytasv commented Oct 28, 2022

View reviewed changes

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentEditor.tsx Outdated Show resolved Hide resolved

bytasv commented Oct 28, 2022

View reviewed changes

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/EditorCanvasHost.tsx Show resolved Hide resolved

bytasv commented Oct 28, 2022

View reviewed changes

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/index.tsx Show resolved Hide resolved

bytasv commented Oct 28, 2022

View reviewed changes

packages/toolpad-app/src/toolpad/DomLoader.tsx Outdated Show resolved Hide resolved

bytasv commented Oct 28, 2022

View reviewed changes

packages/toolpad-app/src/utils/useShortcut.ts Show resolved Hide resolved

bytasv force-pushed the undo-redo branch 2 times, most recently from 29b4685 to e9d364d Compare October 28, 2022 08:57

Add undo & redo functionality

81a6c2f

bytasv force-pushed the undo-redo branch from e9d364d to 81a6c2f Compare October 28, 2022 09:08

Janpot reviewed Oct 28, 2022

View reviewed changes

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/RenderPanel/RenderOverlay.tsx Outdated Show resolved Hide resolved

Janpot reviewed Oct 28, 2022

View reviewed changes

packages/toolpad-app/src/toolpad/DomLoader.tsx Outdated Show resolved Hide resolved

bytasv mentioned this pull request Oct 28, 2022

Undo/Redo action #226

Closed

10 tasks

bytasv added 3 commits October 28, 2022 14:48

Reduce throttle duration, fix single action input detection

1c27ec5

Use getMaybeNode instead

71e447e

Merge branch 'master' into undo-redo

8fdad53

apedroferreira reviewed Oct 28, 2022

View reviewed changes

packages/toolpad-app/src/toolpad/DomLoader.tsx Outdated Show resolved Hide resolved

apedroferreira reviewed Oct 28, 2022

View reviewed changes

packages/toolpad-app/src/toolpad/DomLoader.tsx Outdated Show resolved Hide resolved

Add undo/redo buttons

3038a74

Merge branch 'master' into undo-redo

393d31c

bytasv requested a review from Janpot October 31, 2022 13:01

bytasv added 2 commits October 31, 2022 16:52

Fix destroy redo stack when action is executed

9e27628

Merge branch 'master' into undo-redo

db55d1d

bytasv requested a review from apedroferreira October 31, 2022 15:21

apedroferreira approved these changes Oct 31, 2022

View reviewed changes

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentEditor.tsx Show resolved Hide resolved

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/RenderPanel/RenderOverlay.tsx Show resolved Hide resolved

Refactor undo/redo to use dispatch and properly store everything in s…

cf2c8e2

…tate

bytasv requested review from Janpot and apedroferreira November 2, 2022 16:40

apedroferreira mentioned this pull request Nov 2, 2022

Undo redo fix proposal #1266

Merged

apedroferreira and others added 2 commits November 3, 2022 11:13

Undo redo fix proposal (#1266)

b15faea

Merge branch 'master' into undo-redo

2719d03

Janpot reviewed Nov 3, 2022

View reviewed changes

packages/toolpad-app/src/toolpad/DomLoader.tsx Show resolved Hide resolved

Janpot reviewed Nov 3, 2022

View reviewed changes

bytasv added 2 commits November 3, 2022 11:46

Remove action guard

982c997

Rename undo/redo to stacks

5c2b2a0

Janpot reviewed Nov 3, 2022

View reviewed changes

bytasv added 3 commits November 3, 2022 13:36

Use throttle for undo

64e50cf

Get rid of pending dom ref

6239be7

Use useEvent and cleanup eslint disable

5556232

bytasv requested a review from Janpot November 3, 2022 12:57

Janpot approved these changes Nov 3, 2022

View reviewed changes

Merge branch 'master' into undo-redo

768baf8

apedroferreira approved these changes Nov 3, 2022

View reviewed changes

bytasv merged commit 0c0b7f8 into master Nov 3, 2022

bytasv deleted the undo-redo branch November 3, 2022 14:31

oliviertassinari added the new feature New feature or request label Nov 3, 2022

oliviertassinari reviewed Nov 3, 2022

View reviewed changes

bytasv mentioned this pull request Nov 4, 2022

Fix windows and AZERTY shortcuts for undo redo #1274

Merged

apedroferreira mentioned this pull request Dec 5, 2022

Undo/redo through different views #1417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add undo & redo functionality #1225

Add undo & redo functionality #1225

bytasv commented Oct 28, 2022 •

edited

Loading

render bot commented Oct 28, 2022

render bot commented Oct 31, 2022

bytasv commented Oct 31, 2022

apedroferreira left a comment

bytasv commented Nov 2, 2022

Janpot Nov 3, 2022 •

edited

Loading

bytasv Nov 3, 2022

Janpot Nov 3, 2022 •

edited

Loading

bytasv Nov 3, 2022

Janpot Nov 3, 2022

bytasv Nov 3, 2022

Janpot Nov 3, 2022

bytasv Nov 3, 2022

Janpot Nov 3, 2022 •

edited

Loading

bytasv Nov 3, 2022

Janpot commented Nov 3, 2022

bytasv commented Nov 3, 2022

apedroferreira left a comment

oliviertassinari left a comment

oliviertassinari Nov 3, 2022

bytasv commented Nov 3, 2022

oliviertassinari commented Nov 8, 2022

oliviertassinari commented Nov 10, 2022 •

edited

Loading

Add undo & redo functionality #1225

Add undo & redo functionality #1225

Conversation

bytasv commented Oct 28, 2022 • edited Loading

render bot commented Oct 28, 2022

render bot commented Oct 31, 2022

bytasv commented Oct 31, 2022

apedroferreira left a comment

Choose a reason for hiding this comment

bytasv commented Nov 2, 2022

Janpot Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot commented Nov 3, 2022

bytasv commented Nov 3, 2022

apedroferreira left a comment

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bytasv commented Nov 3, 2022

oliviertassinari commented Nov 8, 2022

oliviertassinari commented Nov 10, 2022 • edited Loading

bytasv commented Oct 28, 2022 •

edited

Loading

Janpot Nov 3, 2022 •

edited

Loading

Janpot Nov 3, 2022 •

edited

Loading

Janpot Nov 3, 2022 •

edited

Loading

oliviertassinari commented Nov 10, 2022 •

edited

Loading