fix: allow controller setting integer to have a negative range #13656

acolombier · 2024-09-15T15:32:56Z

No description provided.

acolombier · 2024-09-15T15:33:56Z

src/controllers/legacycontrollersettings.h

        return AbstractLegacyControllerSetting::valid() &&
                m_defaultValue >= m_minValue && m_savedValue >= m_minValue &&
                m_editedValue >= m_minValue && m_defaultValue <= m_maxValue &&
                m_savedValue <= m_maxValue && m_editedValue <= m_maxValue &&
-                m_stepValue > 0 && m_stepValue < m_maxValue;
+                m_stepValue > 0 &&
+                (typeSize ||


To be confirmed, but I read that some platform use the same byte size for int and long and since the default min and max uses the int limits, without a larger type, this would lead to an overflow.

The name of the temp variable sounds like a size itself. Since long is different on Linux and Windows we try to not use it at all. What is the reason that it is used here?

In case of an int a step value can't be bigger than the half of the range. If we assume 8 bit, we may have
-128 ... 127 and the maximum Step that can be expressed is 127.

So you may check:
m_minValue <= 0 && m_minValue + m_stepValue <= m_maxValue &&
m_maxValue >= 0 && m_maxValue - m_stepValue >= sm_minValue

And m_maxValue - m_stepValue < m_maxValue for the unsigned case ...

and put it in a utility function please. The long condition chain is already not super easy to follow.

And m_maxValue - m_stepValue < m_maxValue for the unsigned case ...

Not sure which usecase you are referring to?

Swiftb0y · 2024-09-15T17:54:10Z

What is the usecase for this? If I understand LegacyControllerNumberSetting correctly, its just modeling a closed integer interval $[ a \ldotp \ldotp b ]$ (where $a \leq b$), right? What is the purpose of allowing $a \ge b$ too?

acolombier · 2024-09-15T19:32:57Z

This doesn't allow a a > b, it allows the step to be less than the max. My usecase was the following: I have a setting that can be in the range [-250; 0] and the step is 1. Currently, this would fall. With this changes, instead of doing making sure the step is less than the max, it makes sure the the step is less than the range side (250 in my case)

daschuer · 2024-09-16T05:31:12Z

src/controllers/legacycontrollersettings.h

        return AbstractLegacyControllerSetting::valid() &&
                m_defaultValue >= m_minValue && m_savedValue >= m_minValue &&
                m_editedValue >= m_minValue && m_defaultValue <= m_maxValue &&
                m_savedValue <= m_maxValue && m_editedValue <= m_maxValue &&
-                m_stepValue > 0 && m_stepValue < m_maxValue;
+                m_stepValue > 0 &&
+                (typeSize ||


The name of the temp variable sounds like a size itself. Since long is different on Linux and Windows we try to not use it at all. What is the reason that it is used here?

daschuer · 2024-09-16T06:21:50Z

src/controllers/legacycontrollersettings.h

        return AbstractLegacyControllerSetting::valid() &&
                m_defaultValue >= m_minValue && m_savedValue >= m_minValue &&
                m_editedValue >= m_minValue && m_defaultValue <= m_maxValue &&
                m_savedValue <= m_maxValue && m_editedValue <= m_maxValue &&
-                m_stepValue > 0 && m_stepValue < m_maxValue;
+                m_stepValue > 0 &&
+                (typeSize ||


In case of an int a step value can't be bigger than the half of the range. If we assume 8 bit, we may have
-128 ... 127 and the maximum Step that can be expressed is 127.

So you may check:
m_minValue <= 0 && m_minValue + m_stepValue <= m_maxValue &&
m_maxValue >= 0 && m_maxValue - m_stepValue >= sm_minValue

Swiftb0y · 2024-09-16T09:03:57Z

This doesn't allow a a > b, it allows the step to be less than the max. My usecase was the following: I have a setting that can be in the range [-250; 0] and the step is 1. Currently, this would fall. With this changes, instead of doing making sure the step is less than the max, it makes sure the the step is less than the range side (250 in my case)

ah, yeah. That makes total sense. Thanks for the explanation.

src/controllers/legacycontrollersettings.h

Swiftb0y

lgtm otherwise.

src/controllers/legacycontrollersettings.h

daschuer · 2024-09-17T05:53:51Z

src/controllers/legacycontrollersettings.h

@@ -263,7 +271,7 @@ class LegacyControllerNumberSetting
                m_defaultValue >= m_minValue && m_savedValue >= m_minValue &&
                m_editedValue >= m_minValue && m_defaultValue <= m_maxValue &&
                m_savedValue <= m_maxValue && m_editedValue <= m_maxValue &&
-                m_stepValue > 0 && m_stepValue < m_maxValue;
+                m_stepValue > 0 && valid_range(m_minValue, m_maxValue, m_stepValue);


We may face the condition that the default "default" = 0 in case of a missing attribute is not valid. Do we want to improve that as well for the negative range?

Added the usecase at the de-serialisation step.

Swiftb0y · 2024-09-19T13:12:00Z

src/controllers/legacycontrollersettings.h

+    VERIFY_OR_DEBUG_ASSERT(step >= 0) {
+        return false;
+    }


do we actually ensure this somewhere else (and provide appropriate debug feedback)? I'm not particularly familiar with the code.

Yes and no. If a setting definition is invalid, you will get a warning log message telling you about it, no actual UI feedback on the matter.

At the time of implementation, the assumption was made that mapping developer would be able to gather log if they needed to

debug message is fine, does it specify what exactly is invalid?

No, it just says it will ignore a definition due to an invalid settings

thats unfortunate. we should fix that in the future. Just saying something is invalid gets increasingly frustrating the more complex the definition is. how would I know which of my (hypothetical) 10 parameters are invalid? Out-of-scope here though. Just suggestion.

You would know which settings is invalid, but it wouldn't details what's wrong with it (e.g Default out of range? Min after max?)

The warning for that is here

Let's say you have 10 setitings (my setting1..mySetting10), and mySetting6 is invalid, you would get something like

Warning: The parsed setting in file "/home/me/myfile.hid.xml" at line 42 appears to be invalid. It will be ignored.

yeah, thats not great once a setting has a bunch of parameters (along with non-obvious constraints). Ideally the parser should be able to report validation failures as they happen.

I might look into that for the future. Happy to get this bugfix in the meantime?

daschuer

LGTM, thank you.

Swiftb0y

LGTM. Thank you.

fix: allow controller setting integer to have a negative range

4803dc1

github-actions bot added code quality controller backend labels Sep 15, 2024

acolombier commented Sep 15, 2024

View reviewed changes

daschuer requested changes Sep 16, 2024

View reviewed changes

Refactor setting check logic

b050325

acolombier force-pushed the fix/controller-setting-integer-negative-range branch from 9aabb13 to b050325 Compare September 16, 2024 11:05

Swiftb0y requested changes Sep 16, 2024

View reviewed changes

src/controllers/legacycontrollersettings.h Outdated Show resolved Hide resolved

Removing irrelevant assertion

27f0752

acolombier requested review from Swiftb0y and daschuer September 16, 2024 16:05

Swiftb0y requested changes Sep 16, 2024

View reviewed changes

src/controllers/legacycontrollersettings.h Outdated Show resolved Hide resolved

daschuer reviewed Sep 17, 2024

View reviewed changes

acolombier requested review from Swiftb0y and daschuer September 19, 2024 12:07

Fix invalid assert and add default out of range usecase

9cf405f

acolombier force-pushed the fix/controller-setting-integer-negative-range branch from 2326ded to 9cf405f Compare September 19, 2024 12:36

Swiftb0y reviewed Sep 19, 2024

View reviewed changes

daschuer approved these changes Sep 19, 2024

View reviewed changes

Swiftb0y approved these changes Sep 20, 2024

View reviewed changes

Swiftb0y merged commit 5b895b8 into mixxxdj:2.5 Sep 20, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow controller setting integer to have a negative range #13656

fix: allow controller setting integer to have a negative range #13656

acolombier commented Sep 15, 2024

acolombier Sep 15, 2024

daschuer Sep 16, 2024

daschuer Sep 16, 2024

daschuer Sep 16, 2024

Swiftb0y Sep 16, 2024

acolombier Sep 16, 2024

Swiftb0y commented Sep 15, 2024

acolombier commented Sep 15, 2024

daschuer Sep 16, 2024

daschuer Sep 16, 2024

Swiftb0y commented Sep 16, 2024

Swiftb0y left a comment

daschuer Sep 17, 2024

acolombier Sep 19, 2024

Swiftb0y Sep 19, 2024

acolombier Sep 19, 2024

Swiftb0y Sep 19, 2024

acolombier Sep 19, 2024

Swiftb0y Sep 19, 2024

acolombier Sep 19, 2024 •

edited

Loading

Swiftb0y Sep 20, 2024

acolombier Sep 20, 2024

Swiftb0y Sep 20, 2024

daschuer left a comment

Swiftb0y left a comment

fix: allow controller setting integer to have a negative range #13656

fix: allow controller setting integer to have a negative range #13656

Conversation

acolombier commented Sep 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Sep 15, 2024

acolombier commented Sep 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Sep 16, 2024

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolombier Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Swiftb0y left a comment

Choose a reason for hiding this comment

acolombier Sep 19, 2024 •

edited

Loading