Batch undo #6410

allejok96 · 2022-05-21T20:56:10Z

Allow multiple journal checkpoint to be grouped together, so they can all be restored with one click.

if (someKindOfContext)
{
    // this will behave like one single undo checkpoint
    auto batchAction = Engine::projectJournal()->beginBatchAction();
    for (auto thing: allThings) { thing->addJournalCheckPoint(); }
}
// this is a separate checkpoint (batchAction ends when it goes out of scope)
otherThing->addJournalCheckPoint();

Batch undo TODO:

Fixes #6251 fixes #5885 fixes #4898 plus some more.

LmmsBot · 2022-05-21T21:05:57Z

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Linux (AppImage): lmms-1.3.0-alpha.1.235+g23a8ecfd2-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.3.0-alpha.1.235+g23a8ecfd2-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha.1.235+g23a8ecfd2-mingw-win64.exe (build link)
Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

macOS

macOS : lmms-1.3.0-alpha.1.235+g23a8ecfd2-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/22c65dab-f748-41bb-9c65-627f89591114/artifacts/0/lmms-1.3.0-alpha.1.235+g23a8ecfd2-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17781?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/dd818bd2-8a2e-4437-b8de-f3610f1a7cf2/artifacts/0/lmms-1.3.0-alpha.1.235+g23a8ecfd2-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17783?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/e00d9cd1-e8f6-49f3-a7a4-070e74d521e1/artifacts/0/lmms-1.3.0-alpha.1.235+g23a8ecfd2-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17784?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/xh0a4mvot3imnylq/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44153820"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/y69x6dxju5tbk0re/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44153820"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/fdb0e396-0e10-4d27-84fb-8d94eba1fbb9/artifacts/0/lmms-1.3.0-alpha.1.235+g23a8ecfd2-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17785?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "ea6674c81c9e3857fe0ef6b0963fbeed1040a6f4"}

Spekular · 2022-05-23T08:09:26Z

Isn't this exactly what we use setJournalling for?

setJournalling(false)
//batch operation
setJournalling(true)
addJournalCheckpoint()

allejok96 · 2022-05-23T16:20:13Z

Nope. setJournalling is when you don't want to journal the same thing many times. This is when you have to journal many unrelated things (no common parent) and want them all to undo at once. Like moving a selection of clips that span multiple tracks.

allejok96 · 2022-05-26T20:29:40Z

Fixes #6251 #5885 and #4898

PhysSong · 2022-07-02T07:10:45Z

Fixes #6251 #5885 and #4898

You should add that in the PR body with "fixes" before every issues to auto-close issues on merge.

Also, could you handle the merge conflicts? I want to review this PR after resolving conflicts.

allejok96 · 2022-07-02T17:55:49Z

@PhysSong conflicts fixed. I can add the "fixed" tags in the commit message when I squash the commits later, right?

zonkmachine · 2022-07-03T20:19:31Z

I can add the "fixed" tags in the commit message when I squash the commits later, right?

Technically I believe that would auto-close those issues, but it's good to have a reference to them in the PR description.

PhysSong

Here are some suggestions and questions. Haven't reviewed ClipView changes yet.

src/core/ProjectJournal.cpp

JohannesLorenz · 2022-07-08T20:25:53Z

~~For information, I'm still about to review this, should be finished this weekend. Would be cool if you can wait with merging.~~ Already finished.

JohannesLorenz

Code review submitted.

Testing Review:

Solo buttons: Have 3 tracks (like in the default song), "solo" track 1, then "solo" track 3. Press Ctrl+z twice -> works. Press Ctrl+shift+z twice -> fails twice.
Clip colors: Create clip, change color (I used pick random), create second clip (e.g. same pattern), change color. Ctrl+z removes color -> OK. Ctrl+z removes 2nd clip -> OK. Ctrl+z removes 1st clip -> fail.
Delete clips: Create 3 clips, mark all, press del. Press Ctrl+z -> they reappear -> OK. you need to press 3 times Ctrl+z so something new happens -> intended?

src/gui/clips/ClipView.cpp

JohannesLorenz · 2022-07-08T19:35:59Z

src/gui/clips/ClipView.cpp

-					}
-				}
-			}
-			else


Why was the else branch removed (I'm unsure both about ...selectAllClips and clipViews.push_back(this)).

getClickedClips() does the equivalent of this whole if-else statement, with the exception of the mentioned selectAllClips(false).

The only result of removing that line is that when you have some clips selected and you ctrl+drag an unselected clip, the old selection will remain until the clip is dropped.

src/gui/clips/ClipView.cpp

src/core/ProjectJournal.cpp

src/core/Track.cpp

JohannesLorenz · 2022-07-08T21:04:16Z

src/core/ProjectJournal.cpp

+	if (!isJournalling()) { return; }
+	// Begin on a new batch if we are not already batching, or if there is nothing to append to
+	if (append ? m_undoCheckPoints.empty() : m_batchingCount == 0) { m_undoCheckPoints.emplace_back(); }
+	++m_batchingCount;


First, I found the variable name confusing. batchingCount sounds like "This is the number of CheckPoints that I am batching". Maybe subjective, though 😁

Secondly, why is this variable changed if we add/remove checkpoints here, but not e.g. in ProjectJournal::addJournalCheckPoint? Intended?

Thirdly, I guess this variable would (if stuff like "secondly" would be changed) be equivalent to something like m_undoCheckPoints.size() or m_undoCheckPoints.size() + m_redoCheckPoints.size()?

Yes confusing name. Perhaps batchesInProgress would be more descriptive?

The count is not of the number of checkpoints in the current batch, instead it counts the number of times beginBatchCheckPoint have been called (like a recursion level, but it's not recursive). Calling the begin function multiple time has no real effect, but we must count the calls, because for every call to begin you expect a call to endBatchCheckPoint.

Ah! So the only reason for that variable is to make sure that begin... and end... match? Then it would all make sense.

An RAII version, like BashPointGuard would be cool (though, probably, less descriptive...).

I'm not so good at RAII so I don't understand how that would be implemented in this case. I just get a feeling it would make things a lot more complex than they had to be, and you'd still have to use some form of counter or vector so you can detect when the last guard is deleted.

What would be nice if there was something like a context manager:

def doThing(): with journal.batchCheckPoints(): for obj in objects: obj.addJournalCheckPoint()

But the closest I can think of is

void doThing() { BatchHandle handle = journal->getBatchHandle(); // error: variable never used for (Object ob: objects) { obj->addJournalCheckPoint(); } }

(It would not remove the need for the counter, but it would remove the need to call endBatchCheckPoint() which could mess things up if forgotten)

Yes, this was what I had in mind, too. It should not cause unused variable warnings, since std::lock_guard is used the same way.

I'm OK with either begin/end or RAII, as long as it's clear what the counter is used for 😁

- Use RAII to start/end batching - Use emplace_back and std::move - Fix batch undo when clips have been deleted with keyboard - Fix broken undo for objects that have been restored (9 yr old bug) - Fix undo when switching from one solo track to another - Rename BatchCheckPoint to CheckPointBatch

allejok96 · 2022-07-12T22:35:53Z

Much stuff going on in last commit.

@JohannesLorenz fixed solo buttons, delete using keyboard and clip colors.

The last one was a 9 year old bug that breaks undo for deleted/restored objects. Try to write some notes and copy the clip and then undo back to before you wrote notes -> fails. I fixed the bug but that introduces an error message JO-ID ... already in use by ...! when copying clips etc. This is not harmful, so let's fix the messages in another PR.

allejok96 added 2 commits May 21, 2022 22:47

Batch undo

bbf9c97

Batch undo of clip colors

8e8c4dd

allejok96 added 2 commits May 22, 2022 12:16

Ignore duplicate checkpoints in the same batch

ef9f7f0

Rename CheckPointGroup to BatchCheckPoint

5003169

allejok96 added 13 commits May 25, 2022 19:29

Fix assert

da14028

Ignore extra clicks when clip is being dragged

c101aff

Clean up redundant or unused code in mousePressEvent

d4819b6

Remove redundant code for copy/select

4a3b04e

Simplify move selection

893b1dd

Batch undo remove, mute and move clips

9c1281a

Shorten clip paste code

3313e00

Batch undo clip paste

c0f70cf

Try fixing assert again

2be0434

Properly journal automatable models

f755b61

Fix clang warnings

4d27f49

Batch undo solo + update clipviews when toggle mute

ca1c35b

Batch undo insert/remove bar

88d280d

allejok96 marked this pull request as ready for review May 26, 2022 20:28

cyber-bridge mentioned this pull request Jun 3, 2022

Fix Undo in Song Editor Fails After Duplicating Multiple Midi Reigions #5920

Closed

JohannesLorenz self-requested a review June 12, 2022 23:30

Merge master into batchUndo

e254c54

PhysSong reviewed Jul 4, 2022

View reviewed changes

src/core/ProjectJournal.cpp Show resolved Hide resolved

src/core/ProjectJournal.cpp Outdated Show resolved Hide resolved

src/core/ProjectJournal.cpp Outdated Show resolved Hide resolved

src/core/ProjectJournal.cpp Outdated Show resolved Hide resolved

JohannesLorenz requested changes Jul 8, 2022

View reviewed changes

allejok96 added 2 commits July 13, 2022 00:13

Merge branch 'master' of https://github.com/LMMS/lmms into batchUndo

ea6674c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch undo #6410

Batch undo #6410

allejok96 commented May 21, 2022 •

edited

LmmsBot commented May 21, 2022 •

edited

Linux

Windows

macOS

Spekular commented May 23, 2022

allejok96 commented May 23, 2022

allejok96 commented May 26, 2022

PhysSong commented Jul 2, 2022

allejok96 commented Jul 2, 2022

zonkmachine commented Jul 3, 2022

PhysSong left a comment •

edited

JohannesLorenz commented Jul 8, 2022 •

edited

JohannesLorenz left a comment

JohannesLorenz Jul 8, 2022

allejok96 Jul 9, 2022

JohannesLorenz Jul 8, 2022

allejok96 Jul 9, 2022

JohannesLorenz Jul 9, 2022

allejok96 Jul 9, 2022 •

edited

JohannesLorenz Jul 9, 2022

allejok96 commented Jul 12, 2022

Batch undo #6410

Are you sure you want to change the base?

Batch undo #6410

Conversation

allejok96 commented May 21, 2022 • edited

LmmsBot commented May 21, 2022 • edited

Linux

Windows

macOS

Spekular commented May 23, 2022

allejok96 commented May 23, 2022

allejok96 commented May 26, 2022

PhysSong commented Jul 2, 2022

allejok96 commented Jul 2, 2022

zonkmachine commented Jul 3, 2022

PhysSong left a comment • edited

Choose a reason for hiding this comment

JohannesLorenz commented Jul 8, 2022 • edited

JohannesLorenz left a comment

Choose a reason for hiding this comment

JohannesLorenz Jul 8, 2022

Choose a reason for hiding this comment

allejok96 Jul 9, 2022

Choose a reason for hiding this comment

JohannesLorenz Jul 8, 2022

Choose a reason for hiding this comment

allejok96 Jul 9, 2022

Choose a reason for hiding this comment

JohannesLorenz Jul 9, 2022

Choose a reason for hiding this comment

allejok96 Jul 9, 2022 • edited

Choose a reason for hiding this comment

JohannesLorenz Jul 9, 2022

Choose a reason for hiding this comment

allejok96 commented Jul 12, 2022

allejok96 commented May 21, 2022 •

edited

LmmsBot commented May 21, 2022 •

edited

PhysSong left a comment •

edited

JohannesLorenz commented Jul 8, 2022 •

edited

allejok96 Jul 9, 2022 •

edited