Beats: Make return value of mutation methods optional

This allows the caller to handle the failure case more efficiently, e.g.
by exiting early if nothing changes.

If the caller does not care wether the operation worked and resulted in
changes or not and simply needs a valid beats pointer, the following
expression can be used:

    pBeats->scale(...).value_or(pBeats)

src/engine/controls/bpmcontrol.cpp

@@ -162,7 +162,11 @@ void BpmControl::adjustBeatsBpm(double deltaBpm) {
     const auto centerBpm = mixxx::Bpm(math_max(kBpmAdjustMin, bpm.value() + deltaBpm));
     mixxx::Bpm adjustedBpm = BeatUtils::roundBpmWithinRange(
             centerBpm - kBpmAdjustStep / 2, centerBpm, centerBpm + kBpmAdjustStep / 2);
-    pTrack->trySetBeats(pBeats->setBpm(adjustedBpm));
+    const auto newBeats = pBeats->setBpm(adjustedBpm);
+    if (!newBeats) {
+        return;
+    }
+    pTrack->trySetBeats(*newBeats);
 }
 
 void BpmControl::slotAdjustBeatsFaster(double v) {
@@ -191,7 +195,10 @@ void BpmControl::slotTranslateBeatsEarlier(double v) {
     if (pBeats) {
         const double sampleOffset = frameInfo().sampleRate * -0.01;
         const mixxx::audio::FrameDiff_t frameOffset = sampleOffset / mixxx::kEngineChannelCount;
-        pTrack->trySetBeats(pBeats->translate(frameOffset));
+        const auto translatedBeats = pBeats->translate(frameOffset);
+        if (translatedBeats) {
+            pTrack->trySetBeats(*translatedBeats);
+        }
     }
 }
 
@@ -208,7 +215,10 @@ void BpmControl::slotTranslateBeatsLater(double v) {
         // TODO(rryan): Track::frameInfo is possibly inaccurate!
         const double sampleOffset = frameInfo().sampleRate * 0.01;
         const mixxx::audio::FrameDiff_t frameOffset = sampleOffset / mixxx::kEngineChannelCount;
-        pTrack->trySetBeats(pBeats->translate(frameOffset));
+        const auto translatedBeats = pBeats->translate(frameOffset);
+        if (translatedBeats) {
+            pTrack->trySetBeats(*translatedBeats);
+        }
     }
 }
 
@@ -245,7 +255,11 @@ void BpmControl::slotTapFilter(double averageLength, int numSamples) {
     averageBpm = BeatUtils::roundBpmWithinRange(averageBpm - kBpmTabRounding,
             averageBpm,
             averageBpm + kBpmTabRounding);
-    pTrack->trySetBeats(pBeats->setBpm(averageBpm));
+    const auto newBeats = pBeats->setBpm(averageBpm);
+    if (!newBeats) {
+        return;
+    }
+    pTrack->trySetBeats(*newBeats);
 }
 
 // static
@@ -961,8 +975,11 @@ void BpmControl::slotBeatsTranslate(double v) {
     if (pBeats) {
         const auto currentPosition = frameInfo().currentPosition.toLowerFrameBoundary();
         const auto closestBeat = pBeats->findClosestBeat(currentPosition);
-        const mixxx::audio::FrameDiff_t delta = currentPosition - closestBeat;
-        pTrack->trySetBeats(pBeats->translate(delta));
+        const mixxx::audio::FrameDiff_t frameOffset = currentPosition - closestBeat;
+        const auto translatedBeats = pBeats->translate(frameOffset);
+        if (translatedBeats) {
+            pTrack->trySetBeats(*translatedBeats);
+        }
     }
 }
 
