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

Undo system #150

Merged
merged 5 commits into from
Jul 10, 2021
Merged

Undo system #150

merged 5 commits into from
Jul 10, 2021

Conversation

rmpowell77
Copy link
Contributor

@rmpowell77 rmpowell77 commented Feb 7, 2021

This closely follows the outline by Anthony Gore:
https://vuejsdevelopers.com/2017/11/13/vue-js-vuex-undo-redo/

What we do is we have the undo system implemented as a stack of Store Commit changes. When we do an undo, we go back to an initial state and redo all of the Store commit commands. With CalChart, not every Store commit is a change we want to undo, so we have a filter where we only save certain commits.

Adding bottom row buttons
using hotkeys.
adding File/Edit menu.

Pre-PR checklist

  • Ran npm run serve and:
    • Checked basic functionality
    • Checked that errors are handled
  • Ran npm run lint
  • Ran npm run test:unit
  • Ran npm run test:e2e and ran relevant tests
  • Attached reviewers to PR and pinged on Slack/email

Screenshots/GIFs

[Attach screenshots if making a visible change!]

@vhlee7
Copy link
Contributor

vhlee7 commented Feb 20, 2021

bug: undo with adding/removing sheets

@vhlee7
Copy link
Contributor

vhlee7 commented Mar 20, 2021

meeting notes:

current issues:
dots are not connected to their continuities
-dots do not regenerate flows
when moving dot, state has updated but not the dot image

grapher don't observe state but flows
- move flow outside of the show object

new issue:
watches to auto regenerate the flow

flow watching the state or vice versa

@rmpowell77 rmpowell77 force-pushed the undo_system branch 2 times, most recently from 77cbfa1 to 99aee60 Compare March 23, 2021 06:07
@rmpowell77
Copy link
Contributor Author

Looks like this is still failing if you do the following:

Add a sheet, remove a sheet.

We need to make sure we reset these to 0:

selectedSS
selectedDotIDs
beat
grapherToolDots

@rmpowell77
Copy link
Contributor Author

Let's rethink this and go with a system that makes snapshots of the Show state, and then restores those snapshot.

What we do is we have the undo system implemented as a stack of show snap shots. When we do an undo, we restore the show to a previous state.

With CalChart, not every Store commit is a change we want to undo, so we have a filter where we only save certain commits.

Adding bottom row buttons

using hotkeys.

adding File/Edit menu.

Fixing up linting and tests.
@rmpowell77
Copy link
Contributor Author

Updated to be based on a snapshotting system.

Copy link
Collaborator

@MichaelTamaki MichaelTamaki left a comment

Choose a reason for hiding this comment

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

Thank you for the work on this! Left some suggestions.

src/components/menu-top/MenuTop.vue Show resolved Hide resolved
src/models/Show.ts Show resolved Hide resolved
src/models/UndoRedo.ts Outdated Show resolved Hide resolved
src/models/UndoRedo.ts Outdated Show resolved Hide resolved
src/models/UndoRedo.ts Show resolved Hide resolved
src/models/UndoRedo.ts Outdated Show resolved Hide resolved
src/models/UndoRedo.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@MichaelTamaki MichaelTamaki left a comment

Choose a reason for hiding this comment

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

Thank you for the work on this! Left some suggestions.

Copy link
Collaborator

@adhamrait adhamrait left a comment

Choose a reason for hiding this comment

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

I like the snapshot idea compared to the equal-and-opposite solution the undoable-action-list solution. I found a bug while testing it out and I included a video in the slack channel! Also a few other smaller bugs that I found:

  • Dot type appearance changes are not undoable
  • Undoing will reset the selected beat to Hup/0

I think 10 undos total is a bit on the shorter side but we can address that when we get #183 and #184.

src/models/UndoRedo.ts Show resolved Hide resolved
@rmpowell77 rmpowell77 requested a review from adhamrait July 4, 2021 06:20
@rmpowell77
Copy link
Contributor Author

I fixed up the bug, but I don't think I can merge till the approvals are done.

@rmpowell77 rmpowell77 dismissed adhamrait’s stale review July 10, 2021 20:20

Because I've addressed the bug.

@rmpowell77 rmpowell77 merged commit 517affe into main Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants