Beats: allow undoing the last BPM/beats change #12954

ronso0 · 2024-03-12T13:29:07Z

Allow undoing the last BPM/beats change.

Up to 10 BeatsPointers are stored on each BPM/beats change.
Apparently the beat analyzer set beats twice on initial scan, so the first result is added to the stack 🤷‍♂️
For changes done in quick succession (<800 ms currently) intermediate states are not stored.

add beats_undo_adjustment ControlPushButton
add "Undo last BPM/beats change" to BPM menu

Fixes #12774 (#10138)

TODO

add Undo button to skins to make this more accessible, probably to beatgrid controls section next to the waveforms
(Tango + LateNight only? in Deere controls are already quite dense)
add a test?
remove debug output / transform into trace logs

Nice to have

Do we need 'Redo'?

I'd say no. Might be nice to have for comparison, but beats operations are easy to redo, so 🤷‍♂️
It would definitely complicate things.

ronso0 · 2024-03-12T14:59:08Z

I think this can easily be extended to provide N undo steps, but I'm not sure how heavy Beats objects are.

Yet another coffee and it's done 🎉

1o steps are stored
initial null beats are not stored
analyzer set beats twice, so the first result is added to the stack 🤷‍♂️

ronso0 · 2024-03-17T15:16:43Z

Manually everything works as expected, just the tests fail. Need to wrap my head around setting beats and stuff.

daschuer

Thank you. Works good.

There is a usability issue when you have used the shift actions form the waveform, like "adjuste earliere" ten times.
You can do it fast via the GUI buttons, but undoing to the original state = 10 x undo is hard via the menu.

Is there a way to remove intermediate adjustments form the undo stack? I think Gimp has also such a strategy to not undo a brush stroke pixel by pixel.

src/track/track.cpp

daschuer · 2024-03-19T07:07:40Z

src/track/track.h

