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 (67)
|
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.
| return s_asideTextFontBaseSize + m_fontSizeOffset; | ||
| } | ||
|
|
||
| QJSValue Theme::fontSize() const |
There was a problem hiding this comment.
Beautiful! Never crossed my mind to try this approach!
There was a problem hiding this comment.
it's probably a good approach to use on all the hardcoded dimensions we have in the app and ultimately to replace the scaling.
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
|
|
||
| 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
from
5309fdb to
85e5f55
Compare
4 participants
What does the PR do
Reimplements Theme from stateful singleton to attached property, dynamically propagated through the components tree.
[TODO[
Affected areas
Architecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
Impact on end user
How to test
Risk