From bf856a107623c788dd20e675f2dbd270edd34c33 Mon Sep 17 00:00:00 2001 From: Kacper Donat Date: Sun, 10 Aug 2025 23:11:34 +0200 Subject: [PATCH] Gui: Make StyleParameterManager::resolve return optional result This changes the resolve method of style parameter manager to return optional intead of definitive result. In reality tokens may not be defioned correctly and optionals provide a good way to nicely handle defaults. --- src/Gui/Dialogs/DlgThemeEditor.cpp | 12 ++-- src/Gui/StyleParameters/ParameterManager.cpp | 22 ++++--- src/Gui/StyleParameters/Parser.cpp | 2 +- .../StyleParameters/ParameterManagerTest.cpp | 63 ++++++++++--------- 4 files changed, 55 insertions(+), 44 deletions(-) diff --git a/src/Gui/Dialogs/DlgThemeEditor.cpp b/src/Gui/Dialogs/DlgThemeEditor.cpp index e12180a8ed..efe66aa1c2 100644 --- a/src/Gui/Dialogs/DlgThemeEditor.cpp +++ b/src/Gui/Dialogs/DlgThemeEditor.cpp @@ -514,6 +514,10 @@ QVariant StyleParametersModel::data(const QModelIndex& index, int role) const const auto& [name, token, _] = *parameterItem; const auto& value = manager->resolve(name.toStdString()); + if (!value) { + return {}; + } + if (role == Qt::DisplayRole) { if (index.column() == ParameterName) { return name; @@ -522,16 +526,16 @@ QVariant StyleParametersModel::data(const QModelIndex& index, int role) const return QString::fromStdString(token.value); } if (index.column() == ParameterType) { - return typeOfTokenValue(value); + return typeOfTokenValue(*value); } if (index.column() == ParameterPreview) { - return QString::fromStdString(value.toString()); + return QString::fromStdString(value->toString()); } } if (role == Qt::DecorationRole) { - if (index.column() == ParameterPreview && std::holds_alternative(value)) { - return colorPreview(std::get(value).asValue()); + if (index.column() == ParameterPreview && std::holds_alternative(*value)) { + return colorPreview(std::get(*value).asValue()); } } } diff --git a/src/Gui/StyleParameters/ParameterManager.cpp b/src/Gui/StyleParameters/ParameterManager.cpp index 54efe995a0..3e2e24ec8f 100644 --- a/src/Gui/StyleParameters/ParameterManager.cpp +++ b/src/Gui/StyleParameters/ParameterManager.cpp @@ -260,12 +260,16 @@ std::string ParameterManager::replacePlaceholders(const std::string& expression, QString::fromStdString(expression), [&](const QRegularExpressionMatch& match) { auto tokenName = match.captured(1).toStdString(); - auto tokenValue = resolve(tokenName, context); - context.visited.erase(tokenName); - return QString::fromStdString(tokenValue.toString()); - } + if (!tokenValue) { + Base::Console().warning("Requested non-existent style parameter token '%s'.\n", tokenName); + return QStringLiteral(""); + } + + context.visited.erase(tokenName); + return QString::fromStdString(tokenValue->toString()); + } ).toStdString(); // clang-format on } @@ -293,18 +297,18 @@ std::optional ParameterManager::expression(const std::string& name) return {}; } -Value ParameterManager::resolve(const std::string& name, ResolveContext context) const +std::optional ParameterManager::resolve(const std::string& name, + ResolveContext context) const { std::optional maybeParameter = this->parameter(name); if (!maybeParameter) { - Base::Console().warning("Requested non-existent design token '%s'.\n", name); - return std::string {}; + return std::nullopt; } if (context.visited.contains(name)) { - Base::Console().warning("The design token '%s' contains circular-reference.\n", name); - return expression(name).value_or(std::string {}); + Base::Console().warning("The style parameter '%s' contains circular-reference.\n", name); + return expression(name); } const Parameter& token = *maybeParameter; diff --git a/src/Gui/StyleParameters/Parser.cpp b/src/Gui/StyleParameters/Parser.cpp index 6e68e57a31..26f664d95b 100644 --- a/src/Gui/StyleParameters/Parser.cpp +++ b/src/Gui/StyleParameters/Parser.cpp @@ -42,7 +42,7 @@ namespace Gui::StyleParameters Value ParameterReference::evaluate(const EvaluationContext& context) const { - return context.manager->resolve(name, context.context); + return context.manager->resolve(name, context.context).value_or("@" + name); } Value Number::evaluate([[maybe_unused]] const EvaluationContext& context) const diff --git a/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp b/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp index e3c53326f2..ceefab2298 100644 --- a/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp +++ b/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp @@ -66,16 +66,18 @@ TEST_F(ParameterManagerTest, BasicParameterResolution) { { auto result = manager.resolve("BaseSize"); - EXPECT_TRUE(std::holds_alternative(result)); - auto length = std::get(result); + EXPECT_TRUE(result.has_value()); + EXPECT_TRUE(std::holds_alternative(*result)); + auto length = std::get(*result); EXPECT_DOUBLE_EQ(length.value, 16.0); // Should get value from source2 (later source) EXPECT_EQ(length.unit, "px"); } { auto result = manager.resolve("PrimaryColor"); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); + EXPECT_TRUE(result.has_value()); + EXPECT_TRUE(std::holds_alternative(*result)); + auto color = std::get(*result); EXPECT_EQ(color.r, 1); EXPECT_EQ(color.g, 0); EXPECT_EQ(color.b, 0); @@ -83,8 +85,9 @@ TEST_F(ParameterManagerTest, BasicParameterResolution) { auto result = manager.resolve("SecondaryColor"); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); + EXPECT_TRUE(result.has_value()); + EXPECT_TRUE(std::holds_alternative(*result)); + auto color = std::get(*result); EXPECT_EQ(color.r, 0); EXPECT_EQ(color.g, 1); EXPECT_EQ(color.b, 0); @@ -96,16 +99,16 @@ TEST_F(ParameterManagerTest, ParameterReferences) { { auto result = manager.resolve("Margin"); - EXPECT_TRUE(std::holds_alternative(result)); - auto length = std::get(result); + EXPECT_TRUE(std::holds_alternative(*result)); + auto length = std::get(*result); EXPECT_DOUBLE_EQ(length.value, 32.0); // @BaseSize * 2 = 16 * 2 = 32 EXPECT_EQ(length.unit, "px"); } { auto result = manager.resolve("Padding"); - EXPECT_TRUE(std::holds_alternative(result)); - auto length = std::get(result); + EXPECT_TRUE(std::holds_alternative(*result)); + auto length = std::get(*result); EXPECT_DOUBLE_EQ(length.value, 8.0); // @BaseSize / 2 = 16 / 2 = 8 EXPECT_EQ(length.unit, "px"); } @@ -116,15 +119,15 @@ TEST_F(ParameterManagerTest, Caching) { // First resolution should cache the result auto result1 = manager.resolve("BaseSize"); - EXPECT_TRUE(std::holds_alternative(result1)); + EXPECT_TRUE(std::holds_alternative(*result1)); // Second resolution should use cached value auto result2 = manager.resolve("BaseSize"); - EXPECT_TRUE(std::holds_alternative(result2)); + EXPECT_TRUE(std::holds_alternative(*result2)); // Results should be identical - auto length1 = std::get(result1); - auto length2 = std::get(result2); + auto length1 = std::get(*result1); + auto length2 = std::get(*result2); EXPECT_DOUBLE_EQ(length1.value, length2.value); EXPECT_EQ(length1.unit, length2.unit); } @@ -134,8 +137,8 @@ TEST_F(ParameterManagerTest, CacheInvalidation) { // Initial resolution auto result1 = manager.resolve("BaseSize"); - EXPECT_TRUE(std::holds_alternative(result1)); - auto length1 = std::get(result1); + EXPECT_TRUE(std::holds_alternative(*result1)); + auto length1 = std::get(*result1); EXPECT_DOUBLE_EQ(length1.value, 16.0); // Reload should clear cache @@ -143,8 +146,8 @@ TEST_F(ParameterManagerTest, CacheInvalidation) // Resolution after reload should work the same auto result2 = manager.resolve("BaseSize"); - EXPECT_TRUE(std::holds_alternative(result2)); - auto length2 = std::get(result2); + EXPECT_TRUE(std::holds_alternative(*result2)); + auto length2 = std::get(*result2); EXPECT_DOUBLE_EQ(length2.value, 16.0); EXPECT_EQ(length1.unit, length2.unit); } @@ -164,8 +167,8 @@ TEST_F(ParameterManagerTest, SourcePriority) // Should get value from the latest source (highest priority) auto result = manager.resolve("BaseSize"); - EXPECT_TRUE(std::holds_alternative(result)); - auto length = std::get(result); + EXPECT_TRUE(std::holds_alternative(*result)); + auto length = std::get(*result); EXPECT_DOUBLE_EQ(length.value, 24.0); EXPECT_EQ(length.unit, "px"); } @@ -259,7 +262,7 @@ TEST_F(ParameterManagerTest, CircularReferenceDetection) // Should handle circular reference gracefully auto result = manager.resolve("A"); // Should return the expression string as fallback - EXPECT_TRUE(std::holds_alternative(result)); + EXPECT_TRUE(std::holds_alternative(*result)); } // Test complex expressions @@ -279,24 +282,24 @@ TEST_F(ParameterManagerTest, ComplexExpressions) { auto result = manager.resolve("ComplexMargin"); - EXPECT_TRUE(std::holds_alternative(result)); - auto length = std::get(result); + EXPECT_TRUE(std::holds_alternative(*result)); + auto length = std::get(*result); EXPECT_DOUBLE_EQ(length.value, 40.0); // (16 + 4) * 2 = 20 * 2 = 40 EXPECT_EQ(length.unit, "px"); } { auto result = manager.resolve("ComplexPadding"); - EXPECT_TRUE(std::holds_alternative(result)); - auto length = std::get(result); + EXPECT_TRUE(std::holds_alternative(*result)); + auto length = std::get(*result); EXPECT_DOUBLE_EQ(length.value, 7.0); // (16 - 2) / 2 = 14 / 2 = 7 EXPECT_EQ(length.unit, "px"); } { auto result = manager.resolve("ColorWithFunction"); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result).asValue(); + EXPECT_TRUE(std::holds_alternative(*result)); + auto color = std::get(*result).asValue(); // Should be lighter than the original red EXPECT_GT(color.lightness(), QColor(0xff0000).lightness()); } @@ -307,8 +310,7 @@ TEST_F(ParameterManagerTest, ErrorHandling) { // Test non-existent parameter auto result = manager.resolve("NonExistent"); - EXPECT_TRUE(std::holds_alternative(result)); - EXPECT_EQ(std::get(result), ""); + EXPECT_FALSE(result.has_value()); // Test invalid expression auto invalidSource = std::make_unique( @@ -323,5 +325,6 @@ TEST_F(ParameterManagerTest, ErrorHandling) // Should handle invalid expression gracefully auto invalidResult = manager.resolve("Invalid"); // Should return the expression string as fallback - EXPECT_TRUE(std::holds_alternative(invalidResult)); + EXPECT_TRUE(invalidResult.has_value()); + EXPECT_TRUE(std::holds_alternative(*invalidResult)); }