search

chrrs / scribble

Fabric mod to expertly edit your books with rich formatting options and page utilities
https://modrinth.com/mod/scribble
MIT License
4 stars 3 forks source link

Feature: undo/redo #18

Closed wiskiw closed 1 month ago

wiskiw commented 2 months ago

Undo-redo is here!

This PR implements undo/redo for the book editor. The implementation is based on command + memento patterns. The current history size - 30 operations.

Undo/redo were added to the following actions:

Some unit tests for the new classes are also included.

This PR should’t affect existing functionality. I checked as much as possible on my side, but please take a look on your side as well. Let me know if you spotted any issues.

chrrs commented 2 months ago

Thank you! It seems like it mostly works really well!

I'll do a more complete review later, but I do have some first thoughts!

Some of it looks like over-abstraction. For example, I feel like the commands are unnecessary, since you're already storing the rich text in the momento's, why not just restore that on both undo and redo? I feel like it would make the code a lot simpler. Manually re-applying also leads to subtle bugs, for example, changing your clipboard breaks the paste command, and I don't think that redo-ing a cut should change your clipboard, as it doesn't do it for copy.

I might be wrong, but I don't see the reason for separating the different types of momento's, as they contain the same data. Couldn't one Momento class represent the editor state as a whole in this case?

Don't get me wrong, most of it looks really good, and feel free to disagree with me!

wiskiw commented 2 months ago

Thanks for the comment. Actually it's a good point that commands might be necessary for the current action set. But it might be helpful to avoid storing the whole book if we want to let the user undo page deletion/insertion(working on it rn)

I’ll try to remove commands and use memento where possible and do not lead to high memory usage. And please do not spend much time looking into the implementation rn, because I still might change it significantly.

wiskiw commented 2 months ago

Thanks again for your comment. I've made many changes since the PR opened, and now it can be reviewed again. Here are the main changes:

  1. Move color and modifiers from RichSelectionManager to BookEditScreen. So now we can now have only one memento for undo/redo - BookEditScreenMemento; RichSelectionManagerMemento was removed.
  2. Fix the Paste command issue that you mentioned before. It's not elegant, but it works. I would like to hear your ideas on how it can be improved.
  3. Combine the command managers for each page into one for the whole book. This lets me implement undo/redo book related operations like page insertion/deletion/page changing etc.
  4. Add two non-memento commands - DeletePageCommand and InsertPageCommand. They don't use memento patterns to significantly reduce memory usage, otherwise we would have to save a copy of all book pages on each command execution. I still have questions about how to change the currentPageIndex (or shouldn't we?) when a user undoes or redoes a page delete and page insert operation.

After you commented I also experimented a bit and tried to remove commands and stay with the memento pattern only. It works for some commands, but it also brings some new questions. I left them in the code under FixMe comment in this branch.

Please let me know if you find any issues or have any suggestions to improve this implementation 🐈‍⬛

chrrs commented 1 month ago

Alright, this looks good now!