@@ -561,6 +566,8 @@ class Track : public QObject {

    // Storage for the track's beats
    mixxx::BeatsPointer m_pBeats;
+    QList<mixxx::BeatsPointer> m_pBeatsUndoStack;


Here we may use QStack with push() and pop()

Good to learn about QStack. So the benefit is to use push() and pop() instead of append() and takeLast()/removeLast(). Dunno if that is worth the extra include, but that's tiny, so I'll use it.

ronso0 · 2024-03-19T10:19:40Z

There is a usability issue when you have used the shift actions form the waveform, like "adjuste earliere" ten times.
You can do it fast via the GUI buttons, but undoing to the original state = 10 x undo is hard via the menu.

Is there a way to remove intermediate adjustments form the undo stack? I think Gimp has also such a strategy to not undo a brush stroke pixel by pixel.

Sure, but I didn't look into it, yet.
So we want to "group" actions that we consider were done without thinking, so how long does thinking take? .5, .8 seconds?
We may simply store the last change timestamp and pop+push if the diff is smaller than X millis, so we essentially only keep the last action of a "batch".

Maybe such a timeout also helps with the initial useless beats setting.

daschuer · 2024-03-19T10:38:15Z

This is on any case a feature, not blocking this PR.

ronso0 · 2024-03-19T14:18:31Z

This is on any case a feature, not blocking this PR.

Was rather easy, let's check if the tests also work as desired.

The last commit adds an Undo button to the beatgrid edit controls in LateNight.
I made that commit a fixup! to block merging.

ronso0 · 2024-03-19T15:47:24Z

Was rather easy, let's check if the tests also work as desired.

Nope. Maybe the timing is different in the tests?

daschuer · 2024-03-23T17:50:42Z

src/track/track.cpp

+    // Don't add null beats to the undo stack. Happens when beats are deserialized,
+    // e.g. when opening the track menu.
+    // Don't add beats to stack which we're about to undo.
+    mixxx::Duration time = mixxx::Time::elapsed();


I stumbled here for a short moment about "elapsed() since what?".
Maybe we should rename it to mixxx::Time::sinceStart() or such.

In addition we may want to use a "PerformanceTimer" object instead of implementing it inline.
Using start() restart() and elapsed()

Did you consider this?

I'm considering this, yes.
Pro argument for the current solution is that we don't create an extra timer object, we simply use the Mixxx time.
A performance timer would be running all the time for each track we touch, even if the user doesn't change the beats (apart from initial load/analyse). A regular one-shot timer (auto-stop) might also work (delta is 800 ms).
Hence I don't really see the necessity to change this.

First of all, I have to a strong demand here. We can keep it as it is.

But let's give some insights:
The PerformanceTimer object not "running" in terms of CPU. It is just a thin wrapper around a single member variable that stores the start time. See:

mixxx/src/util/performancetimer.h

Line 37 in 7111b70

std::chrono::time_point<ClockT> m_startTime;

So basically your current solution wrapped by an API.

Oh, sorry, should have looked closer.
I'll try that!

ronso0 · 2024-04-05T12:24:01Z

Manually this works just fine, but I need help with the test (or merge without it).

* add `beats_undo_adjustment` ControlPushButton * add "Undo last BPM/beats change" to BPM menu, active only if there's something to undo Co-authored-by: Daniel Schürmann <[email protected]>

daschuer · 2024-05-01T18:48:28Z

What is your plan here? Remove form 2.5-beta milestone?

ronso0 · 2024-05-02T12:32:47Z

I need help with the tests, then this is ready.
Or we add the tests later on so we can already get feedback during beta.

ronso0 · 2024-05-03T09:45:31Z

I've discovered some issue with the quick actions. (that might also be the reason why the test fails...)
Don't want to fix this in a hurry (might take some time to do it right anyways), so let's remove it from 2.5-beta.

ronso0 · 2024-05-03T11:30:33Z

I've discovered some issue with the quick actions. (that might also be the reason why the test fails...)

Jup, that was it, the test passes.

Ready to remove the Undo button and the trace output.

daschuer

LGTM, ready for clean up. Thank you.

ronso0 · 2024-05-14T07:59:31Z

The manual update is here mixxxdj/manual#654

daschuer · 2024-05-14T18:40:40Z

Thank you

m0dB · 2024-07-13T16:52:51Z

@ronso0 Did you consider using QUndoStack, and if so, why did you prefer to implement your own stack?

I used QUndoStack for cue add/remove: #13469 . I have only looked briefly at your implementation, but I think it will be trivial to use the QUndoStack instead of the custom stack for the beat changes, but maybe I am missing something.

ronso0 · 2024-07-15T08:12:08Z

No, I was not aware of QUndoStack. I'll comment in #13469 re the details.

github-actions bot added engine ui labels Mar 12, 2024

ronso0 force-pushed the beats-undo branch from 5d208be to f7d9a7d Compare March 12, 2024 13:57

ronso0 added beatgrid bpm labels Mar 12, 2024

ronso0 marked this pull request as ready for review March 12, 2024 15:36

ronso0 mentioned this pull request Mar 12, 2024

Have the "Reset BPM" in the "Adjust BPM" submenu only restore BPM to before any new user change, not fully clear the BPM and beatgrid? #12774

Closed

ronso0 force-pushed the beats-undo branch 3 times, most recently from bd5b8d9 to 495b370 Compare March 16, 2024 23:10

github-actions bot added the code quality label Mar 16, 2024

ronso0 force-pushed the beats-undo branch 3 times, most recently from 350ae70 to 01057fb Compare March 17, 2024 14:56

ronso0 force-pushed the beats-undo branch 2 times, most recently from caf0a86 to 39a07c4 Compare March 19, 2024 02:31

daschuer requested changes Mar 19, 2024

View reviewed changes

ronso0 force-pushed the beats-undo branch from 39a07c4 to fb90d13 Compare March 19, 2024 14:17

github-actions bot added the skins label Mar 19, 2024

ronso0 force-pushed the beats-undo branch 2 times, most recently from d84766f to 2f1e1ca Compare March 19, 2024 15:06

ronso0 force-pushed the beats-undo branch 3 times, most recently from 795b752 to 507aa04 Compare March 22, 2024 12:43

ronso0 force-pushed the beats-undo branch from 507aa04 to 8f93805 Compare March 22, 2024 14:12

daschuer reviewed Mar 23, 2024

View reviewed changes

This comment was marked as off-topic.

Sign in to view

ronso0 added this to the 2.5-beta milestone Apr 5, 2024

Beats: allow undoing the 10 last BPM/beats changes

c911903

* add `beats_undo_adjustment` ControlPushButton * add "Undo last BPM/beats change" to BPM menu, active only if there's something to undo Co-authored-by: Daniel Schürmann <[email protected]>

ronso0 force-pushed the beats-undo branch from 8f93805 to 72589a3 Compare April 10, 2024 14:23

ronso0 added 2 commits May 3, 2024 12:18

Beats undo: don't record actions done in quick succession

2d28bcf

Tests: add test for beats_undo_adjustment

c8dd63c

ronso0 force-pushed the beats-undo branch from 72589a3 to ccb9f87 Compare May 3, 2024 10:22

daschuer approved these changes May 13, 2024

View reviewed changes

ronso0 force-pushed the beats-undo branch from ccb9f87 to c8dd63c Compare May 14, 2024 07:45

ronso0 mentioned this pull request May 14, 2024

controls: add beats_undo_adjustment mixxxdj/manual#654

Merged

daschuer merged commit 854ff93 into mixxxdj:main May 14, 2024
13 checks passed

ronso0 deleted the beats-undo branch May 14, 2024 19:15

This was referenced Jun 2, 2024

Undo BPM Grid Change #12816

Closed

add GUI controls for beats_undo_adjustment #13318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beats: allow undoing the last BPM/beats change #12954

Beats: allow undoing the last BPM/beats change #12954

ronso0 commented Mar 12, 2024 •

edited

Loading

ronso0 commented Mar 12, 2024

ronso0 commented Mar 17, 2024

daschuer left a comment

daschuer Mar 19, 2024

ronso0 Mar 19, 2024

ronso0 commented Mar 19, 2024

daschuer commented Mar 19, 2024

ronso0 commented Mar 19, 2024

ronso0 commented Mar 19, 2024

daschuer Mar 23, 2024

daschuer May 1, 2024

ronso0 May 3, 2024

daschuer May 3, 2024

ronso0 May 3, 2024

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

ronso0 commented Apr 5, 2024

daschuer commented May 1, 2024

ronso0 commented May 2, 2024

ronso0 commented May 3, 2024 •

edited

Loading

ronso0 commented May 3, 2024

daschuer left a comment

ronso0 commented May 14, 2024

daschuer commented May 14, 2024

m0dB commented Jul 13, 2024

ronso0 commented Jul 15, 2024

Beats: allow undoing the last BPM/beats change #12954

Beats: allow undoing the last BPM/beats change #12954

Conversation

ronso0 commented Mar 12, 2024 • edited Loading

TODO

Nice to have

Do we need 'Redo'?

ronso0 commented Mar 12, 2024

ronso0 commented Mar 17, 2024

daschuer left a comment

Choose a reason for hiding this comment

daschuer Mar 19, 2024

Choose a reason for hiding this comment

ronso0 Mar 19, 2024

Choose a reason for hiding this comment

ronso0 commented Mar 19, 2024

daschuer commented Mar 19, 2024

ronso0 commented Mar 19, 2024

ronso0 commented Mar 19, 2024

daschuer Mar 23, 2024

Choose a reason for hiding this comment

daschuer May 1, 2024

Choose a reason for hiding this comment

ronso0 May 3, 2024

Choose a reason for hiding this comment

daschuer May 3, 2024

Choose a reason for hiding this comment

ronso0 May 3, 2024

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

ronso0 commented Apr 5, 2024

daschuer commented May 1, 2024

ronso0 commented May 2, 2024

ronso0 commented May 3, 2024 • edited Loading

ronso0 commented May 3, 2024

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented May 14, 2024

daschuer commented May 14, 2024

m0dB commented Jul 13, 2024

ronso0 commented Jul 15, 2024

ronso0 commented Mar 12, 2024 •

edited

Loading

ronso0 commented May 3, 2024 •

edited

Loading