Theme: C++ implementation based on propagated attached property#19316
Merged
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
Member
Jenkins BuildsClick to see older builds (257)
|
micieslak
force-pushed
the
feat/theme-attached-type
branch
3 times, most recently
from
c1b54b0 to
6cf7b25
Compare
alexjba
approved these changes
Nov 25, 2025
Contributor
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
reviewed
Nov 25, 2025
Member
caybro
left a comment
caybro
left a comment
There was a problem hiding this comment.
Awesome work! Just some suggestions and minor things
| readonly property alias monoFont: monoFont.font | ||
| readonly property alias codeFont: codeFont.font | ||
|
|
||
| FontLoader { |
Member
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
Member
There was a problem hiding this comment.
I meant https://doc.qt.io/qt-6/qfontdatabase.html#addApplicationFont
Member
Author
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.
Member
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
reviewed
Dec 2, 2025
Contributor
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.
micieslak
force-pushed
the
feat/theme-attached-type
branch
3 times, most recently
from
e9ab1e6 to
f1bfc61
Compare
micieslak
requested review from
friofry and
saledjenic
and removed request for
a team
Contributor
|
@micieslak, could we postpone merging this PR until a big tokens-related refactor gets merged? |
micieslak
force-pushed
the
feat/theme-attached-type
branch
from
ca99e7b to
fefffda
Compare
Member
Author
caybro
approved these changes
Dec 4, 2025
Member
caybro
left a comment
caybro
left a comment
There was a problem hiding this comment.
Great work, let's merge this 🚀
micieslak
force-pushed
the
feat/theme-attached-type
branch
from
fefffda to
c3ed396
Compare
micieslak
force-pushed
the
feat/theme-attached-type
branch
from
c3ed396 to
658d72a
Compare
micieslak
force-pushed
the
feat/theme-attached-type
branch
from
658d72a to
af3312b
Compare
This was referenced Dec 10, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does the PR do
Re-implements
Themefrom stateful singleton to attached property object, with dynamically propagated properties through the visual objects tree.The approach is similar to the one used by buit-in themes, like
MaterialorUniversal.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.
Closes: #19213
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.