@@ -980,8 +997,11 @@ void BpmControl::slotBeatsTranslateMatchAlignment(double v) {
         // otherwise it will always return 0 if sync lock is active.
         m_dUserOffset.setValue(0.0);
 
-        const mixxx::audio::FrameDiff_t frameOffset = getPhaseOffset(frameInfo().currentPosition);
-        pTrack->trySetBeats(pBeats->translate(-frameOffset));
+        const mixxx::audio::FrameDiff_t frameOffset = -getPhaseOffset(frameInfo().currentPosition);
+        const auto translatedBeats = pBeats->translate(frameOffset);
+        if (translatedBeats) {
+            pTrack->trySetBeats(*translatedBeats);
+        }
     }
 }
 

src/library/dlgtrackinfo.cpp

@@ -520,33 +520,35 @@ void DlgTrackInfo::clear() {
 }
 
 void DlgTrackInfo::slotBpmDouble() {
-    m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::Double);
-    updateSpinBpmFromBeats();
+    slotBpmScale(mixxx::Beats::BpmScale::Double);
 }
 
 void DlgTrackInfo::slotBpmHalve() {
-    m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::Halve);
-    updateSpinBpmFromBeats();
+    slotBpmScale(mixxx::Beats::BpmScale::Halve);
 }
 
 void DlgTrackInfo::slotBpmTwoThirds() {
-    m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::TwoThirds);
-    updateSpinBpmFromBeats();
+    slotBpmScale(mixxx::Beats::BpmScale::TwoThirds);
 }
 
 void DlgTrackInfo::slotBpmThreeFourth() {
-    m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::ThreeFourths);
-    updateSpinBpmFromBeats();
+    slotBpmScale(mixxx::Beats::BpmScale::ThreeFourths);
 }
 
 void DlgTrackInfo::slotBpmFourThirds() {
-    m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::FourThirds);
-    updateSpinBpmFromBeats();
+    slotBpmScale(mixxx::Beats::BpmScale::FourThirds);
 }
 
 void DlgTrackInfo::slotBpmThreeHalves() {
-    m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::ThreeHalves);
-    updateSpinBpmFromBeats();
+    slotBpmScale(mixxx::Beats::BpmScale::ThreeHalves);
+}
+
+void DlgTrackInfo::slotBpmScale(mixxx::Beats::BpmScale bpmScale) {
+    const auto scaledBeats = m_pBeatsClone->scale(bpmScale);
+    if (scaledBeats) {
+        m_pBeatsClone = *scaledBeats;
+        updateSpinBpmFromBeats();
+    }
 }
 
 void DlgTrackInfo::slotBpmClear() {
@@ -610,7 +612,7 @@ void DlgTrackInfo::slotSpinBpmValueChanged(double value) {
         if (oldValue == bpm) {
             return;
         }
-        m_pBeatsClone = m_pBeatsClone->setBpm(bpm);
+        m_pBeatsClone = m_pBeatsClone->setBpm(bpm).value_or(m_pBeatsClone);
     }
 
     updateSpinBpmFromBeats();

src/library/dlgtrackinfo.h

@@ -57,6 +57,7 @@ class DlgTrackInfo : public QDialog, public Ui::DlgTrackInfo {
     void slotBpmThreeFourth();
     void slotBpmFourThirds();
     void slotBpmThreeHalves();
+    void slotBpmScale(mixxx::Beats::BpmScale bpmScale);
     void slotBpmClear();
     void slotBpmConstChanged(int state);
     void slotBpmTap(double averageLength, int numSamples);

src/test/beatgridtest.cpp

@@ -34,22 +34,22 @@ TEST(BeatGridTest, Scale) {
             mixxx::Bpm(bpm));
 
     EXPECT_DOUBLE_EQ(bpm.value(), pGrid->getBpm().value());
-    pGrid = pGrid->scale(Beats::BpmScale::Double);
+    pGrid = *pGrid->scale(Beats::BpmScale::Double);
     EXPECT_DOUBLE_EQ(2 * bpm.value(), pGrid->getBpm().value());
 
-    pGrid = pGrid->scale(Beats::BpmScale::Halve);
+    pGrid = *pGrid->scale(Beats::BpmScale::Halve);
     EXPECT_DOUBLE_EQ(bpm.value(), pGrid->getBpm().value());
 
-    pGrid = pGrid->scale(Beats::BpmScale::TwoThirds);
+    pGrid = *pGrid->scale(Beats::BpmScale::TwoThirds);
     EXPECT_DOUBLE_EQ(bpm.value() * 2 / 3, pGrid->getBpm().value());
 
-    pGrid = pGrid->scale(Beats::BpmScale::ThreeHalves);
+    pGrid = *pGrid->scale(Beats::BpmScale::ThreeHalves);
     EXPECT_DOUBLE_EQ(bpm.value(), pGrid->getBpm().value());
 
-    pGrid = pGrid->scale(Beats::BpmScale::ThreeFourths);
+    pGrid = *pGrid->scale(Beats::BpmScale::ThreeFourths);
     EXPECT_DOUBLE_EQ(bpm.value() * 3 / 4, pGrid->getBpm().value());
 
-    pGrid = pGrid->scale(Beats::BpmScale::FourThirds);
+    pGrid = *pGrid->scale(Beats::BpmScale::FourThirds);
     EXPECT_DOUBLE_EQ(bpm.value(), pGrid->getBpm().value());
 }
 

src/test/beatmaptest.cpp

@@ -63,22 +63,22 @@ TEST_F(BeatMapTest, Scale) {
     auto pMap = Beats::fromBeatPositions(m_pTrack->getSampleRate(), beats);
 
     EXPECT_DOUBLE_EQ(bpm.value(), pMap->getBpm().value());
-    pMap = pMap->scale(Beats::BpmScale::Double);
+    pMap = *pMap->scale(Beats::BpmScale::Double);
     EXPECT_DOUBLE_EQ(2 * bpm.value(), pMap->getBpm().value());
 
-    pMap = pMap->scale(Beats::BpmScale::Halve);
+    pMap = *pMap->scale(Beats::BpmScale::Halve);
     EXPECT_DOUBLE_EQ(bpm.value(), pMap->getBpm().value());
 
-    pMap = pMap->scale(Beats::BpmScale::TwoThirds);
+    pMap = *pMap->scale(Beats::BpmScale::TwoThirds);
     EXPECT_DOUBLE_EQ(bpm.value() * 2 / 3, pMap->getBpm().value());
 
-    pMap = pMap->scale(Beats::BpmScale::ThreeHalves);
+    pMap = *pMap->scale(Beats::BpmScale::ThreeHalves);
     EXPECT_DOUBLE_EQ(bpm.value(), pMap->getBpm().value());
 
-    pMap = pMap->scale(Beats::BpmScale::ThreeFourths);
+    pMap = *pMap->scale(Beats::BpmScale::ThreeFourths);
     EXPECT_DOUBLE_EQ(bpm.value() * 3 / 4, pMap->getBpm().value());
 
-    pMap = pMap->scale(Beats::BpmScale::FourThirds);
+    pMap = *pMap->scale(Beats::BpmScale::FourThirds);
     EXPECT_DOUBLE_EQ(bpm.value(), pMap->getBpm().value());
 }
 

src/track/beatgrid.cpp

@@ -261,9 +261,9 @@ mixxx::Bpm BeatGrid::getBpmAroundPosition(audio::FramePos position, int n) const
     return getBpm();
 }
 
