From 3a209a7e905abf7f6299a0500ffc925fd5b9c04b Mon Sep 17 00:00:00 2001 From: Kacper Donat Date: Sun, 10 Aug 2025 22:53:53 +0200 Subject: [PATCH 1/6] Base: Allow constexpr Color This changes Base::Color class so it can be used as constexpr, useful for defining various defaults. --- src/Base/Color.cpp | 7 ------- src/Base/Color.h | 7 ++++++- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Base/Color.cpp b/src/Base/Color.cpp index b5619d1e36..0d63b76339 100644 --- a/src/Base/Color.cpp +++ b/src/Base/Color.cpp @@ -34,13 +34,6 @@ using namespace Base; // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) -Color::Color(float red, float green, float blue, float alpha) - : r(red) - , g(green) - , b(blue) - , a(alpha) -{} - Color::Color(uint32_t rgba) : Color {} { diff --git a/src/Base/Color.h b/src/Base/Color.h index 09cc698cd4..cd537acb89 100644 --- a/src/Base/Color.h +++ b/src/Base/Color.h @@ -93,7 +93,12 @@ public: * Defines the color as (R,G,B,A) whereas all values are in the range [0,1]. * \a A defines the alpha value. */ - explicit Color(float R = 0.0, float G = 0.0, float B = 0.0, float A = 1.0); + constexpr explicit Color(float R = 0.0, float G = 0.0, float B = 0.0, float A = 1.0) + : r(R) + , g(G) + , b(B) + , a(A) + {} /** * Does basically the same as the constructor above unless that (R,G,B,A) is From cf55183ece26ade11fcc0e4d26cf5da43f950c3f Mon Sep 17 00:00:00 2001 From: Kacper Donat Date: Sun, 10 Aug 2025 23:01:14 +0200 Subject: [PATCH 2/6] Gui: StyleParameters - use Base::Color instead of QColor If possible we should prefer using our own classess instead of ones coming from frameworks. This changes Style Parameters to use Base::Color class instead of QColor. Calculations are still done using QColor but the data is always exposed as Base::Color. --- src/Gui/Dialogs/DlgThemeEditor.cpp | 7 +- src/Gui/StyleParameters/ParameterManager.cpp | 6 +- src/Gui/StyleParameters/ParameterManager.h | 4 +- src/Gui/StyleParameters/Parser.cpp | 33 +++--- src/Gui/StyleParameters/Parser.h | 4 +- .../StyleParameters/ParameterManagerTest.cpp | 27 ++--- tests/src/Gui/StyleParameters/ParserTest.cpp | 102 +++++++++--------- 7 files changed, 93 insertions(+), 90 deletions(-) diff --git a/src/Gui/Dialogs/DlgThemeEditor.cpp b/src/Gui/Dialogs/DlgThemeEditor.cpp index eeabf70581..03d614d588 100644 --- a/src/Gui/Dialogs/DlgThemeEditor.cpp +++ b/src/Gui/Dialogs/DlgThemeEditor.cpp @@ -30,6 +30,7 @@ #include "ui_DlgThemeEditor.h" #include "BitmapFactory.h" +#include #include #include @@ -67,7 +68,7 @@ QString typeOfTokenValue(const Gui::StyleParameters::Value& value) [](const Gui::StyleParameters::Length&) { return QWidget::tr("Length"); }, - [](const QColor&) { + [](const Base::Color&) { return QWidget::tr("Color"); } }, @@ -529,8 +530,8 @@ QVariant StyleParametersModel::data(const QModelIndex& index, int role) const } if (role == Qt::DecorationRole) { - if (index.column() == ParameterPreview && std::holds_alternative(value)) { - return colorPreview(std::get(value)); + 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 939e3aa55b..5347634ca2 100644 --- a/src/Gui/StyleParameters/ParameterManager.cpp +++ b/src/Gui/StyleParameters/ParameterManager.cpp @@ -95,9 +95,9 @@ std::string Value::toString() const return fmt::format("{}{}", value, unit); } - if (std::holds_alternative(*this)) { - auto color = std::get(*this); - return fmt::format("#{:0>6x}", 0xFFFFFF & color.rgb()); // NOLINT(*-magic-numbers) + if (std::holds_alternative(*this)) { + auto color = std::get(*this); + return fmt::format("#{:0>6x}", color.getPackedRGB() >> 8); // NOLINT(*-magic-numbers) } return std::get(*this); diff --git a/src/Gui/StyleParameters/ParameterManager.h b/src/Gui/StyleParameters/ParameterManager.h index 8736755c99..8e53747c09 100644 --- a/src/Gui/StyleParameters/ParameterManager.h +++ b/src/Gui/StyleParameters/ParameterManager.h @@ -107,9 +107,9 @@ private: * * As a rule, operations can be only performed over values of the same type. */ -struct Value : std::variant +struct Value : std::variant { - using std::variant::variant; + using std::variant::variant; /** * Converts the object into its string representation. diff --git a/src/Gui/StyleParameters/Parser.cpp b/src/Gui/StyleParameters/Parser.cpp index 99910a6a5e..3cd22bc226 100644 --- a/src/Gui/StyleParameters/Parser.cpp +++ b/src/Gui/StyleParameters/Parser.cpp @@ -26,6 +26,7 @@ #include "Parser.h" #include "ParameterManager.h" +#include #include #ifndef _PreComp_ @@ -67,12 +68,12 @@ Value FunctionCall::evaluate(const EvaluationContext& context) const auto colorArg = arguments[0]->evaluate(context); auto amountArg = arguments[1]->evaluate(context); - if (!std::holds_alternative(colorArg)) { + if (!std::holds_alternative(colorArg)) { THROWM(Base::ExpressionError, fmt::format("'{}' is not supported for colors", functionName)); } - auto color = std::get(colorArg); + auto color = std::get(colorArg).asValue(); // In Qt if you want to make color 20% darker or lighter, you need to pass 120 as the value // we, however, want users to pass only the relative difference, hence we need to add the @@ -82,11 +83,11 @@ Value FunctionCall::evaluate(const EvaluationContext& context) const auto amount = 100 + static_cast(std::get(amountArg).value); if (functionName == "lighten") { - return color.lighter(amount); + return Base::Color::fromValue(color.lighter(amount)); } if (functionName == "darken") { - return color.darker(amount); + return Base::Color::fromValue(color.darker(amount)); } return {}; @@ -104,25 +105,25 @@ Value FunctionCall::evaluate(const EvaluationContext& context) const auto secondColorArg = arguments[1]->evaluate(context); auto amountArg = arguments[2]->evaluate(context); - if (!std::holds_alternative(firstColorArg)) { + if (!std::holds_alternative(firstColorArg)) { THROWM(Base::ExpressionError, fmt::format("first argument of '{}' must be color", functionName)); } - if (!std::holds_alternative(secondColorArg)) { + if (!std::holds_alternative(secondColorArg)) { THROWM(Base::ExpressionError, fmt::format("second argument of '{}' must be color", functionName)); } - auto firstColor = std::get(firstColorArg); - auto secondColor = std::get(secondColorArg); + auto firstColor = std::get(firstColorArg); + auto secondColor = std::get(secondColorArg); - auto amount = Base::fromPercent(std::get(amountArg).value); + auto amount = Base::fromPercent(static_cast(std::get(amountArg).value)); - return QColor::fromRgbF( - (1 - amount) * firstColor.redF() + amount * secondColor.redF(), - (1 - amount) * firstColor.greenF() + amount * secondColor.greenF(), - (1 - amount) * firstColor.blueF() + amount * secondColor.blueF() + return Base::Color( + (1 - amount) * firstColor.r + amount * secondColor.r, + (1 - amount) * firstColor.g + amount * secondColor.g, + (1 - amount) * firstColor.b + amount * secondColor.b ); }; @@ -169,7 +170,7 @@ Value BinaryOp::evaluate(const EvaluationContext& context) const Value UnaryOp::evaluate(const EvaluationContext& context) const { Value val = operand->evaluate(context); - if (std::holds_alternative(val)) { + if (std::holds_alternative(val)) { THROWM(Base::ExpressionError, "Unary operations on colors are not supported"); } @@ -286,7 +287,7 @@ std::unique_ptr Parser::parseColor() int b = std::stoi(input.substr(pos, 2), nullptr, hexadecimalBase); pos += 2; - return std::make_unique(QColor(r, g, b)); + return std::make_unique(Base::Color(r / 255.0, g / 255.0, b / 255.0)); }; const auto parseFunctionStyleColor = [&]() { @@ -313,7 +314,7 @@ std::unique_ptr Parser::parseColor() if (!match(')')) { THROWM(Base::ParserError, fmt::format("Expected ')' after color arguments, got '{}'", input[pos])); } - return std::make_unique(QColor(r, g, b, a)); + return std::make_unique(Base::Color(r / 255.0, g / 255.0, b / 255.0, a / 255.0)); }; skipWhitespace(); diff --git a/src/Gui/StyleParameters/Parser.h b/src/Gui/StyleParameters/Parser.h index 3e7867cae9..34ff09ce6d 100644 --- a/src/Gui/StyleParameters/Parser.h +++ b/src/Gui/StyleParameters/Parser.h @@ -83,9 +83,9 @@ struct GuiExport Number: public Expr struct GuiExport Color: public Expr { - QColor color; + Base::Color color; - explicit Color(QColor color) + explicit Color(Base::Color color) : color(std::move(color)) {} diff --git a/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp b/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp index 09c40cf13b..fd5e98cc6f 100644 --- a/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp +++ b/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp @@ -24,6 +24,7 @@ #include #include +#include #include using namespace Gui::StyleParameters; @@ -73,20 +74,20 @@ TEST_F(ParameterManagerTest, BasicParameterResolution) { auto result = manager.resolve("PrimaryColor"); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); - EXPECT_EQ(color.red(), 255); - EXPECT_EQ(color.green(), 0); - EXPECT_EQ(color.blue(), 0); + 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); } { auto result = manager.resolve("SecondaryColor"); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); - EXPECT_EQ(color.red(), 0); - EXPECT_EQ(color.green(), 255); - EXPECT_EQ(color.blue(), 0); + 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); } } @@ -294,10 +295,10 @@ TEST_F(ParameterManagerTest, ComplexExpressions) { auto result = manager.resolve("ColorWithFunction"); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); + EXPECT_TRUE(std::holds_alternative(result)); + auto color = std::get(result).asValue(); // Should be lighter than the original red - EXPECT_GT(color.lightness(), QColor("#ff0000").lightness()); + EXPECT_GT(color.lightness(), QColor(0xff0000).lightness()); } } diff --git a/tests/src/Gui/StyleParameters/ParserTest.cpp b/tests/src/Gui/StyleParameters/ParserTest.cpp index a7e1b50734..563c1265e7 100644 --- a/tests/src/Gui/StyleParameters/ParserTest.cpp +++ b/tests/src/Gui/StyleParameters/ParserTest.cpp @@ -23,7 +23,7 @@ #include -#include +#include #include #include @@ -103,56 +103,56 @@ TEST_F(ParserTest, ParseColors) Parser parser("#ff0000"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); - EXPECT_EQ(color.red(), 255); - EXPECT_EQ(color.green(), 0); - EXPECT_EQ(color.blue(), 0); + 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); } { Parser parser("#00ff00"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); - EXPECT_EQ(color.red(), 0); - EXPECT_EQ(color.green(), 255); - EXPECT_EQ(color.blue(), 0); + 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); } { Parser parser("#0000ff"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); - EXPECT_EQ(color.red(), 0); - EXPECT_EQ(color.green(), 0); - EXPECT_EQ(color.blue(), 255); + EXPECT_TRUE(std::holds_alternative(result)); + auto color = std::get(result); + EXPECT_EQ(color.r, 0); + EXPECT_EQ(color.g, 0); + EXPECT_EQ(color.b, 1); } { Parser parser("rgb(255, 0, 0)"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); - EXPECT_EQ(color.red(), 255); - EXPECT_EQ(color.green(), 0); - EXPECT_EQ(color.blue(), 0); + 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); } { Parser parser("rgba(255, 0, 0, 128)"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); - EXPECT_EQ(color.red(), 255); - EXPECT_EQ(color.green(), 0); - EXPECT_EQ(color.blue(), 0); - EXPECT_EQ(color.alpha(), 128); + EXPECT_TRUE(std::holds_alternative(result)); + auto color = std::get(result); + EXPECT_DOUBLE_EQ(color.r, 1); + EXPECT_DOUBLE_EQ(color.g, 0); + EXPECT_DOUBLE_EQ(color.b, 0); + EXPECT_NEAR(color.a, 128 / 255.0, 1e-6); } } @@ -173,11 +173,11 @@ TEST_F(ParserTest, ParseParameterReferences) Parser parser("@TestColor"); auto expr = parser.parse(); auto result = expr->evaluate({.manager = &manager, .context = {}}); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); - EXPECT_EQ(color.red(), 255); - EXPECT_EQ(color.green(), 0); - EXPECT_EQ(color.blue(), 0); + 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); } { @@ -360,30 +360,30 @@ TEST_F(ParserTest, ParseFunctionCalls) Parser parser("lighten(#ff0000, 20)"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); + EXPECT_TRUE(std::holds_alternative(result)); + auto color = std::get(result).asValue(); // The result should be lighter than the original red - EXPECT_GT(color.lightness(), QColor("#ff0000").lightness()); + EXPECT_GT(color.lightness(), QColor(0xff0000).lightness()); } { Parser parser("darken(#ff0000, 20)"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); + EXPECT_TRUE(std::holds_alternative(result)); + auto color = std::get(result).asValue(); // The result should be darker than the original red - EXPECT_LT(color.lightness(), QColor("#ff0000").lightness()); + EXPECT_LT(color.lightness(), QColor(0xff0000).lightness()); } { Parser parser("lighten(@TestColor, 20)"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); + EXPECT_TRUE(std::holds_alternative(result)); + auto color = std::get(result).asValue(); // The result should be lighter than the original red - EXPECT_GT(color.lightness(), QColor("#ff0000").lightness()); + EXPECT_GT(color.lightness(), QColor(0xff0000).lightness()); } } @@ -496,11 +496,11 @@ TEST_F(ParserTest, ParseWhitespace) Parser parser("rgb(255,0,0)"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); - EXPECT_EQ(color.red(), 255); - EXPECT_EQ(color.green(), 0); - EXPECT_EQ(color.blue(), 0); + 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); } } @@ -539,11 +539,11 @@ TEST_F(ParserTest, ParseEdgeCases) Parser parser("#ff0000"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - EXPECT_TRUE(std::holds_alternative(result)); - auto color = std::get(result); - EXPECT_EQ(color.red(), 255); - EXPECT_EQ(color.green(), 0); - EXPECT_EQ(color.blue(), 0); + 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); } // Single parameter reference From 322ed2c7bc2be46f28075dafc0767535f2b896d6 Mon Sep 17 00:00:00 2001 From: Kacper Donat Date: Sun, 10 Aug 2025 23:06:41 +0200 Subject: [PATCH 3/6] Gui: Rename StyleParameters::Length to StyleParameters::Numeric This change gives a better name to the parameter type that is used to store numeric values. Before it was called length, but it can store other quantities as well so the name is no longer fitting. --- src/Gui/Dialogs/DlgThemeEditor.cpp | 4 +- src/Gui/StyleParameters/ParameterManager.cpp | 17 +-- src/Gui/StyleParameters/ParameterManager.h | 22 ++-- src/Gui/StyleParameters/Parser.cpp | 12 +- src/Gui/StyleParameters/Parser.h | 2 +- .../StyleParameters/ParameterManagerTest.cpp | 40 +++--- tests/src/Gui/StyleParameters/ParserTest.cpp | 120 +++++++++--------- 7 files changed, 109 insertions(+), 108 deletions(-) diff --git a/src/Gui/Dialogs/DlgThemeEditor.cpp b/src/Gui/Dialogs/DlgThemeEditor.cpp index 03d614d588..e12180a8ed 100644 --- a/src/Gui/Dialogs/DlgThemeEditor.cpp +++ b/src/Gui/Dialogs/DlgThemeEditor.cpp @@ -65,8 +65,8 @@ QString typeOfTokenValue(const Gui::StyleParameters::Value& value) [](const std::string&) { return QWidget::tr("Generic"); }, - [](const Gui::StyleParameters::Length&) { - return QWidget::tr("Length"); + [](const Gui::StyleParameters::Numeric&) { + return QWidget::tr("Numeric"); }, [](const Base::Color&) { return QWidget::tr("Color"); diff --git a/src/Gui/StyleParameters/ParameterManager.cpp b/src/Gui/StyleParameters/ParameterManager.cpp index 5347634ca2..54efe995a0 100644 --- a/src/Gui/StyleParameters/ParameterManager.cpp +++ b/src/Gui/StyleParameters/ParameterManager.cpp @@ -31,30 +31,31 @@ #include #include #include +#include #include #endif namespace Gui::StyleParameters { -Length Length::operator+(const Length& rhs) const +Numeric Numeric::operator+(const Numeric& rhs) const { ensureEqualUnits(rhs); return {value + rhs.value, unit}; } -Length Length::operator-(const Length& rhs) const +Numeric Numeric::operator-(const Numeric& rhs) const { ensureEqualUnits(rhs); return {value - rhs.value, unit}; } -Length Length::operator-() const +Numeric Numeric::operator-() const { return {-value, unit}; } -Length Length::operator/(const Length& rhs) const +Numeric Numeric::operator/(const Numeric& rhs) const { if (rhs.value == 0) { THROWM(Base::RuntimeError, "Division by zero"); @@ -68,7 +69,7 @@ Length Length::operator/(const Length& rhs) const return {value / rhs.value, unit}; } -Length Length::operator*(const Length& rhs) const +Numeric Numeric::operator*(const Numeric& rhs) const { if (rhs.unit.empty() || unit.empty()) { return {value * rhs.value, unit}; @@ -78,7 +79,7 @@ Length Length::operator*(const Length& rhs) const return {value * rhs.value, unit}; } -void Length::ensureEqualUnits(const Length& rhs) const +void Numeric::ensureEqualUnits(const Numeric& rhs) const { if (unit != rhs.unit) { THROWM(Base::RuntimeError, @@ -90,8 +91,8 @@ void Length::ensureEqualUnits(const Length& rhs) const std::string Value::toString() const { - if (std::holds_alternative(*this)) { - auto [value, unit] = std::get(*this); + if (std::holds_alternative(*this)) { + auto [value, unit] = std::get(*this); return fmt::format("{}{}", value, unit); } diff --git a/src/Gui/StyleParameters/ParameterManager.h b/src/Gui/StyleParameters/ParameterManager.h index 8e53747c09..50902db46e 100644 --- a/src/Gui/StyleParameters/ParameterManager.h +++ b/src/Gui/StyleParameters/ParameterManager.h @@ -51,7 +51,7 @@ class Parser; * represents a dimensionless length that can be used as a scalar. This struct does not care about * unit conversions as its uses do not require it. */ -struct Length +struct Numeric { /// Numeric value of the length. double value; @@ -66,11 +66,11 @@ struct Length * and hence act as a scalar. * * @code{c++} - * Length a { 10, "px" }; - * Length b { 5, "px" }; + * Numeric a { 10, "px" }; + * Numeric b { 5, "px" }; * - * Length differentUnit { 3, "rem" } - * Length scalar { 2, "" }; + * Numeric differentUnit { 3, "rem" } + * Numeric scalar { 2, "" }; * * // basic operations with the same unit are allowed * auto sum = a + b; // 15 px @@ -85,16 +85,16 @@ struct Length * @endcode * @{ */ - Length operator+(const Length& rhs) const; - Length operator-(const Length& rhs) const; - Length operator-() const; + Numeric operator+(const Numeric& rhs) const; + Numeric operator-(const Numeric& rhs) const; + Numeric operator-() const; - Length operator/(const Length& rhs) const; - Length operator*(const Length& rhs) const; + Numeric operator/(const Numeric& rhs) const; + Numeric operator*(const Numeric& rhs) const; /// @} private: - void ensureEqualUnits(const Length& rhs) const; + void ensureEqualUnits(const Numeric& rhs) const; }; /** diff --git a/src/Gui/StyleParameters/Parser.cpp b/src/Gui/StyleParameters/Parser.cpp index 3cd22bc226..6e68e57a31 100644 --- a/src/Gui/StyleParameters/Parser.cpp +++ b/src/Gui/StyleParameters/Parser.cpp @@ -80,7 +80,7 @@ Value FunctionCall::evaluate(const EvaluationContext& context) const // 100 required by Qt. // // NOLINTNEXTLINE(*-magic-numbers) - auto amount = 100 + static_cast(std::get(amountArg).value); + auto amount = 100 + static_cast(std::get(amountArg).value); if (functionName == "lighten") { return Base::Color::fromValue(color.lighter(amount)); @@ -118,7 +118,7 @@ Value FunctionCall::evaluate(const EvaluationContext& context) const auto firstColor = std::get(firstColorArg); auto secondColor = std::get(secondColorArg); - auto amount = Base::fromPercent(static_cast(std::get(amountArg).value)); + auto amount = Base::fromPercent(static_cast(std::get(amountArg).value)); return Base::Color( (1 - amount) * firstColor.r + amount * secondColor.r, @@ -146,12 +146,12 @@ Value BinaryOp::evaluate(const EvaluationContext& context) const Value lval = left->evaluate(context); Value rval = right->evaluate(context); - if (!std::holds_alternative(lval) || !std::holds_alternative(rval)) { + if (!std::holds_alternative(lval) || !std::holds_alternative(rval)) { THROWM(Base::ExpressionError, "Math operations are supported only on lengths"); } - auto lhs = std::get(lval); - auto rhs = std::get(rval); + auto lhs = std::get(lval); + auto rhs = std::get(rval); switch (op) { case Operator::Add: @@ -174,7 +174,7 @@ Value UnaryOp::evaluate(const EvaluationContext& context) const THROWM(Base::ExpressionError, "Unary operations on colors are not supported"); } - auto v = std::get(val); + auto v = std::get(val); switch (op) { case Operator::Add: return v; diff --git a/src/Gui/StyleParameters/Parser.h b/src/Gui/StyleParameters/Parser.h index 34ff09ce6d..8d2ad27ecc 100644 --- a/src/Gui/StyleParameters/Parser.h +++ b/src/Gui/StyleParameters/Parser.h @@ -72,7 +72,7 @@ struct GuiExport ParameterReference: public Expr struct GuiExport Number: public Expr { - Length value; + Numeric value; Number(double value, std::string unit) : value({value, std::move(unit)}) diff --git a/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp b/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp index fd5e98cc6f..e3c53326f2 100644 --- a/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp +++ b/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp @@ -66,8 +66,8 @@ TEST_F(ParameterManagerTest, BasicParameterResolution) { { 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, 16.0); // Should get value from source2 (later source) EXPECT_EQ(length.unit, "px"); } @@ -96,16 +96,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 +116,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 +134,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 +143,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 +164,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"); } @@ -279,16 +279,16 @@ 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"); } diff --git a/tests/src/Gui/StyleParameters/ParserTest.cpp b/tests/src/Gui/StyleParameters/ParserTest.cpp index 563c1265e7..a2b550f34c 100644 --- a/tests/src/Gui/StyleParameters/ParserTest.cpp +++ b/tests/src/Gui/StyleParameters/ParserTest.cpp @@ -59,8 +59,8 @@ TEST_F(ParserTest, ParseNumbers) Parser parser("42"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 42.0); EXPECT_EQ(length.unit, ""); } @@ -69,8 +69,8 @@ TEST_F(ParserTest, ParseNumbers) Parser parser("10.5px"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 10.5); EXPECT_EQ(length.unit, "px"); } @@ -79,8 +79,8 @@ TEST_F(ParserTest, ParseNumbers) Parser parser("2.5em"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 2.5); EXPECT_EQ(length.unit, "em"); } @@ -89,8 +89,8 @@ TEST_F(ParserTest, ParseNumbers) Parser parser("100%"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 100.0); EXPECT_EQ(length.unit, "%"); } @@ -163,8 +163,8 @@ TEST_F(ParserTest, ParseParameterReferences) Parser parser("@TestParam"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 10.0); EXPECT_EQ(length.unit, "px"); } @@ -184,8 +184,8 @@ TEST_F(ParserTest, ParseParameterReferences) Parser parser("@TestNumber"); auto expr = parser.parse(); auto result = expr->evaluate({.manager = &manager, .context = {}}); - 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, 5.0); EXPECT_EQ(length.unit, ""); } @@ -198,8 +198,8 @@ TEST_F(ParserTest, ParseArithmeticOperations) Parser parser("10 + 5"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 15.0); EXPECT_EQ(length.unit, ""); } @@ -208,8 +208,8 @@ TEST_F(ParserTest, ParseArithmeticOperations) Parser parser("10px + 5px"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 15.0); EXPECT_EQ(length.unit, "px"); } @@ -218,8 +218,8 @@ TEST_F(ParserTest, ParseArithmeticOperations) Parser parser("10 - 5"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 5.0); EXPECT_EQ(length.unit, ""); } @@ -228,8 +228,8 @@ TEST_F(ParserTest, ParseArithmeticOperations) Parser parser("10px - 5px"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 5.0); EXPECT_EQ(length.unit, "px"); } @@ -238,8 +238,8 @@ TEST_F(ParserTest, ParseArithmeticOperations) Parser parser("10 * 5"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 50.0); EXPECT_EQ(length.unit, ""); } @@ -248,8 +248,8 @@ TEST_F(ParserTest, ParseArithmeticOperations) Parser parser("10px * 2"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 20.0); EXPECT_EQ(length.unit, "px"); } @@ -258,8 +258,8 @@ TEST_F(ParserTest, ParseArithmeticOperations) Parser parser("10 / 2"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 5.0); EXPECT_EQ(length.unit, ""); } @@ -268,8 +268,8 @@ TEST_F(ParserTest, ParseArithmeticOperations) Parser parser("10px / 2"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 5.0); EXPECT_EQ(length.unit, "px"); } @@ -282,8 +282,8 @@ TEST_F(ParserTest, ParseComplexExpressions) Parser parser("(10 + 5) * 2"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 30.0); EXPECT_EQ(length.unit, ""); } @@ -292,8 +292,8 @@ TEST_F(ParserTest, ParseComplexExpressions) Parser parser("(10px + 5px) * 2"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 30.0); EXPECT_EQ(length.unit, "px"); } @@ -302,8 +302,8 @@ TEST_F(ParserTest, ParseComplexExpressions) Parser parser("@TestParam + 5px"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 15.0); EXPECT_EQ(length.unit, "px"); } @@ -312,8 +312,8 @@ TEST_F(ParserTest, ParseComplexExpressions) Parser parser("@TestParam * @TestNumber"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 50.0); EXPECT_EQ(length.unit, "px"); } @@ -326,8 +326,8 @@ TEST_F(ParserTest, ParseUnaryOperations) Parser parser("+10"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 10.0); EXPECT_EQ(length.unit, ""); } @@ -336,8 +336,8 @@ TEST_F(ParserTest, ParseUnaryOperations) Parser parser("-10"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, -10.0); EXPECT_EQ(length.unit, ""); } @@ -346,8 +346,8 @@ TEST_F(ParserTest, ParseUnaryOperations) Parser parser("-10px"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, -10.0); EXPECT_EQ(length.unit, "px"); } @@ -476,8 +476,8 @@ TEST_F(ParserTest, ParseWhitespace) Parser parser(" 10 + 5 "); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 15.0); EXPECT_EQ(length.unit, ""); } @@ -486,8 +486,8 @@ TEST_F(ParserTest, ParseWhitespace) Parser parser("10px+5px"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 15.0); EXPECT_EQ(length.unit, "px"); } @@ -528,8 +528,8 @@ TEST_F(ParserTest, ParseEdgeCases) Parser parser("42"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 42.0); EXPECT_EQ(length.unit, ""); } @@ -551,8 +551,8 @@ TEST_F(ParserTest, ParseEdgeCases) Parser parser("@TestParam"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 10.0); EXPECT_EQ(length.unit, "px"); } @@ -565,8 +565,8 @@ TEST_F(ParserTest, ParseOperatorPrecedence) Parser parser("2 + 3 * 4"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 14.0); // 2 + (3 * 4) = 2 + 12 = 14 EXPECT_EQ(length.unit, ""); } @@ -575,8 +575,8 @@ TEST_F(ParserTest, ParseOperatorPrecedence) Parser parser("10 - 3 * 2"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 4.0); // 10 - (3 * 2) = 10 - 6 = 4 EXPECT_EQ(length.unit, ""); } @@ -585,8 +585,8 @@ TEST_F(ParserTest, ParseOperatorPrecedence) Parser parser("20 / 4 + 3"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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); // (20 / 4) + 3 = 5 + 3 = 8 EXPECT_EQ(length.unit, ""); } @@ -599,8 +599,8 @@ TEST_F(ParserTest, ParseNestedParentheses) Parser parser("((2 + 3) * 4)"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 20.0); // (5) * 4 = 20 EXPECT_EQ(length.unit, ""); } @@ -609,8 +609,8 @@ TEST_F(ParserTest, ParseNestedParentheses) Parser parser("(10 - (3 + 2)) * 2"); auto expr = parser.parse(); auto result = expr->evaluate({&manager, {}}); - 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, 10.0); // (10 - 5) * 2 = 5 * 2 = 10 EXPECT_EQ(length.unit, ""); } From 0cce9c1261f59c65dc5eaf293c222baaf200821b Mon Sep 17 00:00:00 2001 From: Kacper Donat Date: Sun, 10 Aug 2025 23:11:34 +0200 Subject: [PATCH 4/6] 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)); } From 2f1d96bf4408fd7affd81237eef2657154f2438c Mon Sep 17 00:00:00 2001 From: Kacper Donat Date: Sun, 10 Aug 2025 23:12:52 +0200 Subject: [PATCH 5/6] Gui: Add ParameterDefinition to Style Parameters This adds a way to define parameters within code, that allows developers to easily encapsulate the parameters and their default values. --- src/Gui/StyleParameters/ParameterManager.h | 61 ++++++++++++++++++- .../StyleParameters/ParameterManagerTest.cpp | 19 ++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/src/Gui/StyleParameters/ParameterManager.h b/src/Gui/StyleParameters/ParameterManager.h index 50902db46e..53d3ed3703 100644 --- a/src/Gui/StyleParameters/ParameterManager.h +++ b/src/Gui/StyleParameters/ParameterManager.h @@ -37,6 +37,14 @@ #include #include +// That macro uses inline const because some older compilers to not properly support constexpr +// for std::string. It should be changed into static constepxr once we migrate to newer compiler. +#define DEFINE_STYLE_PARAMETER(_name_, _defaultValue_) \ + static inline const Gui::StyleParameters::ParameterDefinition _name_ { \ + .name = #_name_, \ + .defaultValue = (_defaultValue_), \ + } + namespace Gui::StyleParameters { @@ -56,7 +64,7 @@ struct Numeric /// Numeric value of the length. double value; /// Unit of the length, empty if the value is dimensionless. - std::string unit; + std::string unit = ""; /** * @name Operators @@ -119,6 +127,33 @@ struct Value : std::variant std::string toString() const; }; +/** + * @brief A structure to define parameters which can be referenced in the code. + * + * @tparam T The type of the parameter. + * + * This structure allows defining parameters which encapsulate a name and a corresponding + * default value. Parameters defined this way can be reused across the codebase for consistency. + * The supported value types include: + * - Numeric types. + * - Colors using `Base::Color`. + * - Strings. + * + * Example Usage: + * @code{.cpp} + * DEFINE_STYLE_PARAMETER(SomeParameter, Numeric { 10 }); + * DEFINE_STYLE_PARAMETER(TextColor, Base::Color(0.5F, 0.2F, 0.8F)); + * @endcode + */ +template +struct ParameterDefinition +{ + /// The name of the parameter, must be unique. + const char* name; + /// The default value of the parameter. + T defaultValue; +}; + /** * @struct Parameter * @@ -413,7 +448,29 @@ public: * @param context Resolution context for handling circular references * @return The resolved value */ - Value resolve(const std::string& name, ResolveContext context = {}) const; + std::optional resolve(const std::string& name, ResolveContext context = {}) const; + + /** + * @brief Resolves a parameter to its final value, based on definition. + * + * This method evaluates the parameter's expression and returns the computed + * value or default one from definition if the parameter is not available. + * + * @param definition Definition of the parameter to resolve + * @param context Resolution context for handling circular references + * @return The resolved value + */ + template + T resolve(const ParameterDefinition& definition, ResolveContext context = {}) const + { + auto value = resolve(definition.name, std::move(context)); + + if (!value || !std::holds_alternative(*value)) { + return definition.defaultValue; + } + + return std::get(*value); + } /** * @brief Evaluates an expression string and returns the result. diff --git a/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp b/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp index ceefab2298..57bb79656c 100644 --- a/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp +++ b/tests/src/Gui/StyleParameters/ParameterManagerTest.cpp @@ -328,3 +328,22 @@ TEST_F(ParameterManagerTest, ErrorHandling) EXPECT_TRUE(invalidResult.has_value()); EXPECT_TRUE(std::holds_alternative(*invalidResult)); } + +DEFINE_STYLE_PARAMETER(BaseSize, Numeric(8, "px")); + +TEST_F(ParameterManagerTest, ResolveParameterDefinition) +{ + auto result = manager.resolve(BaseSize); + EXPECT_DOUBLE_EQ(result.value, 16); + EXPECT_EQ(result.unit, "px"); +} + + +DEFINE_STYLE_PARAMETER(MarginSize, Numeric(16, "px")); + +TEST_F(ParameterManagerTest, ResolveParameterDefinitionDefault) +{ + auto result = manager.resolve(MarginSize); + EXPECT_DOUBLE_EQ(result.value, 16); + EXPECT_EQ(result.unit, "px"); +} From 6bbb4458c92fd711c46e8ace724103b0b1e9d463 Mon Sep 17 00:00:00 2001 From: Kacper Donat Date: Sun, 10 Aug 2025 23:13:58 +0200 Subject: [PATCH 6/6] PartDesign: Use Style Parameters for theming previews --- src/Mod/PartDesign/Gui/CMakeLists.txt | 1 + src/Mod/PartDesign/Gui/StyleParameters.h | 43 +++++++++++++++++++ src/Mod/PartDesign/Gui/ViewProvider.cpp | 17 +++++--- .../PartDesign/Gui/ViewProviderDressUp.cpp | 20 ++++++--- 4 files changed, 69 insertions(+), 12 deletions(-) create mode 100644 src/Mod/PartDesign/Gui/StyleParameters.h diff --git a/src/Mod/PartDesign/Gui/CMakeLists.txt b/src/Mod/PartDesign/Gui/CMakeLists.txt index 6bc83e47ec..91ba4e6679 100644 --- a/src/Mod/PartDesign/Gui/CMakeLists.txt +++ b/src/Mod/PartDesign/Gui/CMakeLists.txt @@ -217,6 +217,7 @@ SET(PartDesignGuiModule_SRCS PreCompiled.h SketchWorkflow.cpp SketchWorkflow.h + StyleParameters.h Utils.cpp Utils.h Workbench.cpp diff --git a/src/Mod/PartDesign/Gui/StyleParameters.h b/src/Mod/PartDesign/Gui/StyleParameters.h new file mode 100644 index 0000000000..7d54f4178c --- /dev/null +++ b/src/Mod/PartDesign/Gui/StyleParameters.h @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: LGPL-2.1-or-later +/**************************************************************************** + * * + * Copyright (c) 2025 Kacper Donat * + * * + * This file is part of FreeCAD. * + * * + * FreeCAD is free software: you can redistribute it and/or modify it * + * under the terms of the GNU Lesser General Public License as * + * published by the Free Software Foundation, either version 2.1 of the * + * License, or (at your option) any later version. * + * * + * FreeCAD is distributed in the hope that it will be useful, but * + * WITHOUT ANY WARRANTY; without even the implied warranty of * + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * + * Lesser General Public License for more details. * + * * + * You should have received a copy of the GNU Lesser General Public * + * License along with FreeCAD. If not, see * + * . * + * * + ***************************************************************************/ + +#ifndef STYLEPARAMETERS_H +#define STYLEPARAMETERS_H + +#include + +namespace PartDesignGui::StyleParameters { + DEFINE_STYLE_PARAMETER(PreviewAdditiveColor, Base::Color(0.0F, 1.0F, 0.6F)); + DEFINE_STYLE_PARAMETER(PreviewSubtractiveColor, Base::Color(1.0F, 0.0F, 0.0F)); + DEFINE_STYLE_PARAMETER(PreviewDressUpColor, Base::Color(1.0F, 0.0F, 1.0F)); + + DEFINE_STYLE_PARAMETER(PreviewErrorColor, Base::Color(1.0F, 0.0F, 0.0F)); + DEFINE_STYLE_PARAMETER(PreviewErrorTransparency, Gui::StyleParameters::Numeric(0.95)); + + DEFINE_STYLE_PARAMETER(PreviewToolTransparency, Gui::StyleParameters::Numeric(0.95)); + DEFINE_STYLE_PARAMETER(PreviewShapeTransparency, Gui::StyleParameters::Numeric(0.8)); + + DEFINE_STYLE_PARAMETER(PreviewLineWidth, Gui::StyleParameters::Numeric(2)); +} + +#endif //STYLEPARAMETERS_H diff --git a/src/Mod/PartDesign/Gui/ViewProvider.cpp b/src/Mod/PartDesign/Gui/ViewProvider.cpp index 6ba8634a34..6e1805e12a 100644 --- a/src/Mod/PartDesign/Gui/ViewProvider.cpp +++ b/src/Mod/PartDesign/Gui/ViewProvider.cpp @@ -33,6 +33,7 @@ #endif #include +#include #include #include #include @@ -51,6 +52,7 @@ #include #include "TaskFeatureParameters.h" +#include "StyleParameters.h" #include "ViewProvider.h" #include "ViewProviderPy.h" @@ -78,13 +80,14 @@ void ViewProvider::attach(App::DocumentObject* pcObject) { ViewProviderPart::attach(pcObject); - if (auto addSubFeature = getObject()) { - const Base::Color green(0.0F, 1.0F, 0.6F); - const Base::Color red(1.0F, 0.0F, 0.0F); + auto* styleParameterManager = Base::provideService(); + if (auto addSubFeature = getObject()) { bool isAdditive = addSubFeature->getAddSubType() == PartDesign::FeatureAddSub::Additive; - PreviewColor.setValue(isAdditive ? green : red); + PreviewColor.setValue( + isAdditive ? styleParameterManager->resolve(StyleParameters::PreviewAdditiveColor) + : styleParameterManager->resolve(StyleParameters::PreviewSubtractiveColor)); } } @@ -217,8 +220,12 @@ void ViewProvider::attachPreview() { ViewProviderPreviewExtension::attachPreview(); + auto* styleParameterManager = Base::provideService(); + + pcPreviewShape->lineWidth = styleParameterManager->resolve(StyleParameters::PreviewLineWidth).value; + pcToolPreview = new PartGui::SoPreviewShape; - pcToolPreview->transparency = 0.95F; + pcToolPreview->transparency = styleParameterManager->resolve(StyleParameters::PreviewToolTransparency).value; pcToolPreview->color.connectFrom(&pcPreviewShape->color); pcPreviewRoot->addChild(pcToolPreview); diff --git a/src/Mod/PartDesign/Gui/ViewProviderDressUp.cpp b/src/Mod/PartDesign/Gui/ViewProviderDressUp.cpp index 199226c984..d6a25dc858 100644 --- a/src/Mod/PartDesign/Gui/ViewProviderDressUp.cpp +++ b/src/Mod/PartDesign/Gui/ViewProviderDressUp.cpp @@ -37,8 +37,11 @@ #include #include "ViewProviderDressUp.h" + +#include "StyleParameters.h" #include "TaskDressUpParameters.h" +#include #include using namespace PartDesignGui; @@ -50,8 +53,8 @@ void ViewProviderDressUp::attach(App::DocumentObject* pcObject) { ViewProvider::attach(pcObject); - const Base::Color magenta(1.0F, 0.0F, 1.0F); - PreviewColor.setValue(magenta); + auto* styleParameterManager = Base::provideService(); + PreviewColor.setValue(styleParameterManager->resolve(StyleParameters::PreviewDressUpColor)); setErrorState(false); } @@ -135,11 +138,14 @@ void ViewProviderDressUp::highlightReferences(const bool on) void ViewProviderDressUp::setErrorState(bool error) { - const Base::Color red(1.0, 0.0, 0.0); + auto* styleParameterManager = Base::provideService(); - constexpr float errorTransparency = 0.95F; - - pcPreviewShape->transparency = error ? errorTransparency : PartGui::SoPreviewShape::defaultTransparency; - pcPreviewShape->color = Base::convertTo(error ? red : PreviewColor.getValue()); + pcPreviewShape->transparency = styleParameterManager + ->resolve(error ? StyleParameters::PreviewErrorTransparency + : StyleParameters::PreviewShapeTransparency) + .value; + pcPreviewShape->color = error + ? styleParameterManager->resolve(StyleParameters::PreviewErrorColor).asValue() + : PreviewColor.getValue().asValue(); }