Add and use mixxx::Bpm::valueOr()

src/library/basetracktablemodel.cpp

@@ -782,8 +782,10 @@ QVariant BaseTrackTableModel::roleValue(
         case ColumnCache::COLUMN_LIBRARYTABLE_BPM: {
             bool ok;
             const auto bpmValue = rawValue.toDouble(&ok);
-            // FIXME: calling bpm.value() without checking bpm.isValid()
-            return ok ? bpmValue : mixxx::Bpm().value();
+            if (!ok) {
+                return mixxx::Bpm::kValueUndefined;
+            }
+            return mixxx::Bpm{bpmValue}.valueOr(mixxx::Bpm::kValueUndefined);
         }
         case ColumnCache::COLUMN_LIBRARYTABLE_TIMESPLAYED:
             return index.sibling(

src/library/dlgtrackinfo.cpp

@@ -366,14 +366,16 @@ void DlgTrackInfo::updateTrackMetadataFields() {
                     m_trackRecord.getMetadata().getTrackInfo().getReplayGain().getRatio()));
 }
 
+void DlgTrackInfo::updateSpinBpmFromBeats() {
+    const auto bpmValue = m_pBeatsClone
+            ? m_pBeatsClone->getBpm().valueOr(mixxx::Bpm::kValueUndefined)
+            : mixxx::Bpm::kValueUndefined;
+    spinBpm->setValue(bpmValue);
+}
+
 void DlgTrackInfo::reloadTrackBeats(const Track& track) {
     m_pBeatsClone = track.getBeats();
-    if (m_pBeatsClone) {
-        // FIXME: calling bpm.value() without checking bpm.isValid()
-        spinBpm->setValue(m_pBeatsClone->getBpm().value());
-    } else {
-        spinBpm->setValue(0.0);
-    }
+    updateSpinBpmFromBeats();
     m_trackHasBeatMap = m_pBeatsClone &&
             !(m_pBeatsClone->getCapabilities() & mixxx::Beats::BEATSCAP_SETBPM);
     bpmConst->setChecked(!m_trackHasBeatMap);
@@ -512,63 +514,45 @@ void DlgTrackInfo::clear() {
 
     resetTrackRecord();
 
-    spinBpm->setValue(0.0);
     m_pBeatsClone.clear();
+    updateSpinBpmFromBeats();
 
     txtLocation->setText("");
 }
 
 void DlgTrackInfo::slotBpmDouble() {
     m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::Double);
-    // read back the actual value
-    mixxx::Bpm newValue = m_pBeatsClone->getBpm();
-    // FIXME: calling bpm.value() without checking bpm.isValid()
-    spinBpm->setValue(newValue.value());
+    updateSpinBpmFromBeats();
 }
 
 void DlgTrackInfo::slotBpmHalve() {
     m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::Halve);
-    // read back the actual value
-    const mixxx::Bpm newValue = m_pBeatsClone->getBpm();
-    // FIXME: calling bpm.value() without checking bpm.isValid()
-    spinBpm->setValue(newValue.value());
+    updateSpinBpmFromBeats();
 }
 
 void DlgTrackInfo::slotBpmTwoThirds() {
     m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::TwoThirds);
-    // read back the actual value
-    const mixxx::Bpm newValue = m_pBeatsClone->getBpm();
-    // FIXME: calling bpm.value() without checking bpm.isValid()
-    spinBpm->setValue(newValue.value());
+    updateSpinBpmFromBeats();
 }
 
 void DlgTrackInfo::slotBpmThreeFourth() {
     m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::ThreeFourths);
-    // read back the actual value
-    const mixxx::Bpm newValue = m_pBeatsClone->getBpm();
-    // FIXME: calling bpm.value() without checking bpm.isValid()
-    spinBpm->setValue(newValue.value());
+    updateSpinBpmFromBeats();
 }
 
 void DlgTrackInfo::slotBpmFourThirds() {
     m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::FourThirds);
