Merge pull request #12954 from ronso0/beats-undo

Beats: allow undoing the last BPM/beats change

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);
 

src/engine/controls/bpmcontrol.cpp

@@ -138,6 +138,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);
@@ -154,6 +161,7 @@ BpmControl::~BpmControl() {
     delete m_pTranslateBeatsMove;
     delete m_pAdjustBeatsFaster;
     delete m_pAdjustBeatsSlower;
+    delete m_pBeatsUndo;
 }
 
 mixxx::Bpm BpmControl::getBpm() const {
@@ -232,6 +240,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();

src/engine/controls/bpmcontrol.h

@@ -110,6 +110,7 @@ class BpmControl : public EngineControl {
     void slotBeatsTranslate(double);
     void slotBeatsTranslateMatchAlignment(double);
     void slotToggleBpmLock(double);
+    void slotBeatsUndoAdjustment(double value);
 
   private:
     SyncMode getSyncMode() const {
@@ -146,6 +147,7 @@ class BpmControl : public EngineControl {
     ControlPushButton* m_pTranslateBeatsEarlier;
     ControlPushButton* m_pTranslateBeatsLater;
     ControlEncoder* m_pTranslateBeatsMove;
+    ControlPushButton* m_pBeatsUndo;
 
     std::unique_ptr<ControlPushButton> m_pBpmLock;
 

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")

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<ControlProxy>(m_sGroup1, "bpm");
+    auto pBeatsTranslateCurPos =
+            std::make_unique<ControlProxy>(m_sGroup1, "beats_translate_curpos");
+    auto pBeatsUndo = std::make_unique<ControlProxy>(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));
+}

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 {
 
@@ -55,6 +56,11 @@ inline mixxx::Bpm getBeatsPointerBpm(
     return pBeats->getBpmInRange(mixxx::audio::kStartFramePos, trackEndPosition);
 }
 
+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
 
 // Don't change this string without an entry in the CHANGELOG!
@@ -83,7 +89,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()
@@ -93,6 +100,7 @@ Track::Track(
                 << "->"
                 << numberOfInstancesBefore + 1;
     }
+    m_beatChangeTimer.start();
 }
 
 Track::~Track() {
@@ -429,6 +437,22 @@ 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();
+        }
+
+        // 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;
@@ -472,6 +496,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);

src/track/track.h

@@ -2,6 +2,7 @@
 
 #include <QList>
 #include <QObject>
+#include <QStack>
 #include <QUrl>
 
 #include "audio/streaminfo.h"
@@ -15,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 {
@@ -348,6 +350,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!
     ///
@@ -562,6 +569,9 @@ class Track : public QObject {
 
     // Storage for the track's beats
     mixxx::BeatsPointer m_pBeats;
+    QStack<mixxx::BeatsPointer> m_pBeatsUndoStack;
+    bool m_undoingBeatsChange;
+    PerformanceTimer m_beatChangeTimer;
 
     // Visual waveform data
     ConstWaveformPointer m_waveform;

src/widget/wtrackmenu.cpp

@@ -499,6 +499,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)) {
@@ -614,7 +620,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);
     }
@@ -1015,6 +1021,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) {
@@ -1666,6 +1673,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)

src/widget/wtrackmenu.h

@@ -150,6 +150,7 @@ class WTrackMenu : public QMenu {
     void slotLockBpm();
     void slotUnlockBpm();
     void slotScaleBpm(mixxx::Beats::BpmScale scale);
+    void slotUndoBeatsChange();
 
     // Info and metadata
     void slotUpdateReplayGainFromPregain();
@@ -227,6 +228,7 @@ class WTrackMenu : public QMenu {
     void clearTrackSelection();
 
     std::pair<bool, bool> getTrackBpmLockStates() const;
+    bool canUndoBeatsChange() const;
 
     /// Get the common rating of all selected tracks.
     /// Return 0 if ratings differ.
@@ -312,6 +314,7 @@ class WTrackMenu : public QMenu {
     QAction* m_pBpmFourThirdsAction{};
     QAction* m_pBpmThreeHalvesAction{};
     QAction* m_pBpmResetAction{};
+    QAction* m_pBpmUndoAction{};
 
     // Track rating and color
     WStarRatingAction* m_pStarRatingAction{};