From 4803dc1f6f2099ed2f0271306c8f18d31169c84a Mon Sep 17 00:00:00 2001 From: Antoine C Date: Sun, 15 Sep 2024 16:22:16 +0100 Subject: [PATCH 1/4] fix: allow controller setting integer to have a negative range --- src/controllers/legacycontrollersettings.h | 7 +++- src/test/controller_mapping_settings_test.cpp | 35 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/controllers/legacycontrollersettings.h b/src/controllers/legacycontrollersettings.h index b7d2f378d9c..a259da5719c 100644 --- a/src/controllers/legacycontrollersettings.h +++ b/src/controllers/legacycontrollersettings.h @@ -259,11 +259,16 @@ class LegacyControllerNumberSetting /// and a strictly positive step, strictly less than max.. /// @return true if valid bool valid() const override { + static constexpr bool typeSize = sizeof(long) == sizeof(int); 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 || + static_cast(m_stepValue) < + static_cast(m_maxValue) - + static_cast(m_minValue)); } static AbstractLegacyControllerSetting* createFrom(const QDomElement& element) { diff --git a/src/test/controller_mapping_settings_test.cpp b/src/test/controller_mapping_settings_test.cpp index 3719fcefaf4..9689f5a6c27 100644 --- a/src/test/controller_mapping_settings_test.cpp +++ b/src/test/controller_mapping_settings_test.cpp @@ -511,3 +511,38 @@ TEST_F(LegacyControllerMappingSettingsTest, discardDuplicateSettings) { ASSERT_EQ(settings.at(1)->value().toNumber(), 50); ASSERT_EQ(settings.at(2)->value().toString(), "myOptionValue1"); } + +TEST_F(LegacyControllerMappingSettingsTest, handleNumberWithNegativeRange) { + QDomDocument doc; + QString dom; + QTextStream(&dom) + << QString(kValidInteger).arg("0", "-500", "0", "1"); + doc.setContent( + QString("%1") + .arg(dom)); + + auto pMapping = LegacyDummyMappingFileHandler::loadDummyMapping( + doc.documentElement(), "/fake/path"); + + ASSERT_EQ(pMapping->getSettings().size(), 1); + ASSERT_EQ(pMapping->getSettings().at(0)->variableName(), "myInteger1"); + ASSERT_EQ(pMapping->getSettings().at(0)->value().toNumber(), 0); + + dom.clear(); + QTextStream(&dom) + << QString(kValidInteger).arg("20", "-100", "100", "50"); + doc.setContent( + QString("%1") + .arg(dom)); + + pMapping = LegacyDummyMappingFileHandler::loadDummyMapping( + doc.documentElement(), "/fake/path"); + + ASSERT_EQ(pMapping->getSettings().size(), 1); + ASSERT_EQ(pMapping->getSettings().at(0)->variableName(), "myInteger1"); + ASSERT_EQ(pMapping->getSettings().at(0)->value().toNumber(), 20); +} From b0503254015982bc1f2fa6169fd9e735bdfaf59f Mon Sep 17 00:00:00 2001 From: Antoine C Date: Mon, 16 Sep 2024 11:55:55 +0100 Subject: [PATCH 2/4] Refactor setting check logic --- src/controllers/legacycontrollersettings.h | 24 ++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/controllers/legacycontrollersettings.h b/src/controllers/legacycontrollersettings.h index a259da5719c..ec10c7358b8 100644 --- a/src/controllers/legacycontrollersettings.h +++ b/src/controllers/legacycontrollersettings.h @@ -6,6 +6,23 @@ #include "controllers/legacycontrollersettingslayout.h" #include "util/parented_ptr.h" +namespace { +template +bool valid_range(T min, T max, T step) { + // Detecting overflow + VERIFY_OR_DEBUG_ASSERT(max + step > max) { + return false; + } + // Detecting overflow + VERIFY_OR_DEBUG_ASSERT(min - step < min) { + return false; + } + + return (min <= 0 && min + step <= max) || + (max >= 0 && max - step >= min); +} +} // namespace + class QSpinBox; class QDoubleSpinBox; @@ -259,16 +276,11 @@ class LegacyControllerNumberSetting /// and a strictly positive step, strictly less than max.. /// @return true if valid bool valid() const override { - static constexpr bool typeSize = sizeof(long) == sizeof(int); 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 && - (typeSize || - static_cast(m_stepValue) < - static_cast(m_maxValue) - - static_cast(m_minValue)); + m_stepValue > 0 && valid_range(m_minValue, m_maxValue, m_stepValue); } static AbstractLegacyControllerSetting* createFrom(const QDomElement& element) { From 27f0752f8a7b130c46ada304569646bbc3ca61ca Mon Sep 17 00:00:00 2001 From: Antoine C Date: Mon, 16 Sep 2024 17:04:24 +0100 Subject: [PATCH 3/4] Removing irrelevant assertion --- src/controllers/legacycontrollersettings.h | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/controllers/legacycontrollersettings.h b/src/controllers/legacycontrollersettings.h index ec10c7358b8..8254afd60ac 100644 --- a/src/controllers/legacycontrollersettings.h +++ b/src/controllers/legacycontrollersettings.h @@ -9,15 +9,6 @@ namespace { template bool valid_range(T min, T max, T step) { - // Detecting overflow - VERIFY_OR_DEBUG_ASSERT(max + step > max) { - return false; - } - // Detecting overflow - VERIFY_OR_DEBUG_ASSERT(min - step < min) { - return false; - } - return (min <= 0 && min + step <= max) || (max >= 0 && max - step >= min); } From 9cf405f2376d52eaad7284e6ffa657f0d3242f04 Mon Sep 17 00:00:00 2001 From: Antoine C Date: Thu, 19 Sep 2024 13:06:28 +0100 Subject: [PATCH 4/4] Fix invalid assert and add default out of range usecase --- src/controllers/legacycontrollersettings.h | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/controllers/legacycontrollersettings.h b/src/controllers/legacycontrollersettings.h index 8254afd60ac..863992376ec 100644 --- a/src/controllers/legacycontrollersettings.h +++ b/src/controllers/legacycontrollersettings.h @@ -9,8 +9,10 @@ namespace { template bool valid_range(T min, T max, T step) { - return (min <= 0 && min + step <= max) || - (max >= 0 && max - step >= min); + VERIFY_OR_DEBUG_ASSERT(step >= 0) { + return false; + } + return step > 0 && ((min <= 0 && min + step <= max) || (max >= 0 && max - step >= min)); } } // namespace @@ -226,7 +228,13 @@ class LegacyControllerNumberSetting } m_defaultValue = ValueDeserializer(element.attribute("default"), &isOk); if (!isOk) { - m_defaultValue = 0; + if (0 > m_maxValue) { + m_defaultValue = m_maxValue; + } else if (0 < m_minValue) { + m_defaultValue = m_minValue; + } else { + m_defaultValue = 0; + } } reset(); save(); @@ -271,7 +279,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 && valid_range(m_minValue, m_maxValue, m_stepValue); + valid_range(m_minValue, m_maxValue, m_stepValue); } static AbstractLegacyControllerSetting* createFrom(const QDomElement& element) {