diff --git a/src/library/basetracktablemodel.cpp b/src/library/basetracktablemodel.cpp index 8e11f0e4fd29..b050bc086ec0 100644 --- a/src/library/basetracktablemodel.cpp +++ b/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( diff --git a/src/library/dlgtrackinfo.cpp b/src/library/dlgtrackinfo.cpp index 6b011c73511e..e314bef50c43 100644 --- a/src/library/dlgtrackinfo.cpp +++ b/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() { diff --git a/src/library/dlgtrackinfo.h b/src/library/dlgtrackinfo.h index fdb7ccf7905b..2cb9d0101103 100644 --- a/src/library/dlgtrackinfo.h +++ b/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; diff --git a/src/test/bpmtest.cpp b/src/test/bpmtest.cpp index 6152bf5fecbf..1ea4f3bf5698 100644 --- a/src/test/bpmtest.cpp +++ b/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); diff --git a/src/track/bpm.h b/src/track/bpm.h index 93800ce2b2ee..5c4f7698b494 100644 --- a/src/track/bpm.h +++ b/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; }