-    // read back the actual value
-    const mixxx::Bpm newValue = m_pBeatsClone->getBpm();
-    // FIXME: calling bpm.value() without checking bpm.isValid()
-    spinBpm->setValue(newValue.value());
+    updateSpinBpmFromBeats();
 }
 
 void DlgTrackInfo::slotBpmThreeHalves() {
     m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::ThreeHalves);
-    // read back the actual value
-    const mixxx::Bpm newValue = m_pBeatsClone->getBpm();
-    // FIXME: calling bpm.value() without checking bpm.isValid()
-    spinBpm->setValue(newValue.value());
+    updateSpinBpmFromBeats();
 }
 
 void DlgTrackInfo::slotBpmClear() {
-    spinBpm->setValue(0);
     m_pBeatsClone.clear();
+    updateSpinBpmFromBeats();
 
     bpmConst->setChecked(true);
     bpmConst->setEnabled(m_trackHasBeatMap);
@@ -579,7 +563,8 @@ void DlgTrackInfo::slotBpmClear() {
 void DlgTrackInfo::slotBpmConstChanged(int state) {
     if (state != Qt::Unchecked) {
         // const beatgrid requested
-        if (spinBpm->value() > 0) {
+        const auto bpm = mixxx::Bpm(spinBpm->value());
+        if (bpm.isValid()) {
             // Since the user is not satisfied with the beat map,
             // it is hard to predict a fitting beat. We know that we
             // cannot use the first beat, since it is out of sync in
@@ -589,7 +574,7 @@ void DlgTrackInfo::slotBpmConstChanged(int state) {
             const mixxx::audio::FramePos cuePosition = m_pLoadedTrack->getMainCuePosition();
             m_pBeatsClone =
                     BeatFactory::makeBeatGrid(m_pLoadedTrack->getSampleRate(),
-                            mixxx::Bpm(spinBpm->value()),
+                            bpm,
                             cuePosition);
         } else {
             m_pBeatsClone.clear();
@@ -613,7 +598,7 @@ void DlgTrackInfo::slotBpmTap(double averageLength, int numSamples) {
             averageBpm + kBpmTabRounding);
     if (averageBpm != m_lastTapedBpm) {
         m_lastTapedBpm = averageBpm;
-        spinBpm->setValue(averageBpm.value());
+        spinBpm->setValue(averageBpm.valueOr(mixxx::Bpm::kValueUndefined));
     }
 }
 
@@ -641,9 +626,7 @@ void DlgTrackInfo::slotSpinBpmValueChanged(double value) {
         m_pBeatsClone = m_pBeatsClone->setBpm(bpm);
     }
 
-    // read back the actual value
-    const mixxx::Bpm newValue = m_pBeatsClone->getBpm();
-    spinBpm->setValue(newValue.value());
+    updateSpinBpmFromBeats();
 }
 
 mixxx::UpdateResult DlgTrackInfo::updateKeyText() {

src/library/dlgtrackinfo.h

@@ -103,6 +103,8 @@ class DlgTrackInfo : public QDialog, public Ui::DlgTrackInfo {
     }
 
     void updateTrackMetadataFields();
+    void updateSpinBpmFromBeats();
+
     const TrackModel* const m_pTrackModel;
 
     TrackPointer m_pLoadedTrack;

src/test/bpmtest.cpp

@@ -7,6 +7,49 @@
 class BpmTest : public testing::Test {
 };
 
+TEST_F(BpmTest, defaultCtor) {
+    EXPECT_EQ(mixxx::Bpm{mixxx::Bpm::kValueUndefined}, mixxx::Bpm{});
+}
+
+TEST_F(BpmTest, isValid) {
+    EXPECT_FALSE(mixxx::Bpm{mixxx::Bpm::kValueUndefined}.isValid());
+    EXPECT_FALSE(mixxx::Bpm{mixxx::Bpm::kValueMin}.isValid());
+    EXPECT_FALSE(mixxx::Bpm{-mixxx::Bpm::kValueMin}.isValid());
+    EXPECT_FALSE(mixxx::Bpm{mixxx::Bpm::kValueMin - 0.001}.isValid());
+    EXPECT_TRUE(mixxx::Bpm{mixxx::Bpm::kValueMin + 0.001}.isValid());
+    EXPECT_TRUE(mixxx::Bpm{mixxx::Bpm::kValueMax}.isValid());
+    EXPECT_FALSE(mixxx::Bpm{-mixxx::Bpm::kValueMax}.isValid());
+    EXPECT_TRUE(mixxx::Bpm{mixxx::Bpm::kValueMax - 0.001}.isValid());
+    // The upper bound is only a soft-limit!
+    EXPECT_TRUE(mixxx::Bpm{mixxx::Bpm::kValueMax + 0.001}.isValid());
+    EXPECT_TRUE(mixxx::Bpm{123.45}.isValid());
+    EXPECT_FALSE(mixxx::Bpm{-123.45}.isValid());
+}
+
+TEST_F(BpmTest, value) {
+    EXPECT_DOUBLE_EQ(123.45, mixxx::Bpm{123.45}.value());
+    EXPECT_DOUBLE_EQ(mixxx::Bpm::kValueMin + 0.001,
+            mixxx::Bpm{mixxx::Bpm::kValueMin + 0.001}.value());
+    // The upper bound is only a soft-limit!
+    EXPECT_DOUBLE_EQ(mixxx::Bpm::kValueMax + 0.001,
+            mixxx::Bpm{mixxx::Bpm::kValueMax + 0.001}.value());
+}
+
+TEST_F(BpmTest, valueOr) {
+    EXPECT_DOUBLE_EQ(123.45, mixxx::Bpm{123.45}.valueOr(-1.0));
+    EXPECT_EQ(-1.0, mixxx::Bpm{-123.45}.valueOr(-1.0));
+    EXPECT_EQ(123.45, mixxx::Bpm{}.valueOr(123.45));
+    EXPECT_EQ(mixxx::Bpm::kValueUndefined,
+            mixxx::Bpm{mixxx::Bpm::kValueMin}.valueOr(
+                    mixxx::Bpm::kValueUndefined));
+    EXPECT_DOUBLE_EQ(mixxx::Bpm::kValueMin + 0.001,
+            mixxx::Bpm{mixxx::Bpm::kValueMin + 0.001}.value());
+    EXPECT_EQ(mixxx::Bpm::kValueMax, mixxx::Bpm{mixxx::Bpm::kValueMax}.valueOr(100.0));
+    // The upper bound is only a soft-limit!
+    EXPECT_DOUBLE_EQ(mixxx::Bpm::kValueMax + 0.001,
+            mixxx::Bpm{mixxx::Bpm::kValueMax + 0.001}.valueOr(mixxx::Bpm::kValueMax));
+}
+
 TEST_F(BpmTest, TestBpmComparisonOperators) {
     EXPECT_EQ(mixxx::Bpm(120), mixxx::Bpm(120));
     EXPECT_EQ(mixxx::Bpm(120), mixxx::Bpm(60) * 2);

src/track/bpm.h

@@ -12,7 +12,7 @@ class Bpm final {
 public:
     static constexpr double kValueUndefined = 0.0;
     static constexpr double kValueMin = 0.0; // lower bound (exclusive)
-    static constexpr double kValueMax = 500.0; // higher bound (inclusive)
+    static constexpr double kValueMax = 500.0; // upper bound (inclusive)
 
     constexpr Bpm()
             : Bpm(kValueUndefined) {
@@ -44,12 +44,24 @@ class Bpm final {
     bool isValid() const {
         return isValidValue(m_value);
     }
+
+    /// Return the valid value.
+    ///
+    /// Triggers a debug assertion if the value is invalid
+    /// and returns kValueUndefined as a fallback.
     double value() const {
         VERIFY_OR_DEBUG_ASSERT(isValid()) {
             return kValueUndefined;
         }
         return m_value;
     }
+
+    /// Return either the valid BPM value or the given default value
+    /// if the BPM is invalid/undefined.
+    double valueOr(double defaultValue) const {
+        return isValid() ? m_value : defaultValue;
+    }
+
     void setValue(double value) {
         m_value = value;
     }