From ffb3999a63a6db59b923325ee560ab66881d6501 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 4 Feb 2023 13:30:00 +0100 Subject: [PATCH 1/6] Unified range checking behavior for BeatJumpSize and for BeatLoopSize between GUI-Widget and controller CO, by moving the check from the widget to the engine. --- src/engine/controls/loopingcontrol.cpp | 22 ++++++++++++++++++++++ src/engine/controls/loopingcontrol.h | 1 + src/widget/wbeatspinbox.cpp | 7 +------ 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/engine/controls/loopingcontrol.cpp b/src/engine/controls/loopingcontrol.cpp index 9b052efb6f0..8217644ba0c 100644 --- a/src/engine/controls/loopingcontrol.cpp +++ b/src/engine/controls/loopingcontrol.cpp @@ -17,6 +17,9 @@ namespace { constexpr mixxx::audio::FrameDiff_t kMinimumAudibleLoopSizeFrames = 150; +constexpr double kMinBeatJumpSize = 0.03125; +constexpr double kMaxBeatJumpSize = 512; + // returns true if a is valid and is fairly close to target (within +/- 1 frame). bool positionNear(mixxx::audio::FramePos a, mixxx::audio::FramePos target) { return a.isValid() && a > target - 1 && a < target + 1; @@ -175,6 +178,9 @@ LoopingControl::LoopingControl(const QString& group, this, &LoopingControl::slotBeatJump, Qt::DirectConnection); m_pCOBeatJumpSize = new ControlObject(ConfigKey(group, "beatjump_size"), true, false, false, 4.0); + m_pCOBeatJumpSize->connectValueChangeRequest(this, + &LoopingControl::slotBeatJumpSizeChangeRequest, + Qt::DirectConnection); m_pCOBeatJumpSizeHalve = new ControlPushButton(ConfigKey(group, "beatjump_size_halve")); connect(m_pCOBeatJumpSizeHalve, @@ -1429,6 +1435,14 @@ void LoopingControl::slotBeatLoop(double beats, bool keepStartPoint, bool enable void LoopingControl::slotBeatLoopSizeChangeRequest(double beats) { // slotBeatLoop will call m_pCOBeatLoopSize->setAndConfirm if // new beatloop_size is valid + + double maxBeatSize = s_dBeatSizes[sizeof(s_dBeatSizes) / sizeof(s_dBeatSizes[0]) - 1]; + double minBeatSize = s_dBeatSizes[0]; + if ((beats < minBeatSize) || (beats > maxBeatSize)) { + // Don't clamp the value here to not fall out of a measure + return; + } + slotBeatLoop(beats, true, false); } @@ -1495,6 +1509,14 @@ void LoopingControl::slotBeatJump(double beats) { } } +void LoopingControl::slotBeatJumpSizeChangeRequest(double beats) { + if ((beats < kMinBeatJumpSize) || (beats > kMaxBeatJumpSize)) { + // Don't clamp the value here to not fall out of a measure + return; + } + m_pCOBeatJumpSize->setAndConfirm(beats); +} + void LoopingControl::slotBeatJumpSizeHalve(double pressed) { if (pressed > 0) { m_pCOBeatJumpSize->set(m_pCOBeatJumpSize->get() / 2); diff --git a/src/engine/controls/loopingcontrol.h b/src/engine/controls/loopingcontrol.h index 3f3eebdf7ee..7aa7cebf8a0 100644 --- a/src/engine/controls/loopingcontrol.h +++ b/src/engine/controls/loopingcontrol.h @@ -87,6 +87,7 @@ class LoopingControl : public EngineControl { // Jump forward or backward by beats. void slotBeatJump(double beats); + void slotBeatJumpSizeChangeRequest(double beats); void slotBeatJumpSizeHalve(double pressed); void slotBeatJumpSizeDouble(double pressed); void slotBeatJumpForward(double pressed); diff --git a/src/widget/wbeatspinbox.cpp b/src/widget/wbeatspinbox.cpp index c41dc7da094..663bab35fd9 100644 --- a/src/widget/wbeatspinbox.cpp +++ b/src/widget/wbeatspinbox.cpp @@ -54,12 +54,7 @@ void WBeatSpinBox::stepBy(int steps) { QString temp = text(); int cursorPos = lineEdit()->cursorPosition(); if (validate(temp, cursorPos) == QValidator::Acceptable) { - double editValue = valueFromText(temp); - newValue = editValue * pow(2, steps); - if (newValue < minimum() || newValue > maximum()) { - // don't clamp the value here to not fall out of a measure - newValue = editValue; - } + newValue = valueFromText(temp) * pow(2, steps); } else { // here we have an unacceptable edit, going back to the old value first newValue = oldValue; From 8f35f9bf3b788d85473ac0be6506399b2b45dece Mon Sep 17 00:00:00 2001 From: Joerg Date: Sun, 5 Feb 2023 00:32:18 +0100 Subject: [PATCH 2/6] Fixed slotLoopHalve/Double --- src/engine/controls/loopingcontrol.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/engine/controls/loopingcontrol.cpp b/src/engine/controls/loopingcontrol.cpp index 8217644ba0c..29a227aa860 100644 --- a/src/engine/controls/loopingcontrol.cpp +++ b/src/engine/controls/loopingcontrol.cpp @@ -346,7 +346,7 @@ void LoopingControl::slotLoopHalve(double pressed) { return; } - slotBeatLoop(m_pCOBeatLoopSize->get() / 2.0, true, false); + m_pCOBeatLoopSize->set(m_pCOBeatLoopSize->get() / 2.0); } void LoopingControl::slotLoopDouble(double pressed) { @@ -354,7 +354,7 @@ void LoopingControl::slotLoopDouble(double pressed) { return; } - slotBeatLoop(m_pCOBeatLoopSize->get() * 2.0, true, false); + m_pCOBeatLoopSize->set(m_pCOBeatLoopSize->get() * 2.0); } void LoopingControl::process(const double dRate, From 28d705d22923d7b766964cb887b7ccfcb1ab78f8 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sun, 5 Feb 2023 00:35:19 +0100 Subject: [PATCH 3/6] Added test cases for the loopsize / beatjumpsize range check --- src/test/looping_control_test.cpp | 84 +++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/src/test/looping_control_test.cpp b/src/test/looping_control_test.cpp index ef05bc79b1f..3b53f9cf157 100644 --- a/src/test/looping_control_test.cpp +++ b/src/test/looping_control_test.cpp @@ -67,6 +67,10 @@ class LoopingControlTest : public MockedEngineBackendTest { m_pButtonBeatLoopActivate = std::make_unique( m_sGroup1, "beatloop_activate"); m_pBeatJumpSize = std::make_unique(m_sGroup1, "beatjump_size"); + m_pButtonBeatJumpSizeDouble = std::make_unique( + m_sGroup1, "beatjump_size_double"); + m_pButtonBeatJumpSizeHalve = std::make_unique( + m_sGroup1, "beatjump_size_halve"); m_pButtonBeatJumpForward = std::make_unique( m_sGroup1, "beatjump_forward"); m_pButtonBeatJumpBackward = std::make_unique( @@ -121,6 +125,8 @@ class LoopingControlTest : public MockedEngineBackendTest { std::unique_ptr m_pBeatLoopSize; std::unique_ptr m_pButtonBeatLoopActivate; std::unique_ptr m_pBeatJumpSize; + std::unique_ptr m_pButtonBeatJumpSizeHalve; + std::unique_ptr m_pButtonBeatJumpSizeDouble; std::unique_ptr m_pButtonBeatJumpForward; std::unique_ptr m_pButtonBeatJumpBackward; std::unique_ptr m_pButtonBeatLoopRoll1Activate; @@ -712,6 +718,45 @@ TEST_F(LoopingControlTest, BeatLoopSize_IsSetByNumberedControl) { EXPECT_EQ(2.0, m_pBeatLoopSize->get()); } +TEST_F(LoopingControlTest, BeatLoopSize_SetRangeCheck) { + m_pBeatLoopSize->set(512.0); + EXPECT_EQ(512, m_pBeatLoopSize->get()); + + m_pBeatLoopSize->set(150.0); + EXPECT_EQ(150, m_pBeatLoopSize->get()); + + m_pBeatLoopSize->set(513.0); + EXPECT_EQ(150, m_pBeatLoopSize->get()); + + m_pButtonLoopDouble->set(1.0); + m_pButtonLoopDouble->set(0.0); + EXPECT_EQ(300.0, m_pBeatLoopSize->get()); + + m_pButtonLoopDouble->set(1.0); + m_pButtonLoopDouble->set(0.0); + EXPECT_EQ(300.0, m_pBeatLoopSize->get()); + + m_pBeatLoopSize->set(1 / 32.0); + EXPECT_EQ(1 / 32.0, m_pBeatLoopSize->get()); + + m_pBeatLoopSize->set(1 / 10.0); + EXPECT_EQ(1 / 10.0, m_pBeatLoopSize->get()); + + m_pBeatLoopSize->set(1 / 33.0); + EXPECT_EQ(1 / 10.0, m_pBeatLoopSize->get()); + + m_pBeatLoopSize->set(0); + EXPECT_EQ(1 / 10.0, m_pBeatLoopSize->get()); + + m_pButtonLoopHalve->set(1.0); + m_pButtonLoopHalve->set(0.0); + EXPECT_EQ(1 / 20.0, m_pBeatLoopSize->get()); + + m_pButtonLoopHalve->set(1.0); + m_pButtonLoopHalve->set(0.0); + EXPECT_EQ(1 / 20.0, m_pBeatLoopSize->get()); +} + TEST_F(LoopingControlTest, BeatLoopSize_SetDoesNotStartLoop) { m_pTrack1->trySetBpm(120.0); m_pBeatLoopSize->set(16.0); @@ -814,6 +859,45 @@ TEST_F(LoopingControlTest, LegacyBeatLoopControl) { EXPECT_EQ(6.0, m_pBeatLoopSize->get()); } +TEST_F(LoopingControlTest, BeatjumpSize_SetRangeCheck) { + m_pBeatJumpSize->set(512.0); + EXPECT_EQ(512, m_pBeatJumpSize->get()); + + m_pBeatJumpSize->set(150.0); + EXPECT_EQ(150, m_pBeatJumpSize->get()); + + m_pBeatJumpSize->set(513.0); + EXPECT_EQ(150, m_pBeatJumpSize->get()); + + m_pButtonBeatJumpSizeDouble->set(1.0); + m_pButtonBeatJumpSizeDouble->set(0.0); + EXPECT_EQ(300.0, m_pBeatJumpSize->get()); + + m_pButtonBeatJumpSizeDouble->set(1.0); + m_pButtonBeatJumpSizeDouble->set(0.0); + EXPECT_EQ(300.0, m_pBeatJumpSize->get()); + + m_pBeatJumpSize->set(1 / 32.0); + EXPECT_EQ(1 / 32.0, m_pBeatJumpSize->get()); + + m_pBeatJumpSize->set(1 / 10.0); + EXPECT_EQ(1 / 10.0, m_pBeatJumpSize->get()); + + m_pBeatJumpSize->set(1 / 33.0); + EXPECT_EQ(1 / 10.0, m_pBeatJumpSize->get()); + + m_pBeatJumpSize->set(0); + EXPECT_EQ(1 / 10.0, m_pBeatJumpSize->get()); + + m_pButtonBeatJumpSizeHalve->set(1.0); + m_pButtonBeatJumpSizeHalve->set(0.0); + EXPECT_EQ(1 / 20.0, m_pBeatJumpSize->get()); + + m_pButtonBeatJumpSizeHalve->set(1.0); + m_pButtonBeatJumpSizeHalve->set(0.0); + EXPECT_EQ(1 / 20.0, m_pBeatJumpSize->get()); +} + TEST_F(LoopingControlTest, Beatjump_JumpsByBeats) { const auto bpm = mixxx::Bpm{120}; m_pTrack1->trySetBpm(bpm); From b84fdacbdb3f0d73a6287378134c6649a32fd993 Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 18 Feb 2023 15:15:28 +0100 Subject: [PATCH 4/6] Use same limit source for beatJumpSize as for beaLoopSize --- src/engine/controls/loopingcontrol.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/engine/controls/loopingcontrol.cpp b/src/engine/controls/loopingcontrol.cpp index 29a227aa860..dbbfc539015 100644 --- a/src/engine/controls/loopingcontrol.cpp +++ b/src/engine/controls/loopingcontrol.cpp @@ -17,9 +17,6 @@ namespace { constexpr mixxx::audio::FrameDiff_t kMinimumAudibleLoopSizeFrames = 150; -constexpr double kMinBeatJumpSize = 0.03125; -constexpr double kMaxBeatJumpSize = 512; - // returns true if a is valid and is fairly close to target (within +/- 1 frame). bool positionNear(mixxx::audio::FramePos a, mixxx::audio::FramePos target) { return a.isValid() && a > target - 1 && a < target + 1; @@ -1436,9 +1433,9 @@ void LoopingControl::slotBeatLoopSizeChangeRequest(double beats) { // slotBeatLoop will call m_pCOBeatLoopSize->setAndConfirm if // new beatloop_size is valid - double maxBeatSize = s_dBeatSizes[sizeof(s_dBeatSizes) / sizeof(s_dBeatSizes[0]) - 1]; - double minBeatSize = s_dBeatSizes[0]; - if ((beats < minBeatSize) || (beats > maxBeatSize)) { + double maxBeatLoopSize = s_dBeatSizes[sizeof(s_dBeatSizes) / sizeof(s_dBeatSizes[0]) - 1]; + double minBeatLoopSize = s_dBeatSizes[0]; + if ((beats < minBeatLoopSize) || (beats > maxBeatLoopSize)) { // Don't clamp the value here to not fall out of a measure return; } @@ -1510,10 +1507,15 @@ void LoopingControl::slotBeatJump(double beats) { } void LoopingControl::slotBeatJumpSizeChangeRequest(double beats) { - if ((beats < kMinBeatJumpSize) || (beats > kMaxBeatJumpSize)) { + // Use same limits as for beat loop size + double maxBeatJumpSize = s_dBeatSizes[sizeof(s_dBeatSizes) / sizeof(s_dBeatSizes[0]) - 1]; + double minBeatJumpSize = s_dBeatSizes[0]; + + if ((beats < minBeatJumpSize) || (beats > maxBeatJumpSize)) { // Don't clamp the value here to not fall out of a measure return; } + m_pCOBeatJumpSize->setAndConfirm(beats); } From 20edf5eaa5a5fb2b54b6ced6d4e13507793bc3fa Mon Sep 17 00:00:00 2001 From: Joerg Date: Sat, 18 Feb 2023 15:52:50 +0100 Subject: [PATCH 5/6] Added comments which describe the test steps and limits. --- src/test/looping_control_test.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/test/looping_control_test.cpp b/src/test/looping_control_test.cpp index 3b53f9cf157..ec6668aa969 100644 --- a/src/test/looping_control_test.cpp +++ b/src/test/looping_control_test.cpp @@ -719,39 +719,49 @@ TEST_F(LoopingControlTest, BeatLoopSize_IsSetByNumberedControl) { } TEST_F(LoopingControlTest, BeatLoopSize_SetRangeCheck) { + // Set BeatLoopSize to the maximum allowed value of 512 m_pBeatLoopSize->set(512.0); EXPECT_EQ(512, m_pBeatLoopSize->get()); m_pBeatLoopSize->set(150.0); EXPECT_EQ(150, m_pBeatLoopSize->get()); + // Set BeatLoopSize to a value above the allowed maximum of 512 -> This must be ignored m_pBeatLoopSize->set(513.0); EXPECT_EQ(150, m_pBeatLoopSize->get()); + // Double BeatLoopSize (the result is 300 which is in the allowed range) m_pButtonLoopDouble->set(1.0); m_pButtonLoopDouble->set(0.0); EXPECT_EQ(300.0, m_pBeatLoopSize->get()); + // Double BeatLoopSize (the result would be 600 which is above the allowed + // maximum of 512 -> This must be ignored) m_pButtonLoopDouble->set(1.0); m_pButtonLoopDouble->set(0.0); EXPECT_EQ(300.0, m_pBeatLoopSize->get()); + // Set BeatLoopSize to the minimum allowed value m_pBeatLoopSize->set(1 / 32.0); EXPECT_EQ(1 / 32.0, m_pBeatLoopSize->get()); m_pBeatLoopSize->set(1 / 10.0); EXPECT_EQ(1 / 10.0, m_pBeatLoopSize->get()); + // Set BeatLoopSize to a value below the allowed minimum of 1/32 -> This must be ignored m_pBeatLoopSize->set(1 / 33.0); EXPECT_EQ(1 / 10.0, m_pBeatLoopSize->get()); m_pBeatLoopSize->set(0); EXPECT_EQ(1 / 10.0, m_pBeatLoopSize->get()); + // Halve BeatLoopSize (the result is 1/20 which is in the allowed range) m_pButtonLoopHalve->set(1.0); m_pButtonLoopHalve->set(0.0); EXPECT_EQ(1 / 20.0, m_pBeatLoopSize->get()); + // Halve BeatLoopSize (the result would be 1/40 which is below the allowed + // minimum of 1/32 -> This must be ignored) m_pButtonLoopHalve->set(1.0); m_pButtonLoopHalve->set(0.0); EXPECT_EQ(1 / 20.0, m_pBeatLoopSize->get()); @@ -860,39 +870,49 @@ TEST_F(LoopingControlTest, LegacyBeatLoopControl) { } TEST_F(LoopingControlTest, BeatjumpSize_SetRangeCheck) { + // Set BeatJumpSize to the maximum allowed value m_pBeatJumpSize->set(512.0); EXPECT_EQ(512, m_pBeatJumpSize->get()); m_pBeatJumpSize->set(150.0); EXPECT_EQ(150, m_pBeatJumpSize->get()); + // Set BeatJumpSize to a value above the allowed maximum of 512 -> This must be ignored m_pBeatJumpSize->set(513.0); EXPECT_EQ(150, m_pBeatJumpSize->get()); + // Double BeatJumpSize (the result is 300 which is in the allowed range) m_pButtonBeatJumpSizeDouble->set(1.0); m_pButtonBeatJumpSizeDouble->set(0.0); EXPECT_EQ(300.0, m_pBeatJumpSize->get()); + // Double BeatJumpSize (the result would be 600 which is above the allowed + // maximum of 512-> This must be ignored) m_pButtonBeatJumpSizeDouble->set(1.0); m_pButtonBeatJumpSizeDouble->set(0.0); EXPECT_EQ(300.0, m_pBeatJumpSize->get()); + // Set BeatJumpSize to the minimum allowed value m_pBeatJumpSize->set(1 / 32.0); EXPECT_EQ(1 / 32.0, m_pBeatJumpSize->get()); m_pBeatJumpSize->set(1 / 10.0); EXPECT_EQ(1 / 10.0, m_pBeatJumpSize->get()); + // Set BeatJumpSize to a value below the allowed minimum of 1/32 -> This must be ignored m_pBeatJumpSize->set(1 / 33.0); EXPECT_EQ(1 / 10.0, m_pBeatJumpSize->get()); m_pBeatJumpSize->set(0); EXPECT_EQ(1 / 10.0, m_pBeatJumpSize->get()); + // Halve BeatJumpSize (the result is 1/20 which is in the allowed range) m_pButtonBeatJumpSizeHalve->set(1.0); m_pButtonBeatJumpSizeHalve->set(0.0); EXPECT_EQ(1 / 20.0, m_pBeatJumpSize->get()); + // Halve BeatJumpSize (the result would be 1/40 which is below the allowed + // minimum of 1/32 -> This must be ignored) m_pButtonBeatJumpSizeHalve->set(1.0); m_pButtonBeatJumpSizeHalve->set(0.0); EXPECT_EQ(1 / 20.0, m_pBeatJumpSize->get()); From a325712cfa4bcf0138c9023807c747a74a968a5d Mon Sep 17 00:00:00 2001 From: Joerg Date: Thu, 23 Mar 2023 01:00:02 +0100 Subject: [PATCH 6/6] Removed uneeded workaround, for applying value in WBeatSpinBox::stepBy in two steps --- src/widget/wbeatspinbox.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/widget/wbeatspinbox.cpp b/src/widget/wbeatspinbox.cpp index 663bab35fd9..9e8986a8130 100644 --- a/src/widget/wbeatspinbox.cpp +++ b/src/widget/wbeatspinbox.cpp @@ -61,13 +61,7 @@ void WBeatSpinBox::stepBy(int steps) { } // Do not call QDoubleSpinBox::setValue directly in case // the new value of the ControlObject needs to be confirmed. - // Curiously, m_valueControl.set() does not cause slotControlValueChanged - // to execute for beatjump_size, so call QDoubleSpinBox::setValue in this function. m_valueControl.set(newValue); - double coValue = m_valueControl.get(); - if (coValue != value()) { - setValue(coValue); - } selectAll(); }