From c9119039bb2494620f09bd50fa91dc9f72caf669 Mon Sep 17 00:00:00 2001 From: ronso0 Date: Tue, 12 Mar 2024 14:23:32 +0100 Subject: [PATCH 1/3] Beats: allow undoing the 10 last BPM/beats changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- src/controllers/controlpickermenu.cpp | 4 ++ src/engine/controls/bpmcontrol.cpp | 19 ++++++++++ src/engine/controls/bpmcontrol.h | 2 + src/skin/legacy/tooltips.cpp | 4 ++ src/track/track.cpp | 26 ++++++++++++- src/track/track.h | 8 ++++ src/widget/wtrackmenu.cpp | 53 ++++++++++++++++++++++++++- src/widget/wtrackmenu.h | 3 ++ 8 files changed, 117 insertions(+), 2 deletions(-) diff --git a/src/controllers/controlpickermenu.cpp b/src/controllers/controlpickermenu.cpp index 73fa1b6e605..1a2ce5aa1b0 100644 --- a/src/controllers/controlpickermenu.cpp +++ b/src/controllers/controlpickermenu.cpp @@ -276,6 +276,10 @@ ControlPickerMenu::ControlPickerMenu(QWidget* pParent) tr("Toggle the BPM/beatgrid lock"), tr("Toggle the BPM/beatgrid lock"), pBpmMenu); + addDeckControl("beats_undo_adjustment", + tr("Revert last BPM/Beatgrid Change"), + tr("Revert last BPM/Beatgrid Change of the loaded track."), + pBpmMenu); pBpmMenu->addSeparator(); addDeckAndSamplerControl("quantize", tr("Quantize Mode"), tr("Toggle quantize mode"), pBpmMenu); diff --git a/src/engine/controls/bpmcontrol.cpp b/src/engine/controls/bpmcontrol.cpp index 52bef4ca3ba..bf86094a42e 100644 --- a/src/engine/controls/bpmcontrol.cpp +++ b/src/engine/controls/bpmcontrol.cpp @@ -139,6 +139,13 @@ BpmControl::BpmControl(const QString& group, &BpmControl::slotToggleBpmLock, Qt::DirectConnection); + m_pBeatsUndo = new ControlPushButton(ConfigKey(group, "beats_undo_adjustment")); + connect(m_pBeatsUndo, + &ControlObject::valueChanged, + this, + &BpmControl::slotBeatsUndoAdjustment, + Qt::DirectConnection); + // Measures distance from last beat in percentage: 0.5 = half-beat away. m_pThisBeatDistance = new ControlProxy(group, "beat_distance", this); m_pSyncMode = new ControlProxy(group, "sync_mode", this); @@ -155,6 +162,7 @@ BpmControl::~BpmControl() { delete m_pTranslateBeatsMove; delete m_pAdjustBeatsFaster; delete m_pAdjustBeatsSlower; + delete m_pBeatsUndo; } mixxx::Bpm BpmControl::getBpm() const { @@ -233,6 +241,17 @@ void BpmControl::slotTranslateBeatsMove(double v) { } } +void BpmControl::slotBeatsUndoAdjustment(double v) { + if (v <= 0) { + return; + } + const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack(); + if (!pTrack) { + return; + } + pTrack->undoBeatsChange(); +} + void BpmControl::slotBpmTap(double v) { if (v > 0) { m_tapFilter.tap(); diff --git a/src/engine/controls/bpmcontrol.h b/src/engine/controls/bpmcontrol.h index af174cf9d27..614e78efdad 100644 --- a/src/engine/controls/bpmcontrol.h +++ b/src/engine/controls/bpmcontrol.h @@ -109,6 +109,7 @@ class BpmControl : public EngineControl { void slotBeatsTranslate(double); void slotBeatsTranslateMatchAlignment(double); void slotToggleBpmLock(double); + void slotBeatsUndoAdjustment(double value); private: SyncMode getSyncMode() const { @@ -145,6 +146,7 @@ class BpmControl : public EngineControl { ControlPushButton* m_pTranslateBeatsEarlier; ControlPushButton* m_pTranslateBeatsLater; ControlEncoder* m_pTranslateBeatsMove; + ControlPushButton* m_pBeatsUndo; std::unique_ptr m_pToggleBpmLock; diff --git a/src/skin/legacy/tooltips.cpp b/src/skin/legacy/tooltips.cpp index 1fec732bd35..e68f0151e16 100644 --- a/src/skin/legacy/tooltips.cpp +++ b/src/skin/legacy/tooltips.cpp @@ -435,6 +435,10 @@ void Tooltips::addStandardTooltips() { << tr("Adjust Beatgrid") << tr("Adjust beatgrid to match another playing deck."); + add("beats_undo_adjustment") + << tr("Revert last BPM/Beatgrid Change") + << tr("Revert last BPM/Beatgrid Change of the loaded track."); + //this is a special case, in some skins we might display a transparent png for bpm_tap on top of visual_bpm add("bpm_tap_visual_bpm") << tr("Tempo and BPM Tap") diff --git a/src/track/track.cpp b/src/track/track.cpp index 659f37ce717..dd0927b7b54 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -55,6 +55,8 @@ inline mixxx::Bpm getBeatsPointerBpm( return pBeats->getBpmInRange(mixxx::audio::kStartFramePos, trackEndPosition); } +constexpr int kMaxBeatsUndoStack = 10; + } // anonymous namespace // Don't change this string without an entry in the CHANGELOG! @@ -83,7 +85,8 @@ Track::Track( m_fileAccess(std::move(fileAccess)), m_record(trackId), m_bDirty(false), - m_bMarkedForMetadataExport(false) { + m_bMarkedForMetadataExport(false), + m_undoingBeatsChange(false) { if (kLogStats && kLogger.debugEnabled()) { long numberOfInstancesBefore = s_numberOfInstances.fetch_add(1); kLogger.debug() @@ -429,6 +432,15 @@ bool Track::setBeatsWhileLocked(mixxx::BeatsPointer pBeats) { if (m_pBeats == pBeats) { return false; } + // 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. + if (!m_undoingBeatsChange && m_pBeats != nullptr) { + if (m_pBeatsUndoStack.size() >= kMaxBeatsUndoStack) { + m_pBeatsUndoStack.removeFirst(); + } + m_pBeatsUndoStack.push(m_pBeats); + } m_pBeats = std::move(pBeats); m_record.refMetadata().refTrackInfo().setBpm(getBeatsPointerBpm(m_pBeats, getDuration())); return true; @@ -472,6 +484,18 @@ mixxx::BeatsPointer Track::getBeats() const { return m_pBeats; } +void Track::undoBeatsChange() { + if (!canUndoBeatsChange()) { + return; + } + + auto locked = lockMutex(&m_qMutex); + m_undoingBeatsChange = true; + const auto pPrevBeats = m_pBeatsUndoStack.pop(); + trySetBeats(pPrevBeats); + m_undoingBeatsChange = false; +} + void Track::afterBeatsAndBpmUpdated( QT_RECURSIVE_MUTEX_LOCKER* pLock) { DEBUG_ASSERT(pLock); diff --git a/src/track/track.h b/src/track/track.h index 17ecb259242..038de45b3b9 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -2,6 +2,7 @@ #include #include +#include #include #include "audio/streaminfo.h" @@ -348,6 +349,11 @@ class Track : public QObject { bool trySetBeats(mixxx::BeatsPointer pBeats); bool trySetAndLockBeats(mixxx::BeatsPointer pBeats); + void undoBeatsChange(); + bool canUndoBeatsChange() const { + return !m_pBeatsUndoStack.isEmpty(); + } + /// Imports the given list of cue infos as cue points, /// thereby replacing all existing cue points! /// @@ -561,6 +567,8 @@ class Track : public QObject { // Storage for the track's beats mixxx::BeatsPointer m_pBeats; + QStack m_pBeatsUndoStack; + bool m_undoingBeatsChange; // Visual waveform data ConstWaveformPointer m_waveform; diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 889e7e646e8..d318511f6e3 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -486,6 +486,12 @@ void WTrackMenu::createActions() { &QAction::triggered, this, &WTrackMenu::slotClearBeats); + + m_pBpmUndoAction = new QAction(tr("Undo last BPM/beats change"), m_pBPMMenu); + connect(m_pBpmUndoAction, + &QAction::triggered, + this, + &WTrackMenu::slotUndoBeatsChange); } if (featureIsEnabled(Feature::Analyze)) { @@ -601,7 +607,7 @@ void WTrackMenu::setupActions() { m_pBPMMenu->addAction(m_pBpmUnlockAction); m_pBPMMenu->addSeparator(); m_pBPMMenu->addAction(m_pBpmResetAction); - m_pBPMMenu->addSeparator(); + m_pBPMMenu->addAction(m_pBpmUndoAction); addMenu(m_pBPMMenu); } @@ -965,6 +971,7 @@ void WTrackMenu::updateMenus() { m_pBpmFourThirdsAction->setEnabled(!anyBpmLocked); m_pBpmThreeHalvesAction->setEnabled(!anyBpmLocked); m_pBpmResetAction->setEnabled(!anyBpmLocked); + m_pBpmUndoAction->setEnabled(!anyBpmLocked && canUndoBeatsChange()); // Append scaled BPM preview for single selection if (singleTrackSelected) { @@ -1596,6 +1603,50 @@ void WTrackMenu::slotScaleBpm(mixxx::Beats::BpmScale scale) { namespace { +class UndoBeatsChangeTrackPointerOperation : public mixxx::TrackPointerOperation { + public: + explicit UndoBeatsChangeTrackPointerOperation() { + } + + private: + void doApply( + const TrackPointer& pTrack) const override { + if (pTrack->isBpmLocked()) { + return; + } + pTrack->undoBeatsChange(); + } +}; + +} // anonymous namespace + +void WTrackMenu::slotUndoBeatsChange() { + const auto progressLabelText = + tr("Undo BPM/beats change of %n track(s)", "", getTrackCount()); + const auto trackOperator = + UndoBeatsChangeTrackPointerOperation(); + applyTrackPointerOperation( + progressLabelText, + &trackOperator); +} + +bool WTrackMenu::canUndoBeatsChange() const { + const auto pTrackPointerIterator = newTrackPointerIterator(); + if (!pTrackPointerIterator) { + // Empty, i.e. nothing to do + return false; + } + while (auto nextTrackPointer = pTrackPointerIterator->nextItem()) { + const auto pTrack = *nextTrackPointer; + if (!pTrack->canUndoBeatsChange()) { + return false; + } + } + return true; +} + +namespace { + class LockBpmTrackPointerOperation : public mixxx::TrackPointerOperation { public: explicit LockBpmTrackPointerOperation(bool lock) diff --git a/src/widget/wtrackmenu.h b/src/widget/wtrackmenu.h index 4b44bbd8edb..40e45537be0 100644 --- a/src/widget/wtrackmenu.h +++ b/src/widget/wtrackmenu.h @@ -145,6 +145,7 @@ class WTrackMenu : public QMenu { void slotLockBpm(); void slotUnlockBpm(); void slotScaleBpm(mixxx::Beats::BpmScale scale); + void slotUndoBeatsChange(); // Info and metadata void slotUpdateReplayGainFromPregain(); @@ -221,6 +222,7 @@ class WTrackMenu : public QMenu { void clearTrackSelection(); std::pair getTrackBpmLockStates() const; + bool canUndoBeatsChange() const; /// Get the common track color of all tracks this menu is shown for, or /// return `nullopt` if there is no common color. Tracks may have no color @@ -302,6 +304,7 @@ class WTrackMenu : public QMenu { QAction* m_pBpmFourThirdsAction{}; QAction* m_pBpmThreeHalvesAction{}; QAction* m_pBpmResetAction{}; + QAction* m_pBpmUndoAction{}; // Track color WColorPickerAction* m_pColorPickerAction{}; From 2d28bcf54e459f9e3267e20ef398260774e8787a Mon Sep 17 00:00:00 2001 From: ronso0 Date: Tue, 19 Mar 2024 15:00:48 +0100 Subject: [PATCH 2/3] Beats undo: don't record actions done in quick succession --- src/track/track.cpp | 14 +++++++++++++- src/track/track.h | 2 ++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/track/track.cpp b/src/track/track.cpp index dd0927b7b54..9a449af8f38 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -8,6 +8,7 @@ #include "sources/metadatasource.h" #include "util/assert.h" #include "util/logger.h" +#include "util/time.h" namespace { @@ -56,6 +57,9 @@ inline mixxx::Bpm getBeatsPointerBpm( } constexpr int kMaxBeatsUndoStack = 10; +// The minimum time that has to pass between beat changes to consider them 'separate'. +// Used to filter actions done in quick succession. +constexpr int kQuickBeatChangeDeltaMillis = 800; } // anonymous namespace @@ -96,6 +100,7 @@ Track::Track( << "->" << numberOfInstancesBefore + 1; } + m_beatChangeTimer.start(); } Track::~Track() { @@ -439,8 +444,15 @@ bool Track::setBeatsWhileLocked(mixxx::BeatsPointer pBeats) { if (m_pBeatsUndoStack.size() >= kMaxBeatsUndoStack) { m_pBeatsUndoStack.removeFirst(); } - m_pBeatsUndoStack.push(m_pBeats); + + // If changes done in quick succession, e.g. quick beats_translate_later, + // we only store the beats from before the first quick action. + mixxx::Duration elapsed = m_beatChangeTimer.restart(); + if (elapsed > mixxx::Duration::fromMillis(kQuickBeatChangeDeltaMillis)) { + m_pBeatsUndoStack.push(m_pBeats); + } } + m_pBeats = std::move(pBeats); m_record.refMetadata().refTrackInfo().setBpm(getBeatsPointerBpm(m_pBeats, getDuration())); return true; diff --git a/src/track/track.h b/src/track/track.h index 038de45b3b9..86f449d9b5a 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -16,6 +16,7 @@ #include "util/compatibility/qmutex.h" #include "util/fileaccess.h" #include "util/memory.h" +#include "util/performancetimer.h" #include "waveform/waveform.h" class Track : public QObject { @@ -569,6 +570,7 @@ class Track : public QObject { mixxx::BeatsPointer m_pBeats; QStack m_pBeatsUndoStack; bool m_undoingBeatsChange; + PerformanceTimer m_beatChangeTimer; // Visual waveform data ConstWaveformPointer m_waveform; From c8dd63ca4c622f41569f9dc9a4c0fa1ca531e6d9 Mon Sep 17 00:00:00 2001 From: ronso0 Date: Fri, 3 May 2024 12:19:14 +0200 Subject: [PATCH 3/3] Tests: add test for `beats_undo_adjustment` --- src/test/beatstranslatetest.cpp | 72 +++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/src/test/beatstranslatetest.cpp b/src/test/beatstranslatetest.cpp index bf7b7052201..e3e925df134 100644 --- a/src/test/beatstranslatetest.cpp +++ b/src/test/beatstranslatetest.cpp @@ -51,3 +51,75 @@ TEST_F(BeatsTranslateTest, SimpleTranslateMatch) { mixxx::BeatsPointer pBeats = m_pTrack2->getBeats(); EXPECT_FRAMEPOS_EQ(seekPosition * -1, pBeats->findClosestBeat(mixxx::audio::kStartFramePos)); } + +TEST_F(BeatsTranslateTest, BeatsUndoTest) { + const auto bpm60 = mixxx::Bpm(60.0); + const auto origin123 = mixxx::audio::FramePos{123.0}; + auto grid = mixxx::Beats::fromConstTempo( + m_pTrack1->getSampleRate(), mixxx::audio::kStartFramePos, bpm60); + m_pTrack1->trySetBeats(grid); + EXPECT_FRAMEPOS_EQ(mixxx::audio::kStartFramePos, mixxx::audio::kStartFramePos); + + auto pBpm = std::make_unique(m_sGroup1, "bpm"); + auto pBeatsTranslateCurPos = + std::make_unique(m_sGroup1, "beats_translate_curpos"); + auto pBeatsUndo = std::make_unique(m_sGroup1, "beats_undo_adjustment"); + pBpm->set(bpm60.value()); + m_pChannel1->getEngineBuffer()->seekAbs(origin123); + ProcessBuffer(); // first process to schedule seek in a stopped deck + ProcessBuffer(); // then seek + // Beats undo ignores changes done in quick succession (ignore delay is 800 millis), + // so after the each set of beat changes, the newest item in the undo stack + QTest::qSleep(810); + + // Populate the beats_undo stack with 10 "beats_translate_curpos" actions at + // different positions. + // should be the state from before the sequence. + // In order to have a before state, we translate the beats to 123. + pBeatsTranslateCurPos->set(1.0); + pBeatsTranslateCurPos->set(0.0); + // Now sleep so only the steps of the sequence are recognized as quick actions. + auto newSeekPosition = m_pChannel1->getEngineBuffer()->getExactPlayPos(); + EXPECT_FRAMEPOS_EQ(origin123, newSeekPosition); + QTest::qSleep(810); + + for (int i = 0; i < 5; i++) { + newSeekPosition += 5.0; + m_pChannel1->getEngineBuffer()->seekAbs(newSeekPosition); + ProcessBuffer(); // first process to schedule seek in a stopped deck + ProcessBuffer(); // then seek + pBeatsTranslateCurPos->set(1.0); + pBeatsTranslateCurPos->set(0.0); + } + + // We should be at 148 now + newSeekPosition = m_pChannel1->getEngineBuffer()->getExactPlayPos(); + const auto pos148 = mixxx::audio::FramePos{148.0}; + EXPECT_FRAMEPOS_EQ(newSeekPosition, pos148); + + // Sleep to keep the current state in the undo stack. + QTest::qSleep(810); + + for (int i = 0; i < 5; i++) { + newSeekPosition += 5.0; + m_pChannel1->getEngineBuffer()->seekAbs(newSeekPosition); + ProcessBuffer(); // first process to schedule seek in a stopped deck + ProcessBuffer(); // then seek + pBeatsTranslateCurPos->set(1.0); + pBeatsTranslateCurPos->set(0.0); + } + // After this quick sequence we're at frame 173. + newSeekPosition = m_pChannel1->getEngineBuffer()->getExactPlayPos(); + EXPECT_FRAMEPOS_EQ(newSeekPosition, mixxx::audio::FramePos{173.0}); + + pBeatsUndo->set(1.0); + pBeatsUndo->set(0.0); + mixxx::BeatsPointer pBeats = m_pTrack1->getBeats(); + EXPECT_FRAMEPOS_EQ(pos148, pBeats->findClosestBeat(mixxx::audio::kStartFramePos)); + + // Undo once more to get back to 123. + pBeatsUndo->set(1.0); + pBeatsUndo->set(0.0); + pBeats = m_pTrack1->getBeats(); + EXPECT_FRAMEPOS_EQ(origin123, pBeats->findClosestBeat(mixxx::audio::kStartFramePos)); +}