-BeatsPointer BeatGrid::translate(audio::FrameDiff_t offset) const {
+std::optional<BeatsPointer> BeatGrid::translate(audio::FrameDiff_t offset) const {
     VERIFY_OR_DEBUG_ASSERT(isValid()) {
-        return clonePointer();
+        return std::nullopt;
     }
 
     mixxx::track::io::BeatGrid grid = m_grid;
@@ -275,9 +275,9 @@ BeatsPointer BeatGrid::translate(audio::FrameDiff_t offset) const {
     return std::make_shared<BeatGrid>(MakeSharedTag{}, *this, grid, m_beatLengthFrames);
 }
 
-BeatsPointer BeatGrid::scale(BpmScale scale) const {
+std::optional<BeatsPointer> BeatGrid::scale(BpmScale scale) const {
     VERIFY_OR_DEBUG_ASSERT(isValid()) {
-        return clonePointer();
+        return std::nullopt;
     }
 
     mixxx::track::io::BeatGrid grid = m_grid;
@@ -304,11 +304,12 @@ BeatsPointer BeatGrid::scale(BpmScale scale) const {
         break;
     default:
         DEBUG_ASSERT(!"scale value invalid");
-        return clonePointer();
+        return std::nullopt;
     }
 
     if (!bpm.isValid()) {
-        return clonePointer();
+        qWarning() << "BeatGrid: Scaling would result in invalid BPM!";
+        return std::nullopt;
     }
 
     bpm = BeatUtils::roundBpmWithinRange(bpm - kBpmScaleRounding, bpm, bpm + kBpmScaleRounding);
@@ -317,9 +318,9 @@ BeatsPointer BeatGrid::scale(BpmScale scale) const {
     return std::make_shared<BeatGrid>(MakeSharedTag{}, *this, grid, beatLengthFrames);
 }
 
-BeatsPointer BeatGrid::setBpm(mixxx::Bpm bpm) const {
+std::optional<BeatsPointer> BeatGrid::setBpm(mixxx::Bpm bpm) const {
     VERIFY_OR_DEBUG_ASSERT(bpm.isValid()) {
-        return clonePointer();
+        return std::nullopt;
     }
 
     mixxx::track::io::BeatGrid grid = m_grid;

src/track/beatgrid.h

@@ -62,9 +62,9 @@ class BeatGrid final : public Beats {
     // Beat mutations
     ////////////////////////////////////////////////////////////////////////////
 
-    BeatsPointer translate(audio::FrameDiff_t offset) const override;
-    BeatsPointer scale(BpmScale scale) const override;
-    BeatsPointer setBpm(mixxx::Bpm bpm) const override;
+    std::optional<BeatsPointer> translate(audio::FrameDiff_t offset) const override;
+    std::optional<BeatsPointer> scale(BpmScale scale) const override;
+    std::optional<BeatsPointer> setBpm(mixxx::Bpm bpm) const override;
 
     ////////////////////////////////////////////////////////////////////////////
     // Hidden constructors

src/track/beatmap.cpp

@@ -532,9 +532,9 @@ mixxx::Bpm BeatMap::getBpmAroundPosition(audio::FramePos position, int n) const
             numberOfBeats, m_sampleRate, lowerFrame, upperFrame);
 }
 
-BeatsPointer BeatMap::translate(audio::FrameDiff_t offset) const {
+std::optional<BeatsPointer> BeatMap::translate(audio::FrameDiff_t offset) const {
     if (!isValid()) {
-        return clonePointer();
+        return std::nullopt;
     }
 
     BeatList beats = m_beats;
@@ -555,9 +555,9 @@ BeatsPointer BeatMap::translate(audio::FrameDiff_t offset) const {
     return std::make_shared<BeatMap>(MakeSharedTag{}, *this, beats, m_nominalBpm);
 }
 
-BeatsPointer BeatMap::scale(BpmScale scale) const {
+std::optional<BeatsPointer> BeatMap::scale(BpmScale scale) const {
     VERIFY_OR_DEBUG_ASSERT(isValid()) {
-        return clonePointer();
+        return std::nullopt;
     }
 
     BeatList beats = m_beats;
@@ -596,16 +596,16 @@ BeatsPointer BeatMap::scale(BpmScale scale) const {
         break;
     default:
         DEBUG_ASSERT(!"scale value invalid");
-        return clonePointer();
+        return std::nullopt;
     }
 
     mixxx::Bpm bpm = calculateNominalBpm(beats, m_sampleRate);
     return std::make_shared<BeatMap>(MakeSharedTag{}, *this, beats, bpm);
 }
 
-BeatsPointer BeatMap::setBpm(mixxx::Bpm bpm) const {
+std::optional<BeatsPointer> BeatMap::setBpm(mixxx::Bpm bpm) const {
     VERIFY_OR_DEBUG_ASSERT(bpm.isValid()) {
-        return clonePointer();
+        return std::nullopt;
     }
 
     const auto firstBeatPosition = mixxx::audio::FramePos(m_beats.first().frame_position());

src/track/beatmap.h

@@ -64,9 +64,9 @@ class BeatMap final : public Beats {
     // Beat mutations
     ////////////////////////////////////////////////////////////////////////////
 
-    BeatsPointer translate(audio::FrameDiff_t offset) const override;
-    BeatsPointer scale(BpmScale scale) const override;
-    BeatsPointer setBpm(mixxx::Bpm bpm) const override;
+    std::optional<BeatsPointer> translate(audio::FrameDiff_t offset) const override;
+    std::optional<BeatsPointer> scale(BpmScale scale) const override;
+    std::optional<BeatsPointer> setBpm(mixxx::Bpm bpm) const override;
 
     ////////////////////////////////////////////////////////////////////////////
     // Hidden constructors

src/track/beats.h

@@ -5,6 +5,7 @@
 #include <QString>
 #include <QVector>
 #include <memory>
+#include <optional>
 
 #include "audio/frame.h"
 #include "audio/types.h"
@@ -167,13 +168,22 @@ class Beats : private std::enable_shared_from_this<Beats> {
     /// Translate all beats in the song by `offset` frames. Beats that lie
     /// before the start of the track or after the end of the track are *not*
     /// removed.
-    virtual BeatsPointer translate(audio::FrameDiff_t offset) const = 0;
+    //
+    /// Returns a pointer to the modified beats object, or `nullopt` on
+    /// failure.
+    virtual std::optional<BeatsPointer> translate(audio::FrameDiff_t offset) const = 0;
 
     /// Scale the position of every beat in the song by `scale`.
-    virtual BeatsPointer scale(BpmScale scale) const = 0;
+    //
+    /// Returns a pointer to the modified beats object, or `nullopt` on
+    /// failure.
+    virtual std::optional<BeatsPointer> scale(BpmScale scale) const = 0;
 
     /// Adjust the beats so the global average BPM matches `bpm`.
-    virtual BeatsPointer setBpm(mixxx::Bpm bpm) const = 0;
+    //
+    /// Returns a pointer to the modified beats object, or `nullopt` on
+    /// failure.
+    virtual std::optional<BeatsPointer> setBpm(mixxx::Bpm bpm) const = 0;
 
   protected:
     /// Type tag for making public constructors of derived classes inaccessible.

src/track/track.cpp

@@ -359,10 +359,14 @@ bool Track::trySetBpmWhileLocked(mixxx::Bpm bpm) {
         return trySetBeatsWhileLocked(std::move(pBeats));
     } else if (m_pBeats->getBpm() != bpm) {
         // Continue with the regular cases
-        if (kLogger.debugEnabled()) {
-            kLogger.debug() << "Updating BPM:" << getLocation();
+        const auto newBeats = m_pBeats->setBpm(bpm);
+        if (newBeats) {
+            if (kLogger.debugEnabled()) {
+                kLogger.debug() << "Updating BPM:" << getLocation();
+            }
+
+            return trySetBeatsWhileLocked(*newBeats);
         }
-        return trySetBeatsWhileLocked(m_pBeats->setBpm(bpm));
     }
     return false;
 }

src/widget/wtrackmenu.cpp

@@ -1231,7 +1231,11 @@ class ScaleBpmTrackPointerOperation : public mixxx::TrackPointerOperation {
         if (!pBeats) {
             return;
         }
-        pTrack->trySetBeats(pBeats->scale(m_bpmScale));
+        const auto scaledBeats = pBeats->scale(m_bpmScale);
+        if (!scaledBeats) {
+            return;
+        }
+        pTrack->trySetBeats(*scaledBeats);
     }
 
     const mixxx::Beats::BpmScale m_bpmScale;