This is a basic styling of a QtQuickControls2 series based on Plasma theme.
it has the main controls available in Qt 5.7
it installs them as a separate import (org.kde.plasma.controls 3.0) for
use restricted to plasmoids, is probably needed to be still installed as
a style as well
Details
- Reviewers
davidedmundson - Group Reviewers
Plasma - Commits
- R242:76af5399dd95: Plasma controls based on QtQuickControls2
R242:b2f6a9503738: separately install as style and import
tried with minimal QML files, a more comprehensive gallery may be needed.
pending considerations:
- some of the classes, like Drawer, the dialogs and ApplicationWindow *don't* make sense in plasmoids
- it will probably still need to be installed also as a qqc2 style, as this should be used in Plasma mobile
- probably only some of the controls should be installed as a separate import, the whole set as a style strictly for Plasma Mobile use. the same qml files would be used, so no maintainance overhead
Diff Detail
- Repository
- R242 Plasma Framework (Library)
- Branch
- mart/qqc2style
- Lint
No Linters Available - Unit
No Unit Test Coverage
Skimmed through it until ItemDelegate.qml, few nitpicks for that below
src/declarativeimports/plasmacomponents3/BusyIndicator.qml | ||
---|---|---|
28 ↗ | (On Diff #11076) | Apparently in Qt 5.8 the implicit sizes now take into account padding but since we want to support 5.7 (?) makes sense. Mentioned in the changelog: https://code.qt.io/cgit/qt/qtquickcontrols2.git/tree/dist/changes-5.8.0 |
41 ↗ | (On Diff #11076) | You sure this doesn't break anything if you mess with anchors and sizes and don't have a wrapper Item around it? |
44 ↗ | (On Diff #11076) | I think with QtQuick 2 smooth doesn't matter anymore, other than making it look worse. From docs
|
src/declarativeimports/plasmacomponents3/Button.qml | ||
37 ↗ | (On Diff #11076) | Docs say
|
39 ↗ | (On Diff #11076) | What Label is this? I don't see any import/namespace that would provide this here |
41 ↗ | (On Diff #11076) | QQC2 Label supposedly supports font (and probably other) inheritance |
src/declarativeimports/plasmacomponents3/CheckIndicator.qml | ||
63 ↗ | (On Diff #11076) | Now that we have activeFocusReason we could potentially behave differently whether it was user-focused or tab-focused (not now but keeping that in mind for the future) |
src/declarativeimports/plasmacomponents3/ComboBox.qml | ||
96 ↗ | (On Diff #11076) | in-window Popup makes me sad :( |
src/declarativeimports/plasmacomponents3/DialogButtonBox.qml | ||
41 ↗ | (On Diff #11076) | Do we really want a ListView here? Also, how does it work with e.g. HelpRole and things that would go on the left rather than the right? Dunno if it supports that though |
src/declarativeimports/plasmacomponents3/Drawer.qml | ||
35 ↗ | (On Diff #11076) | Math.round(units.devicePixelRatio) instead of 1? |
src/declarativeimports/plasmacomponents3/Frame.qml | ||
34 ↗ | (On Diff #11076) | Hardcoded number |
src/declarativeimports/plasmacomponents3/BusyIndicator.qml | ||
---|---|---|
28 ↗ | (On Diff #11076) | would it break with an update to Qt 5.8? |
src/declarativeimports/plasmacomponents3/Button.qml | ||
37 ↗ | (On Diff #11076) | would this have evvect even if hoverEnabled: true is hardcoded that way? |
39 ↗ | (On Diff #11076) | ourselves.. it's in this patch |
41 ↗ | (On Diff #11076) | yes, in order to apply the font at the qqc2 label subclass we are using there if the user specified a custom font for this button |
- start a Plasma qqc2 style
- add labels and button
- add ToolButton
- add checkboxes and check delegates
- warnings--
- add ComboBox
- add Container and Control
- add Dialog, Popup
- add Drawer
- add Frame
- iadd GroupBox
- add RadioButton and RadioDelegate
- add a progressbar
- Menu and MenuItem
- add TabBar andTabButton
- animate transition
- layout fixes
- New controls
- add BusyIndicator
- Merge branch 'master' into mart/qqc2style
- install the components as a separate import
- separately install as style and import
- remove smooth property
- adapt to final api
- proper sizehints for the busy widget
- hardcoded numbers --
- use devicepixelratios
I want to do another full review next week when I'm home; but I'd be happy with doing that after we merge it, as it'll only be small things, and nothing which will affect API.
In general it all looks very good, I like that you split out ButtonShadow and all the rest seems neat. Good stuff.
src/declarativeimports/plasmacomponents3/Button.qml | ||
---|---|---|
28 | I don't get why we check background here and in other places. It would only be null if a user subclassed this and then overwrote the property to undefined, at which point they deserve errors. | |
51 ↗ | (On Diff #11076) | we do someting differently for widths and heights which isn't ideal, can we move the units.gridUnit * 1.6 into here as an implicitHeight? |
src/declarativeimports/plasmacomponents3/CheckIndicator.qml | ||
32 ↗ | (On Diff #11077) | why? |
src/declarativeimports/plasmacomponents3/GroupBox.qml | ||
34 ↗ | (On Diff #11077) | booo! |
src/declarativeimports/plasmacomponents3/Label.qml | ||
27 ↗ | (On Diff #11077) | Now we have an API break, lets fix this. As soon as you put this in a layout it won't do anything, so it's quite inconsistent. But I'd rather we didn't have it at all, we have to work round it in lots of places. If we want a padded Label, we can make a new subclass in the import with all the other headings. |
31 ↗ | (On Diff #11077) | Bhushan did a patch for P-F that changed this on the current label if we're on mobile, is that still possible? |
src/declarativeimports/plasmacomponents3/TextField.qml | ||
60 ↗ | (On Diff #11077) | can we kill an item here? background: FrameSvgItem { TextFiledFocus { anchors.fill:parent } } |
src/declarativeimports/plasmacomponents3/qmldir | ||
39 ↗ | (On Diff #11077) | maybe we don't want: ToolTip, Popup, Dialog, DialogButtonBox here and only in the style? Also which is the "correct" ItemDelegate we have one of them in PlasmaExtras |
Generally good to go. Just one major gripe about Label, and making sure our installed items and qmldir are in sync.
src/declarativeimports/CMakeLists.txt | ||
---|---|---|
11 ↗ | (On Diff #11080) | typo |
42 ↗ | (On Diff #11080) | It's easier to add something later than to remove it. I'd go with not installing for now, then revisit it. Maybe same for Menus. |
src/declarativeimports/plasmacomponents3/Label.qml | ||
27 ↗ | (On Diff #11077) | I still want this changed. |
src/declarativeimports/plasmacomponents3/qmldir | ||
11 ↗ | (On Diff #11630) | Dialog and DialogButtonBox are listed here but aren't installed. I think that will cause an error. And do we need CheckIndicator here? |
- don't install files that shouldn't
src/declarativeimports/CMakeLists.txt | ||
---|---|---|
42 ↗ | (On Diff #11080) | ok. what about the combobox? |
src/declarativeimports/plasmacomponents3/Button.qml | ||
28 | good point | |
src/declarativeimports/plasmacomponents3/Label.qml | ||
27 ↗ | (On Diff #11077) | the reason for the padding is that it makes it correctly vertically aligned with other widgets both in a Row and a RowLayout. |
src/declarativeimports/plasmacomponents3/qmldir | ||
11 ↗ | (On Diff #11630) | removed. |
src/declarativeimports/plasmacomponents3/Label.qml | ||
---|---|---|
27 ↗ | (On Diff #11077) | It probably made sense when it was written, but your comments about layouts is outdated. No manual changes are needed. try: If it was a problem we'd be seeing it all the time in the config modules. It is true for manual positioning and anchors, but I think the vast majority of the time where we'd want to be aligned to a button we'd be in a layout with that button. I'm sure there will be some changes needed, but it'll be few. (if I remove the line in my QQC1 label locally, I can't see any) In constrast run this on your workspace folder: and you'll see the number of times we have to reverse this logic. |
src/declarativeimports/plasmacomponents3/Label.qml | ||
---|---|---|
27 ↗ | (On Diff #11077) | so, GridLayout and RowLayout align correctly, while Row still doesn't. so, ok for me to remove it, and require *layouts are used instead of simple positioners everywhere |