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

Introduce new Undo/Redo system #1817

Open
wants to merge 61 commits into
base: master
Choose a base branch
from

Conversation

MrStevns
Copy link
Member

@MrStevns MrStevns commented Apr 8, 2024

This PR introduces a new backup system, aka. the undo/redo system that I've tried to introduce a bunch of times over a few years but never completed or completed but was too big to actually review properly 🙃
What's different about this PR is that everything but backups for strokes and polyline has been scrapped.

Won't that leave us with a less feature complete undo/redo system then, you ask? No, because the old system still exist, basically we can switch between the old and the new system, while we continue to develop it.

Some benefits of that is:

  1. Ability to review this PR without having all the bells and whistles.
  2. While the PR is small enough, we can tweak and agree on the foundation, which is the main focus of this PR.

In addition to that, in order to avoid leaking the complexity of having two backup systems, all the backup system logic exist in the same place. As such everything including the old system reside now in backupmanager.cpp/h files. The old backup system will work as is and still call editor, which now just redirect it to backup manager but otherwise it's the same.

For simplicity sake and to avoid all kinds of weird edge cases, once the new undo manager has been activated, you'll need to restart the application, same goes for when you deactivate it.

There's still a bit of work before the PR is ready to be reviewed, mainly getting selection modifications working again and some cleanup.

@MrStevns MrStevns added this to the 0.7.0 milestone Apr 8, 2024
@MrStevns MrStevns marked this pull request as draft April 8, 2024 16:54
@MrStevns MrStevns added the Undo label Apr 9, 2024
@MrStevns MrStevns changed the title Introduce new backup system Introduce new Undo/Redo system Apr 9, 2024
@chchwy chchwy self-requested a review April 11, 2024 09:03
@MrStevns
Copy link
Member Author

I've spend some hours on addressing the latest comments and making the new undo/redo implementation more robust.

  • I've changed the terminology from BackupElement to UndoRedoCommand.
  • The saveState handling has been improved such that old state can't linger and you can't call the new backup method when the prerequisite data is nullptr.
  • I've fixed a memory leak caused by the clone in saveStates
  • I've removed the undo/redo prefix, now it's only there for the old system.

Let me know what you think.

app/src/generalpage.cpp Outdated Show resolved Hide resolved
app/src/generalpage.cpp Outdated Show resolved Hide resolved
app/ui/generalpage.ui Outdated Show resolved Hide resolved
core_lib/src/managers/undoredomanager.cpp Outdated Show resolved Hide resolved
core_lib/src/tool/movetool.cpp Outdated Show resolved Hide resolved
MrStevns and others added 7 commits April 25, 2024 16:04
Co-authored-by: Jakob <j5lx@fmail.co.uk>
Co-authored-by: Jakob <j5lx@fmail.co.uk>
Co-authored-by: Jakob <j5lx@fmail.co.uk>
It's no longer possible to call add without having a valid save state. This makes it easier to see where the state comes from as well as allowing you to keep multiple save states at the same time.
@MrStevns MrStevns requested a review from J5lx April 27, 2024 15:41
}

Layer* currentLayer = editor()->layers()->currentLayer();
switch (type)
Copy link
Member

Choose a reason for hiding this comment

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

What bothers me is that this abstraction creates a disconnect from how users interact with the program. When I use the pencil, I expect that to show up in the undo history as a use of the pencil. When I use the eraser, I expect that to show up as a use of the eraser. When I use the smudge tool, I expect that to show up as a use of the smudge tool. Yet as it is now, they all show up simply as “strokes”, despite serving fundamentally different purposes. What I’m missing is an individual description for each undoable action as seen from a user perspective, like the legacy system.

You said that you want to ensure that the description is related to the UndoRedoType. However, your implementation goes far beyond that, since it forces the description to strictly match the UndoRedoType. At the same time, it’s not entirely clear to me what the UndoRedoType represents. Internally, the program distinguishes between vector, bitmap, and transform commands. Externally, undoable actions include pencil, eraser, select, move, pen, polyline, bucket, brush, smudge. And then UndoRedoType has a subset of that which is stroke, polyline and selection, and even operations like floodfill are considered strokes when determining their description. It just seems somewhat arbitrary. With that said though, my main concern is the highlighted part above.

core_lib/src/managers/undoredomanager.cpp Outdated Show resolved Hide resolved
core_lib/src/tool/movetool.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for Requested Changes
Development

Successfully merging this pull request may close these issues.

None yet

4 participants