Skip to content

Inefficient implementation of undo/redo #699

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

Merged
merged 1 commit into from
Jan 26, 2022
Merged

Inefficient implementation of undo/redo #699

merged 1 commit into from
Jan 26, 2022

Conversation

mbfraga
Copy link
Collaborator

@mbfraga mbfraga commented Jan 24, 2022

Summary / How this PR fixes the problem?

Implement basic undo/redo. code is somewhat inefficient...but honestly fairly ok considering other limitations currently in the software. I don't guarantee this is bug-free ;)

Steps to Test

Undo with ctrl-z, redo with ctrl-shift-z

Known Issues / Things To Do

Todo:

  • probably should limit number of undos/redos
  • There is probably a way to reduce special code to make sure
    things are copied and not just reference-shared...vala...

Todo:
* probably should limit number of undos/redos
* There is probably a way to reduce special code to make sure
things are copied and not just reference-shared...vala...
@Alecaddd Alecaddd self-requested a review January 24, 2022 17:24
Copy link
Member

@Alecaddd Alecaddd left a comment

Choose a reason for hiding this comment

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

Great initial implementation!
The only issue is that the app crashes if I create a new item after an undo action. This is due to the layers item not updating properly at every history interaction and freaking out when trying to update the list.
I'll deal with it in a follow up PR, I'm okay with merging this.

Comment on lines -64 to +59
construct {
public Model () {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if there's a particular reason why you converted this from a construct creation to a class creation. The construct always runs before any class with argument is called, so there's not need to call this().
Did you stumble upon any issue with it?

@Alecaddd Alecaddd merged commit 13401b9 into main Jan 26, 2022
@Alecaddd Alecaddd deleted the history branch January 26, 2022 03:10
@Alecaddd Alecaddd mentioned this pull request Jan 26, 2022
46 tasks
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.

2 participants