Merge pull request #11248 from JoergAtGithub/unify_beatjumpandloopsiz…

…e_rangecheck

Unified range checking behavior for BeatJumpSize and BeatLoopSize between GUI-Widget and controller CO

src/engine/controls/loopingcontrol.cpp

@@ -175,6 +175,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,
@@ -340,15 +343,15 @@ 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) {
     if (pressed <= 0.0) {
         return;
     }
 
-    slotBeatLoop(m_pCOBeatLoopSize->get() * 2.0, true, false);
+    m_pCOBeatLoopSize->set(m_pCOBeatLoopSize->get() * 2.0);
 }
 
 void LoopingControl::process(const double dRate,
@@ -1429,6 +1432,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 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;
+    }
+
     slotBeatLoop(beats, true, false);
 }
 
@@ -1495,6 +1506,19 @@ void LoopingControl::slotBeatJump(double beats) {
     }
 }
 
+void LoopingControl::slotBeatJumpSizeChangeRequest(double beats) {
+    // 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);
+}
+
 void LoopingControl::slotBeatJumpSizeHalve(double pressed) {
     if (pressed > 0) {
         m_pCOBeatJumpSize->set(m_pCOBeatJumpSize->get() / 2);

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);

src/test/looping_control_test.cpp

@@ -67,6 +67,10 @@ class LoopingControlTest : public MockedEngineBackendTest {
         m_pButtonBeatLoopActivate = std::make_unique<PollingControlProxy>(
                 m_sGroup1, "beatloop_activate");
         m_pBeatJumpSize = std::make_unique<PollingControlProxy>(m_sGroup1, "beatjump_size");
+        m_pButtonBeatJumpSizeDouble = std::make_unique<PollingControlProxy>(
+                m_sGroup1, "beatjump_size_double");
+        m_pButtonBeatJumpSizeHalve = std::make_unique<PollingControlProxy>(
+                m_sGroup1, "beatjump_size_halve");
         m_pButtonBeatJumpForward = std::make_unique<PollingControlProxy>(
                 m_sGroup1, "beatjump_forward");
         m_pButtonBeatJumpBackward = std::make_unique<PollingControlProxy>(
@@ -121,6 +125,8 @@ class LoopingControlTest : public MockedEngineBackendTest {
     std::unique_ptr<PollingControlProxy> m_pBeatLoopSize;
     std::unique_ptr<PollingControlProxy> m_pButtonBeatLoopActivate;
     std::unique_ptr<PollingControlProxy> m_pBeatJumpSize;
+    std::unique_ptr<PollingControlProxy> m_pButtonBeatJumpSizeHalve;
+    std::unique_ptr<PollingControlProxy> m_pButtonBeatJumpSizeDouble;
     std::unique_ptr<PollingControlProxy> m_pButtonBeatJumpForward;
     std::unique_ptr<PollingControlProxy> m_pButtonBeatJumpBackward;
     std::unique_ptr<PollingControlProxy> m_pButtonBeatLoopRoll1Activate;
@@ -712,6 +718,55 @@ TEST_F(LoopingControlTest, BeatLoopSize_IsSetByNumberedControl) {
     EXPECT_EQ(2.0, m_pBeatLoopSize->get());
 }
 
+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());
+}
+
 TEST_F(LoopingControlTest, BeatLoopSize_SetDoesNotStartLoop) {
     m_pTrack1->trySetBpm(120.0);
     m_pBeatLoopSize->set(16.0);
@@ -814,6 +869,55 @@ TEST_F(LoopingControlTest, LegacyBeatLoopControl) {
     EXPECT_EQ(6.0, m_pBeatLoopSize->get());
 }
 
+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());
+}
+
 TEST_F(LoopingControlTest, Beatjump_JumpsByBeats) {
     const auto bpm = mixxx::Bpm{120};
     m_pTrack1->trySetBpm(bpm);

src/widget/wbeatspinbox.cpp

@@ -54,25 +54,14 @@ 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;
     }
     // 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();
 }