D7941: use Kirigami Theme for colors
mart closed this revision. mart added a comment. dependencies are up to date now in metadata REPOSITORY R858 Qt Quick Controls 2: Desktop Style REVISION DETAIL https://phabricator.kde.org/D7941 To: mart, #plasma, #kirigami, hein Cc: bcooksley, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
bcooksley reopened this revision. bcooksley added a comment. This revision is now accepted and ready to land. This code introduces a new dependency which has yet to be declared in the kde-build-metadata repository. As this has been several days now, i'll be reverting this commit and any others which interfere in the revert process in 24 hours if this isn't corrected. REPOSITORY R858 Qt Quick Controls 2: Desktop Style REVISION DETAIL https://phabricator.kde.org/D7941 To: mart, #plasma, #kirigami, hein Cc: bcooksley, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
mart marked an inline comment as done. mart added a comment. In https://phabricator.kde.org/D7941#151979, @davidedmundson wrote: > Is 5.39 tagged already? should be tagged next saturday. the two issues are adressed in master. REPOSITORY R858 Qt Quick Controls 2: Desktop Style REVISION DETAIL https://phabricator.kde.org/D7941 To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
This revision was automatically updated to reflect the committed changes. Closed by commit R858:aa33639fcfd9: use Kirigami Theme for colors (authored by mart). REPOSITORY R858 Qt Quick Controls 2: Desktop Style CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7941?vs=20296=20299 REVISION DETAIL https://phabricator.kde.org/D7941 AFFECTED FILES CMakeLists.txt kirigami-plasmadesktop-integration/CMakeLists.txt kirigami-plasmadesktop-integration/kirigamiplasmafactory.cpp kirigami-plasmadesktop-integration/kirigamiplasmafactory.h kirigami-plasmadesktop-integration/kirigamiplasmaintegration.json kirigami-plasmadesktop-integration/plasmadesktoptheme.cpp kirigami-plasmadesktop-integration/plasmadesktoptheme.h org.kde.desktop/Button.qml org.kde.desktop/CheckBox.qml org.kde.desktop/CheckDelegate.qml org.kde.desktop/ComboBox.qml org.kde.desktop/Dial.qml org.kde.desktop/Dialog.qml org.kde.desktop/Drawer.qml org.kde.desktop/Frame.qml org.kde.desktop/GroupBox.qml org.kde.desktop/ItemDelegate.qml org.kde.desktop/Label.qml org.kde.desktop/Menu.qml org.kde.desktop/MenuItem.qml org.kde.desktop/Popup.qml org.kde.desktop/RadioButton.qml org.kde.desktop/RadioDelegate.qml org.kde.desktop/RangeSlider.qml org.kde.desktop/Slider.qml org.kde.desktop/SpinBox.qml org.kde.desktop/Switch.qml org.kde.desktop/SwitchDelegate.qml org.kde.desktop/TabBar.qml org.kde.desktop/TextArea.qml org.kde.desktop/TextField.qml org.kde.desktop/ToolBar.qml org.kde.desktop/ToolButton.qml org.kde.desktop/ToolTip.qml org.kde.desktop/private/DefaultListItemBackground.qml plugin/CMakeLists.txt plugin/SystemPaletteSingleton.qml plugin/TextSingleton.qml plugin/kquickstyleitem.cpp plugin/kquickstyleitem_p.h plugin/qmldir To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
davidedmundson added a comment.
Is 5.39 tagged already?
INLINE COMMENTS
> plasmadesktoptheme.cpp:48
> +this, [this]() {
> +if (m_parentItem && m_parentItem->window()) {
> +connect(m_parentItem->window(),
> ::activeChanged,
and disconnect from the old window
> plasmadesktoptheme.cpp:82
> +
> +KIconLoader::global()->setCustomPalette(pal);
> +
This is bad
If some other place miles away in a random lib in a different place in the UI,
sets a colour, it will be wrong.
Why meddle with the global loader if you're passing it as an arg anyway?
REPOSITORY
R858 Qt Quick Controls 2: Desktop Style
REVISION DETAIL
https://phabricator.kde.org/D7941
To: mart, #plasma, #kirigami, hein
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
hein accepted this revision. This revision is now accepted and ready to land. REPOSITORY R858 Qt Quick Controls 2: Desktop Style BRANCH mart/kirigamiintegration REVISION DETAIL https://phabricator.kde.org/D7941 To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
mart reopened this revision. REPOSITORY R858 Qt Quick Controls 2: Desktop Style REVISION DETAIL https://phabricator.kde.org/D7941 To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
mart updated this revision to Diff 20296. mart added a comment. - add the kirigami plasma desktop integration here REPOSITORY R858 Qt Quick Controls 2: Desktop Style CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7941?vs=20293=20296 BRANCH mart/kirigamiintegration REVISION DETAIL https://phabricator.kde.org/D7941 AFFECTED FILES CMakeLists.txt kirigami-plasmadesktop-integration/CMakeLists.txt kirigami-plasmadesktop-integration/kirigamiplasmafactory.cpp kirigami-plasmadesktop-integration/kirigamiplasmafactory.h kirigami-plasmadesktop-integration/kirigamiplasmaintegration.json kirigami-plasmadesktop-integration/plasmadesktoptheme.cpp kirigami-plasmadesktop-integration/plasmadesktoptheme.h org.kde.desktop/Button.qml org.kde.desktop/CheckBox.qml org.kde.desktop/CheckDelegate.qml org.kde.desktop/ComboBox.qml org.kde.desktop/Dial.qml org.kde.desktop/Dialog.qml org.kde.desktop/Drawer.qml org.kde.desktop/Frame.qml org.kde.desktop/GroupBox.qml org.kde.desktop/ItemDelegate.qml org.kde.desktop/Label.qml org.kde.desktop/Menu.qml org.kde.desktop/MenuItem.qml org.kde.desktop/Popup.qml org.kde.desktop/RadioButton.qml org.kde.desktop/RadioDelegate.qml org.kde.desktop/RangeSlider.qml org.kde.desktop/Slider.qml org.kde.desktop/SpinBox.qml org.kde.desktop/Switch.qml org.kde.desktop/SwitchDelegate.qml org.kde.desktop/TabBar.qml org.kde.desktop/TextArea.qml org.kde.desktop/TextField.qml org.kde.desktop/ToolBar.qml org.kde.desktop/ToolButton.qml org.kde.desktop/ToolTip.qml org.kde.desktop/private/DefaultListItemBackground.qml plugin/CMakeLists.txt plugin/SystemPaletteSingleton.qml plugin/TextSingleton.qml plugin/kquickstyleitem.cpp plugin/kquickstyleitem_p.h plugin/qmldir To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
This revision was automatically updated to reflect the committed changes. Closed by commit R858:1190c5e0c9f2: use Kirigami Theme for colors (authored by mart). REPOSITORY R858 Qt Quick Controls 2: Desktop Style CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7941?vs=19981=20293 REVISION DETAIL https://phabricator.kde.org/D7941 AFFECTED FILES CMakeLists.txt org.kde.desktop/Button.qml org.kde.desktop/CheckBox.qml org.kde.desktop/CheckDelegate.qml org.kde.desktop/ComboBox.qml org.kde.desktop/Dial.qml org.kde.desktop/Dialog.qml org.kde.desktop/Drawer.qml org.kde.desktop/Frame.qml org.kde.desktop/GroupBox.qml org.kde.desktop/ItemDelegate.qml org.kde.desktop/Label.qml org.kde.desktop/Menu.qml org.kde.desktop/MenuItem.qml org.kde.desktop/Popup.qml org.kde.desktop/RadioButton.qml org.kde.desktop/RadioDelegate.qml org.kde.desktop/RangeSlider.qml org.kde.desktop/Slider.qml org.kde.desktop/SpinBox.qml org.kde.desktop/Switch.qml org.kde.desktop/SwitchDelegate.qml org.kde.desktop/TabBar.qml org.kde.desktop/TextArea.qml org.kde.desktop/TextField.qml org.kde.desktop/ToolBar.qml org.kde.desktop/ToolButton.qml org.kde.desktop/ToolTip.qml org.kde.desktop/private/DefaultListItemBackground.qml plugin/CMakeLists.txt plugin/SystemPaletteSingleton.qml plugin/TextSingleton.qml plugin/kquickstyleitem.cpp plugin/kquickstyleitem_p.h plugin/qmldir To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
mart updated this revision to Diff 19981. mart added a comment. - use Kirigami Theme for colors - clean dead code - use kirigami colors - get rid of SystemPaletteSingleton and TextSingleton REPOSITORY R858 Qt Quick Controls 2: Desktop Style CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7941?vs=19943=19981 BRANCH mart/kirigamiintegration REVISION DETAIL https://phabricator.kde.org/D7941 AFFECTED FILES CMakeLists.txt org.kde.desktop/Button.qml org.kde.desktop/CheckBox.qml org.kde.desktop/CheckDelegate.qml org.kde.desktop/ComboBox.qml org.kde.desktop/Dial.qml org.kde.desktop/Dialog.qml org.kde.desktop/Drawer.qml org.kde.desktop/Frame.qml org.kde.desktop/GroupBox.qml org.kde.desktop/ItemDelegate.qml org.kde.desktop/Label.qml org.kde.desktop/Menu.qml org.kde.desktop/MenuItem.qml org.kde.desktop/Popup.qml org.kde.desktop/RadioButton.qml org.kde.desktop/RadioDelegate.qml org.kde.desktop/RangeSlider.qml org.kde.desktop/Slider.qml org.kde.desktop/SpinBox.qml org.kde.desktop/Switch.qml org.kde.desktop/SwitchDelegate.qml org.kde.desktop/TabBar.qml org.kde.desktop/TextArea.qml org.kde.desktop/TextField.qml org.kde.desktop/ToolBar.qml org.kde.desktop/ToolButton.qml org.kde.desktop/ToolTip.qml org.kde.desktop/private/DefaultListItemBackground.qml plugin/CMakeLists.txt plugin/SystemPaletteSingleton.qml plugin/TextSingleton.qml plugin/kquickstyleitem.cpp plugin/kquickstyleitem_p.h plugin/qmldir To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
mart reopened this revision. REPOSITORY R858 Qt Quick Controls 2: Desktop Style REVISION DETAIL https://phabricator.kde.org/D7941 To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
This revision was automatically updated to reflect the committed changes. Closed by commit R858:9e3ee983d161: clean dead code (authored by mart). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D7941?vs=19942=19943#toc REPOSITORY R858 Qt Quick Controls 2: Desktop Style CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7941?vs=19942=19943 REVISION DETAIL https://phabricator.kde.org/D7941 AFFECTED FILES plugin/kquickstyleitem.cpp To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
mart updated this revision to Diff 19942. mart added a comment. proper diff? REPOSITORY R858 Qt Quick Controls 2: Desktop Style CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7941?vs=19940=19942 BRANCH mart/kirigamiintegration REVISION DETAIL https://phabricator.kde.org/D7941 AFFECTED FILES CMakeLists.txt org.kde.desktop/Button.qml org.kde.desktop/CheckBox.qml org.kde.desktop/ComboBox.qml org.kde.desktop/ItemDelegate.qml org.kde.desktop/Label.qml org.kde.desktop/Slider.qml org.kde.desktop/SpinBox.qml org.kde.desktop/TextArea.qml org.kde.desktop/TextField.qml org.kde.desktop/ToolButton.qml org.kde.desktop/private/DefaultListItemBackground.qml plugin/CMakeLists.txt plugin/kquickstyleitem.cpp plugin/kquickstyleitem_p.h To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
mart added inline comments. INLINE COMMENTS > davidedmundson wrote in kquickstyleitem.cpp:167 > I meant specifically the setInherit > > that's also exposed in QML and you have that being explicitly set there. ouch yeah, that's actually a leftover :) REPOSITORY R858 Qt Quick Controls 2: Desktop Style REVISION DETAIL https://phabricator.kde.org/D7941 To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
mart updated this revision to Diff 19940. mart added a comment. Restricted Application added a project: Kirigami. remove dead code update colors on the fly on qapp palette change REPOSITORY R858 Qt Quick Controls 2: Desktop Style CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7941?vs=19800=19940 BRANCH mart/kirigamiintegration REVISION DETAIL https://phabricator.kde.org/D7941 AFFECTED FILES plugin/kquickstyleitem.cpp To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
D7941: use Kirigami Theme for colors
davidedmundson added inline comments. INLINE COMMENTS > davidedmundson wrote in kquickstyleitem.cpp:167 > I don't understand what this is doing? It looks wrong? I meant specifically the setInherit that's also exposed in QML and you have that being explicitly set there. REPOSITORY R858 Qt Quick Controls 2: Desktop Style REVISION DETAIL https://phabricator.kde.org/D7941 To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7941: use Kirigami Theme for colors
mart added a comment. In https://phabricator.kde.org/D7941#148757, @davidedmundson wrote: > This is set to be a framework. > We can't have a framework that imports something from workspace. it imports kirigami, which is a framework, so it just makes it tier2 INLINE COMMENTS > davidedmundson wrote in Label.qml:32 > This change is unrelated. > Also I said in a review about how we shouldn't be doing this. it is as since StylePrivate is mostly binding QPalette colors, all its uses should be eventually removed > davidedmundson wrote in kquickstyleitem.cpp:167 > I don't understand what this is doing? It looks wrong? it's getting (and eventually creating if didn't exist yet) the attached property Kirigami.Theme, which is a subclass of the public symbol PlatformTheme, which is guaranteed to exist at that point > davidedmundson wrote in kquickstyleitem.cpp:709 > This area needs some tidying. > > Also we need this line if the palette isn't explicitly set to something else. definitely a mess, yeah :) Anyways, the idea is not to use anymore the qapplication's palette as areas of the application window can use a completely different palette. so m_theme->palette() will fallback to QApplication::palette() when the runtime integration to kcolorscheme is not installed, and instead will be built upon KColorscheme when https://phabricator.kde.org/D7940 is installed REPOSITORY R858 Qt Quick Controls 2: Desktop Style REVISION DETAIL https://phabricator.kde.org/D7941 To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7941: use Kirigami Theme for colors
davidedmundson added a comment. This is set to be a framework. We can't have a framework that imports something from workspace. INLINE COMMENTS > Label.qml:32 > > -height: Math.round(Math.max(paintedHeight, > StylePrivate.TextSingleton.height * 1.6)) > +height: Math.round(Math.max(paintedHeight, Kirigami.Units * 1.6)) > verticalAlignment: lineCount > 1 ? Text.AlignTop : Text.AlignVCenter This change is unrelated. Also I said in a review about how we shouldn't be doing this. > kquickstyleitem.cpp:167 > +Q_ASSERT(m_theme); > +m_theme->setInherit(true); > + I don't understand what this is doing? It looks wrong? > kquickstyleitem.cpp:709 > > -m_styleoption->palette = QApplication::palette(classNameForItem()); > +//m_styleoption->palette = QApplication::palette(classNameForItem()); > +//m_theme->setColorSet(Kirigami::PlatformTheme::Complementary); This area needs some tidying. Also we need this line if the palette isn't explicitly set to something else. REPOSITORY R858 Qt Quick Controls 2: Desktop Style REVISION DETAIL https://phabricator.kde.org/D7941 To: mart, #plasma, #kirigami, hein Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7941: use Kirigami Theme for colors
mart added reviewers: Plasma, Kirigami, hein. REPOSITORY R858 Qt Quick Controls 2: Desktop Style REVISION DETAIL https://phabricator.kde.org/D7941 To: mart, #plasma, #kirigami, hein Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D7941: use Kirigami Theme for colors
mart created this revision. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. REVISION SUMMARY it makes possible to use kirigami's color sets REPOSITORY R858 Qt Quick Controls 2: Desktop Style BRANCH mart/kirigamiintegration REVISION DETAIL https://phabricator.kde.org/D7941 AFFECTED FILES CMakeLists.txt org.kde.desktop/Button.qml org.kde.desktop/CheckBox.qml org.kde.desktop/ComboBox.qml org.kde.desktop/ItemDelegate.qml org.kde.desktop/Label.qml org.kde.desktop/Slider.qml org.kde.desktop/SpinBox.qml org.kde.desktop/TextArea.qml org.kde.desktop/TextField.qml org.kde.desktop/ToolButton.qml org.kde.desktop/private/DefaultListItemBackground.qml plugin/CMakeLists.txt plugin/kquickstyleitem.cpp plugin/kquickstyleitem_p.h To: mart Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
