Add Undo/Redo History

[regulus79]

This PR adds a sidebar menu showing a list of all the recorded actions the user has done which are stored in the undo/redo checkpoint list. If the user wants to undo/redo back a long distance to a particular action, they can do that by double-clicking on the entry in the undo history, or by rightclicking and selecting "undo"/"redo".

The main purpose of this PR is less about user experience and more for debugging purposes. If we are able to see all of the recorded actions, it may help us track down issues in the undo system which may be hard to catch otherwise.

Also

This PR fixes a serious issue with the undo system, where journalling objects would not be recreated with the correct ID when redoing actions. This was due to an issue in JournallingObject::restoreState where the code was checking for the nodename "journal" when it should have been checking for "journallingObject".
Steps to reproduce on master:

1. Add clip
2. Add notes in the clip
3. Undo until the clip disappears
4. Redo back
5. The clip does not get its notes back!

However, this should be fixed in this PR.

Demo

2025-04-02.22-16-50.mp4

Notable Changes

• Added a parameter to addJournalCheckPoint to give a user-readable name for the action.
  • Added reasons/descriptions to a few actions, namely moving/resizing a note, setting the value of a model, and adding clips to tracks. Currently, all other actions are labeled as "Unknown".
• Added a new class, UndoRedoMenu, to handle the population of the undo/redo menus with the actions.
  • Most of the code for this class was copied from RecentProjectsMenu.
• Made CheckPoint public in ProjectJournal, along with adding getter functions for the undo/redo histories.
• Make ProjectJournal inherit QObject so that it can have signals.
• Fixed a bug where JournallingObjects were not updating their id when they were recreated.

[bratpeki]

I like it, but could there be some more info?

• Add note C#
• Set ... volume to -2dB
• Set ... value to 0.23

Also, what's "Unknown"? 🤣

[regulus79]

I like it, but could there be some more info?

• Add note C#
• Set ... volume to -2dB
• Set ... value to 0.23

Also, what's "Unknown"? 🤣

Thanks for the feedback! I've added some more info when setting the value of a model. However, making proper labels for adding/resizing notes would be more involved. This PR is more about laying the groundwork for a more transparent undo system rather than adding labels for all possible actions. There are like 52 calls to addJournalCheckPoint in the program, so I was thinking of tackling them in one or more future PRs in order not to clutter this one.

Because of this, any action which doesn't have a label is just named "Unknown" for now.

[AW1534]

I got a segv

*** WEAK-JACK: initializing
*** WEAK-JACK: OK. (0)
Lv2 plugin SUMMARY: 0 of 0  loaded in 0 msecs.
Connection established.

Stream successfully created

QtXmlWrapper::loadXMLfile(): empty data
Journalling object id 9299706 for "Set [Saint6] Spinz 808>Interpolation mode to 1" is not valid! This may point to an unresolved issue somewhere in the undo system.

Process finished with exit code 139 (interrupted by signal 11:SIGSEGV)

video.mp4

[regulus79]

I got a segv

Thank you for discovering this bug! From what I can tell, this appears to be due to the calls to create the ClipView being triggered by a signal emitted after the Clip is created. Because all of the undo-ing is performed at once, the signals can get lost and end up getting triggered after multiple checkpoints have already been undone.

I found that adding QApplication::processEvents() after each call to undo/redo fixed the issue. This allows the gui signals to finish processing before moving on to the next undo checkpoint, so that nothing gets out of order. This may not be the best solution, and I suppose ideally undo/redo would be implemented solely in the gui which would maybe prevent this. But, I think we're planning on reworking the entire undo system soon anyways, so for now it's probably fine.

[AW1534]

Works great 👍 (This is not a code review)

[sakertooth]

Concerns:

