From bd65c358880cdde2f32d5e4e62ab36e8e4d57eb8 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 1 Aug 2021 13:11:50 +0200 Subject: [PATCH 1/3] Add mixxx::Bpm::valueOrUndefined() --- src/test/bpmtest.cpp | 41 +++++++++++++++++++++++++++++++++++++++++ src/track/bpm.h | 13 ++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/test/bpmtest.cpp b/src/test/bpmtest.cpp index 6152bf5fecbf..c5e8c3d4ccc6 100644 --- a/src/test/bpmtest.cpp +++ b/src/test/bpmtest.cpp @@ -7,6 +7,47 @@ 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, valueOrUndefined) { + EXPECT_DOUBLE_EQ(123.45, mixxx::Bpm{123.45}.valueOrUndefined()); + EXPECT_EQ(mixxx::Bpm::kValueUndefined, mixxx::Bpm{-123.45}.valueOrUndefined()); + EXPECT_EQ(mixxx::Bpm::kValueUndefined, mixxx::Bpm{}.valueOrUndefined()); + EXPECT_EQ(mixxx::Bpm::kValueUndefined, mixxx::Bpm{mixxx::Bpm::kValueMin}.valueOrUndefined()); + 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}.valueOrUndefined()); + // The upper bound is only a soft-limit! + EXPECT_DOUBLE_EQ(mixxx::Bpm::kValueMax + 0.001, + mixxx::Bpm{mixxx::Bpm::kValueMax + 0.001}.valueOrUndefined()); +} + 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..3daf0dadd12e 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,23 @@ 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 the valid value or kValueUndefined if invalid. + double valueOrUndefined() const { + return isValid() ? m_value : kValueUndefined; + } + void setValue(double value) { m_value = value; } From 978a510075e64b39d8ec28798fc2aa7feb20235a Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 1 Aug 2021 13:13:27 +0200 Subject: [PATCH 2/3] Handle invalid/undefined BPM values --- src/library/basetracktablemodel.cpp | 6 ++- src/library/dlgtrackinfo.cpp | 59 ++++++++++------------------- src/library/dlgtrackinfo.h | 2 + 3 files changed, 27 insertions(+), 40 deletions(-) diff --git a/src/library/basetracktablemodel.cpp b/src/library/basetracktablemodel.cpp index 8e11f0e4fd29..dfae5dbab03e 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}.valueOrUndefined(); } case ColumnCache::COLUMN_LIBRARYTABLE_TIMESPLAYED: return index.sibling( diff --git a/src/library/dlgtrackinfo.cpp b/src/library/dlgtrackinfo.cpp index 6b011c73511e..248c443f2f63 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().valueOrUndefined() + : 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.valueOrUndefined()); } } @@ -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; From 36fc53cc5498e80a9bf97f601fbbc23866c21de4 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 5 Aug 2021 14:06:32 +0200 Subject: [PATCH 3/3] Provide more versatile Bpm::valueOr() with custom default value --- src/library/basetracktablemodel.cpp | 2 +- src/library/dlgtrackinfo.cpp | 4 ++-- src/test/bpmtest.cpp | 16 +++++++++------- src/track/bpm.h | 7 ++++--- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/library/basetracktablemodel.cpp b/src/library/basetracktablemodel.cpp index dfae5dbab03e..b050bc086ec0 100644 --- a/src/library/basetracktablemodel.cpp +++ b/src/library/basetracktablemodel.cpp @@ -785,7 +785,7 @@ QVariant BaseTrackTableModel::roleValue( if (!ok) { return mixxx::Bpm::kValueUndefined; } - return mixxx::Bpm{bpmValue}.valueOrUndefined(); + 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 248c443f2f63..e314bef50c43 100644 --- a/src/library/dlgtrackinfo.cpp +++ b/src/library/dlgtrackinfo.cpp @@ -368,7 +368,7 @@ void DlgTrackInfo::updateTrackMetadataFields() { void DlgTrackInfo::updateSpinBpmFromBeats() { const auto bpmValue = m_pBeatsClone - ? m_pBeatsClone->getBpm().valueOrUndefined() + ? m_pBeatsClone->getBpm().valueOr(mixxx::Bpm::kValueUndefined) : mixxx::Bpm::kValueUndefined; spinBpm->setValue(bpmValue); } @@ -598,7 +598,7 @@ void DlgTrackInfo::slotBpmTap(double averageLength, int numSamples) { averageBpm + kBpmTabRounding); if (averageBpm != m_lastTapedBpm) { m_lastTapedBpm = averageBpm; - spinBpm->setValue(averageBpm.valueOrUndefined()); + spinBpm->setValue(averageBpm.valueOr(mixxx::Bpm::kValueUndefined)); } } diff --git a/src/test/bpmtest.cpp b/src/test/bpmtest.cpp index c5e8c3d4ccc6..1ea4f3bf5698 100644 --- a/src/test/bpmtest.cpp +++ b/src/test/bpmtest.cpp @@ -35,17 +35,19 @@ TEST_F(BpmTest, value) { mixxx::Bpm{mixxx::Bpm::kValueMax + 0.001}.value()); } -TEST_F(BpmTest, valueOrUndefined) { - EXPECT_DOUBLE_EQ(123.45, mixxx::Bpm{123.45}.valueOrUndefined()); - EXPECT_EQ(mixxx::Bpm::kValueUndefined, mixxx::Bpm{-123.45}.valueOrUndefined()); - EXPECT_EQ(mixxx::Bpm::kValueUndefined, mixxx::Bpm{}.valueOrUndefined()); - EXPECT_EQ(mixxx::Bpm::kValueUndefined, mixxx::Bpm{mixxx::Bpm::kValueMin}.valueOrUndefined()); +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}.valueOrUndefined()); + 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}.valueOrUndefined()); + mixxx::Bpm{mixxx::Bpm::kValueMax + 0.001}.valueOr(mixxx::Bpm::kValueMax)); } TEST_F(BpmTest, TestBpmComparisonOperators) { diff --git a/src/track/bpm.h b/src/track/bpm.h index 3daf0dadd12e..5c4f7698b494 100644 --- a/src/track/bpm.h +++ b/src/track/bpm.h @@ -56,9 +56,10 @@ class Bpm final { return m_value; } - /// Return the valid value or kValueUndefined if invalid. - double valueOrUndefined() const { - return isValid() ? m_value : kValueUndefined; + /// 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) {