Theme: C++ implementation based on propagated attached property #19316
Conversation
micieslak
requested review from
a team,
alexjba,
caybro and
noeliaSD
as code owners
micieslak
force-pushed
the
feat/theme-attached-type
branch
from
71355b5 to
d6a2153
Compare
Jenkins BuildsClick to see older builds (154)
|
micieslak
force-pushed
the
feat/theme-attached-type
branch
3 times, most recently
from
c1b54b0 to
6cf7b25
Compare
alexjba
left a comment
alexjba
left a comment
There was a problem hiding this comment.
Awesome work! I love it :beers
Let's get it merged quickly now that we have the release branch. It's probably a burden to maintain.
micieslak
force-pushed
the
feat/theme-attached-type
branch
2 times, most recently
from
cf4e110 to
5309fdb
Compare
caybro
left a comment
caybro
left a comment
There was a problem hiding this comment.
Awesome work! Just some suggestions and minor things
| { | ||
| QColor c = color; | ||
| if (alpha > 0.0 && alpha <= 1.0) | ||
| c.setAlphaF(alpha); |
There was a problem hiding this comment.
Not sure this is going to work as expected... you usually want to switch to a HSL/HSV color space first
There was a problem hiding this comment.
Old code:
function setColorAlpha(color, alpha) {
return Qt.hsla(color.hslHue, color.hslSaturation, color.hslLightness, alpha)
}There was a problem hiding this comment.
This code is replacement for:
function alphaColor(color, alpha) {
let actualColor = Qt.darker(color, 1)
actualColor.a = alpha
return actualColor
}where the first line let actualColor = Qt.darker(color, 1) is just tricky conversion in case e.g. "red" color literal is provided. The version you quoted is from Utils, and it's not related with this code.
There was a problem hiding this comment.
Yeah but that's why it does the darker first, it probably converts to HSL behind the scene
There was a problem hiding this comment.
No it's just to convert literals like "red" to color type.
|
|
||
| Q_PROPERTY(Style style READ style WRITE setStyle RESET resetStyle | ||
| NOTIFY styleChanged) | ||
| Q_PROPERTY(const ThemePalette* palette READ palette |
There was a problem hiding this comment.
Perhaps we could also expose other old properties (like fonts) in order to minimize the QML diff, wdyt?
There was a problem hiding this comment.
I found it cleaner to keep things like Fonts as a separate, dedicated component responsible only for fonts. Especially that there is that FontLoader loading logic inside.
It creates bigger diff indeed, but it's just fully automatic replacement.
There was a problem hiding this comment.
Yup, bigger diff but also... it will be very hard to backport fixes now, e.g. to 2.36 branch but I agree that's just a temporary problem
| readonly property alias monoFont: monoFont.font | ||
| readonly property alias codeFont: codeFont.font | ||
|
|
||
| FontLoader { |
There was a problem hiding this comment.
We could (pre)load all these fonts from C++ too, and it would be faster imo :) And just expose the three baseFont/monoFont/codeFont properties
There was a problem hiding this comment.
I meant https://doc.qt.io/qt-6/qfontdatabase.html#addApplicationFont
There was a problem hiding this comment.
The idea was to keep the attached type as small/simple as possible. Ideally only stuff that can benefit from propagation should be there. I found it more modular when keeping things like that separated.
There was a problem hiding this comment.
Yup, this wouldn't be part of the propagation, just initialized and setup somewhere else, not in QML
micieslak
force-pushed
the
feat/theme-attached-type
branch
6 times, most recently
from
b16bc5d to
bd198c5
Compare
noeliaSD
left a comment
noeliaSD
left a comment
There was a problem hiding this comment.
Great work on this implementation! The structure looks solid and the overall approach makes the theming system feel clean and easy to work with. I’ve left a couple of comments and suggestions, but everything is looking really good.
Next step for me will be to run a full test of the PR on both desktop and mobile to ensure everything behaves correctly across breakpoints.
| readonly property real disabledOpacity: 0.3 | ||
| readonly property real pressedOpacity: 0.7 | ||
|
|
||
| function changeTheme(target: QtObject, theme:int) { |
There was a problem hiding this comment.
We could unify the nomenclature with setTheme instead.
| Q_PROPERTY(int fontSizeOffset READ fontSizeOffset WRITE setFontSizeOffset | ||
| RESET resetFontSizeOffset NOTIFY fontSizeOffsetChanged) | ||
|
|
||
| Q_PROPERTY(int secondaryAdditionalTextSize |
There was a problem hiding this comment.
I know we currently use these text properties across the codebase, but I’m increasingly unsure about their usefulness. The naming pattern is really hard to interpret. It's difficult to predict what something like secondaryAdditionalTextSize or tertiaryTextFontSize is supposed to represent. The hierarchy isn’t intuitive, and it makes the system harder to navigate and maintain.
I think we should evaluate whether this abstraction still serves us, or whether we need a more predictable, semantic text scale.
There was a problem hiding this comment.
I think it's result of long evolution of design, where new designs were requiring new values and they were put under new, creative names. Now it would be valuable to re-define smaller set of variants, with better naming, in Figma, as part of new design system. Then we can change it in code and refactor usages.
| readonly property bool bottomSheet: !bottomSheetAllowed ? false: | ||
| d.windowHeight > d.windowWidth | ||
| && d.windowWidth <= Theme.portraitBreakpoint.width | ||
| && d.windowWidth <= ThemeUtils.portraitBreakpoint.width |
There was a problem hiding this comment.
While implementing responsive components, I consistently felt that we’re missing a global property for this. Each component ends up checking the window width independently to determine whether it's in a compact/portrait mode, which leads to duplication and inconsistent logic.
Would it make sense to introduce a centralized property (something like portraitMode or a general compactMode flag) that all components can rely on? Having a single source of truth would make the responsive behavior more consistent across the entire app and simplify component logic.
There was a problem hiding this comment.
Yeah, I think it's good idea to consider using propagated property for that purpose. It will be easy to add to the Theme attached property and control from top-level main component, with possibility to adjust on any level if needed.
|
|
||
| // paddings | ||
| qreal defaultPadding() const; | ||
| qreal defaultXlPadding() const; |
There was a problem hiding this comment.
Why do we need these properties? Shouldn't be just internal?
There was a problem hiding this comment.
Yes, it was also my feeling that it should be internal. But now we are using those default values in some components. For some reason it was decided to have some values constant, with no respect to the padding size selected by the user.
| } | ||
|
|
||
| function linkTypeColor(linkType) { | ||
| function linkTypeColor(linkType: int, palette: ThemePalette) : color { |
There was a problem hiding this comment.
Maybe these colors should be also inside the palette?
There was a problem hiding this comment.
Yeah, would be great to invent better solution for that, especially that the palette is passed there only for handling default color. However I'm reluctant about making Theme aware of some external enums like that link type in this case.
|
|
||
| function colorForColorId(colorId) { | ||
| if (colorId < 0 || colorId >= Theme.palette.userCustomizationColors.length) { | ||
| function colorForColorId(palette, colorId) { |
There was a problem hiding this comment.
Shouldn't this be part of StatusColors?
There was a problem hiding this comment.
Rather not because StatusColors holds just fixed definitions. Here we want to fetch color from give palette.
| } | ||
|
|
||
| function getHoveredColor(colorId) { | ||
| function getHoveredColor(palette, colorId) { |
There was a problem hiding this comment.
Same here.. shouldn't this be part of StatusColors instead?
There was a problem hiding this comment.
No, because StatusColors holds just read-only constants. It also doesn't fit to ThemePalette as the colorId abstraction is not part of theme palette.
| } | ||
|
|
||
| function getIdForColor(color){ | ||
| function getIdForColor(palette, color){ |
| } | ||
|
|
||
| function getColorForId(colorId) { | ||
| function getColorForId(palette, colorId) { |
There was a problem hiding this comment.
Overall, the functions changed here feel like they should belong to StatusColors instead. Conceptually, they align more with color-token handling than with the responsibilities of this module.
There was a problem hiding this comment.
I think that those things could be extracted to more focused ColorUtils, but shouldn't belong to StatusColors which is stateless singleton. Holding app-wise palette in a singleton is against the idea of the whole refactor, as that global mutable state is removed in this PR. Those functions are just "glue" between app logic, mapping concepts from both ends (like colorId and some colors from palette).
micieslak
force-pushed
the
feat/theme-attached-type
branch
2 times, most recently
from
d6a9bd3 to
e9ab1e6
Compare
micieslak
force-pushed
the
feat/theme-attached-type
branch
from
e9ab1e6 to
f1bfc61
Compare
micieslak
requested review from
friofry and
saledjenic
and removed request for
a team
5 participants
What does the PR do
Re-implements
Themefrom stateful singleton to attached property object, with dynamically propagated properties through the visual objects tree.It consists of following elements:
Theme- attached property type, holds state (padding, font size offset, style), which is propagated within the objects tree.StatusColors- singleton, stateless, holds fixed colors definitionsThemePalette<- non-mutable, not creatable component served byTheme, defining colors. We have two instances for dark/light stylesThemeUtils- singleton, stateless utility methods and constantsFonts- singleton, stateless, loads and serves fontsAssets- singleton, stateless, access point for png/svg/emojiBy using attached property type, visual features like colors and sizes are no longer single global state, the same for all components within the app. It brings better flexibility when building responsive layouts for multiple platforms.
Affected areas
Entire QML codebase
Architecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Impact on end user
This change doesn't bring any changes visible to the end user.
How to test
Run the app and go through all possible views to detect incorrect colors, fonts, paddings, etc.
Risk
As there are many changes, there is a risk of introducing regressions related to visual appearance.