1. Why is the user allowed to undo any action on the undo stack? Shouldn't they only be allowed to undo actions from the top? If not, then it isn't much of a stack, and I think this can cause problems because it may leave the project in an invalid state.
2. When viewing changes, showing the diff would be a lot better, but that's probably a lot harder too. In any case, what if something changes internally and we no longer store the current state of a journal object for every single action made across history? What if we decided to just change the state on the fly by storing only the undo/redo action code in function objects and the latest state?
3. When viewing changes, what happens when you change the zoom? (Which shouldn't even be stored as an undo action..., zooming in/out is workflow-based. There isn't much benefit clogging up the undo stack especially when you are constantly zooming in and out. Time and time again changes on the undo stack get lost because of this or the user has to undo an absurd number of times to undo the actual change they made to the project, which at that point the DAW would've already crashed most likely 😐). Do we just not show anything, show exactly the changed attribute, or show everything in SongEditor's state?
4. Are you sure we aren't exposing too much unnecessary information to the user (even if the feature is focused on developers)?
5. How exactly does this help with debugging? It would be nice if examples could be provided that explain how this feature helps with debugging. What kind of issues are you trying to track down?

[regulus79]

Why is the user allowed to undo any action on the undo stack?

They aren't sorry, when you rightclick and undo an action (or double click) it undos all the actions up to that point, so the project history should never get out of sync with itself. Maybe the action just being "Undo" isn't the best?

When viewing changes, showing the diff would be a lot better, but that's probably a lot harder too.

That might be possible(?) but it would probably require storing both the new state and the old state of the object and then somehow constructing a diff of the xml. Currently when addJournalCheckPoint is called, it just pushes the current/old state of the object, not the new state.

In any case, what if something changes internally and we no longer store the current state of a journal object for every single action made across history? What if we decided to just change the state on the fly by storing only the undo/redo action code in function objects and the latest state?

Yes, I was also kind of thinking about this. The way it is currently set up is a little bit centered around the current undo system, but it would be very easy for me to remove the "View Changes" function if the new undo system doesn't use it. Or maybe if the new undo system has some support for getting exactly what values were changed, then maybe it could be changed to work with that. But yeah, I am envisioning that this setup will change a little bit once a new undo system is implemented.

When viewing changes, what happens when you change the zoom? ... Do we just not show anything, show exactly the changed attribute, or show everything in SongEditor's state?

It should just show the previous state of the zoom model, with its old value and everything stored in xml. Since the model itself if a journalling object, its save state should be pretty well contained.

Are you sure we aren't exposing too much unnecessary information to the user (even if the feature is focused on developers)?

I mean... yeah I see what you mean, the average user has absolutely no need to view the changes of an undo checkpoint lol. But, it is hidden behind a context menu, so maybe it's fine? Either way, I think the average user would probably still find it useful to see a list of all their previous actions, and be able to precisely undo to a certain point. Also, if something breaks, or some kind of odd bug occurs, a dev could ask to see their undo history, or the xml state of the last checkpoint, which could help with debugging.

How exactly does this help with debugging? It would be nice if examples could be provided that explain how this feature helps with debugging. What kind of issues are you trying to track down?

OH YEAH!!! One example (also in the pr description^^) was the fact that when you create a clip, add some notes, then undo everything back to the start, then redo to the end, the clip will reappear, but the actions for adding the notes will be lost. This bug would have been almost impossible to figure why it was happening (at least for me). But, because I was able to view the state of each undo checkpoint, by carefully comparing them, I finally figured out the issue. The clip was not retaining its previous journalling id when it was recreated. Then, I was able to track down where in the code was causing that issue, and it turned out to be a line which someone forgot to update from "journal" to "journallingObject". But now it's fixed, so users can have a little more peace of mind when undoing/redoing!

[sakertooth]

OH YEAH!!! One example (also in the pr description^^) was the fact that when you create a clip, add some notes, then undo everything back to the start, then redo to the end, the clip will reappear, but the actions for adding the notes will be lost. This bug would have been almost impossible to figure why it was happening (at least for me). But, because I was able to view the state of each undo checkpoint, by carefully comparing them, I finally figured out the issue. The clip was not retaining its previous journalling id when it was recreated. Then, I was able to track down where in the code was causing that issue, and it turned out to be a line which someone forgot to update from "journal" to "journallingObject". But now it's fixed, so users can have a little more peace of mind when undoing/redoing!

If magic string literals weren't used, the bug would've never existed. If we don't begin to properly fix the root causes of these bugs, they will keep happening over and over again, just in different ways at most. Its hard to classify a bug as fixed if it can made into a regression in the near future by following the same bad coding practices that continue to plague the codebase.

Careful use of a debugger would've helped fix this bug in the same way. That's what a debugger is for. That being said, I unfortunately cannot see how this feature would be useful for bug hunting. It also exposes too much unnecessary detail to the user. Its great that it helped you find the bug, but it does not justify the feature being useful for everyone, users and developers alike. Others are free to disagree. This is just my perspective.

[sakertooth]

While I do not like the feature necessarily, since you fixed a bug I should give actionable feedback. I would try using constexpr auto and storing "journalingObject" in some kind of constant string expression, something like constexpr auto joAttribute = "journalingObject". This would pretty much prevent the bug from happening again. All that's left would be using that attribute where necessary.

[regulus79]

I can if you want, although to be fair that string is only used twice, both times in JournallingObject.cpp

[sakertooth]

I can if you want, although to be fair that string is only used twice, both times in JournallingObject.cpp

This disregards the fact that the code can change at any point. Looking at code as a fixed entity at a fixed point in time is not a good idea.

It's also just good practice. You'd be surprised at how small changes like this can add up and help. You generally do not want to use magic string literals.. case in point, bugs like this happen.

[regulus79]

Alright, I have added a constexpr string for the journallingObject node name in the most recent commit.

[szeli1]

Wrong copyright?

[regulus79]

I think I left it like that since I basically copied the code from another file and just changed some things, so I wasn't sure if I should put me as the author. Maybe I should also add myself?

[szeli1]

You could add yourself, to me it seems like you contributed to the file not small amounts.

[szeli1]

I don't know what others think is the plan, but #7895 changes a lot of things, so If the plan is to replace the journal system with #7895, then this PR will have to be redone by someone. I think this feature fits in #7895 more coherently than in JO and ProjectJournal. In #7895 every data is provided (like action / command name, changed data) to construct a message.

If this PR is merged before #7895, then it will add to somebody's work load to replace it after #7895. If this PR is merged after #7895, then it will have to be refactored by @regulus79 which is also not ideal.

What should be done?

[szeli1]

Careful use of a debugger would've helped fix this bug in the same way. That's what a debugger is for. That being said, I unfortunately cannot see how this feature would be useful for bug hunting. It also exposes too much unnecessary detail to the user. Its great that it helped you find the bug, but it does not justify the feature being useful for everyone, users and developers alike. Others are free to disagree. This is just my perspective.

This can be useful if a more advanced undo system was made, where there could be multiple branches of changes just like on git. If a user undoes multiple times to see a past version, then accidentally change something, then they can't go back to the ideal version, because it is deleted. Usually they need to repeat the same actions to go back to the ideal version, but this is time consuming and if somebody suffers from short term memory loss, this process can get difficult.

[regulus79]

I don't know what others think is the plan, but #7895 changes a lot of things, so If the plan is to replace the journal system with #7895, then this PR will have to be redone by someone. I think this feature fits in #7895 more coherently than in JO and ProjectJournal. In #7895 every data is provided (like action / command name, changed data) to construct a message.

I totally agree! I mostly made this PR assuming that the new undo system would be pretty far in the future, but if it looks like that's going to be merged sometime soon, I'm perfectly fine with waiting and reworking this PR after it's done! Honestly, since a big part of this PR was supposed to be helping finding bugs with the undo system, if we are switching to a better undo system which doesn't have as many bugs, this PR might not be as needed anymore.

[szeli1]

I totally agree! I mostly made this PR assuming that the new undo system would be pretty far in the future

It is pretty far in the future, #7895 lays down the groundwork, but doesn't implement undo, also every widget needs to adopt #7895 before the undo is replaced.

[regulus79]

Thanks for reviewing!

[sakertooth]

Honestly, since a big part of this PR was supposed to be helping finding bugs with the undo system, if we are switching to a better undo system which doesn't have as many bugs, this PR might not be as needed anymore.

IMO, more time should be spent figuring out how exactly we should go about improving the undo system. @szeli1 is working on it but we never really saw eye to eye on their work.

[sakertooth]

Also, I still think the PR and what it's meant to do is not working in favor for our users and arguably for our developers. Developers should spend more time simply working on fixing the undo system, solving the fundamental, root problem that needed to be addressed in the first place.

We don't need to take huge leaps and 5 mouthfuls of tasks at once to fix it. We can gradually improve it. That's always an option..

[messmerd]

If we wanted this feature to only be available to developers for debugging purposes (at least for now) while we work on fixing the undo/redo system, one option is to conditionally enable it when compiled in debug mode

[szeli1]

If we wanted this feature to only be available to developers for debugging purposes (at least for now) while we work on fixing the undo/redo system, one option is to conditionally enable it when compiled in debug mode

So the unfortunate situation is that this PR's direction goes directly against #7895 and my and @sakertooth direction. As far as I understand we want to remove JO and ProjectJorunal completely and replace it with a better command based system. This means almost everything in this PR will have to be replaced. The command structure in #7895 integrates better with this PR's features: displaying a command name and a changed value.

we never really saw eye to eye on their work.

I'm interested to find a solution that works for everyone. Also who is "we"?, as far as I know, only you expressed your opinions about fixing the undo system..

[sakertooth]

Also just realized that Qt did the work in this PR, see QUndoView.

The solution seems clear if we want to do this. We need to start embracing what Qt provides for us and move the system to the GUI.