Porting to Kirigami.FormLayout and using twinFormLayouts in order to ensure alignment with the main layout.
Details
- Reviewers
ngraham mart hein - Group Reviewers
Plasma - Commits
- R120:4516ab53726c: [image-wallpaper] Port to Kirigami.FormLayout and use twinFormLayouts
Diff Detail
- Repository
- R120 Plasma Workspace
- Branch
- fix-hor-alignment (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 10368 Build 10386: arc lint + arc unit
The spacing change is clearly correct. As for the width change, we should match the exact calculation used for the other combobox we're trying to match, which is width: theme.mSize(theme.defaultFont).width * 24. Otherwise they'll drift out of sync when formAlignment changes or the next time Kirigami.Units.largeSpacing is modified.
This is just theoretical, but what are the obstacles with adding this whole row to the actual FormLayout file and just setting visible: when_image_wallpaper_is_used ?
Because the wallpaper chooser is plugin-based, and the image wallpaper is just one plugin out of many. In principle a wallpaper plugin could have whatever UI it wants, and its configuration UI has to live in a different place. It's just a visual trick that they look integrated together in the page.
It seems like we're doing too many tricks. We even have to vertically align the label with the combobox because we're using a Row instead of RowLayout. In this file, why not just do:
Kirigami.FormLayout { width: MainFormLayout.width // grab geometry from ConfigurationContainmentAppearance.qml so that this form layout thinks it's working with the same elements as the main one Combobox { Kirigami.FormData.label: ("Positioning:") } }
This would make the code much cleaner, I just have to figure out how trick the image wallpaper's FormLayout into thinking its form is as wide as the main one's. It would also probably mean we don't lose the label when window side is minimum and main FormLayout becomes left aligned.
That's why. :) They come from different sources, so the plugin's formLayout doesn't have access to the width data for the chooser's formLayout. However if you can figure out how to make this work, I think that would definitely be the ideal solution because then it wouldn't need any hacks.
Good news! config.qml is already grabbing property int formAlignment from ConfigurationContainmentAppearance.qml so snatching something is not an issue. It's just a matter of figuring out which property to take and to then use for our FormLayout in config.qml
@mart would you happen to know how to trick one FormLayout into thinking its content is as wide as another FormLayout's content?
Maybe it would be worth it to check out how the other wallpaper plugins that don't suffer from this issue align their controls ?
potd:
Color seems to do:
Row { spacing: units.largeSpacing / 2 QtControls.Label { width: formAlignment - units.largeSpacing [...] } KQuickControls.ColorButton {
Mhm if I change it to use units like Color instead of Kirigami units it seems aligned properly:
The spacing in the row is still too big though. Notice how ":" is more left than above. But it's like this in 5.15 anyway. I think you uncovered what messed up the alignment - the port to Kirigami units.
This aligns everything nicely:
It seems it was changed in this commit to explicitly use Kirigami units: 98d9f681a37e2ac2feb6bf5cb5e8a54f4c7e874e
Was the Color wallpaper forgotten or did this reason not apply to it? Should we revert it?
I think the rationale was "we're already importing Kirigami, so let's not import PlasmaCore as well solely for its units when Kirigami can provide units". As for why it wasn't ported elsewhere, maybe those plugins weren't using Kirigami yet.
Bottom line though is we can either tweak Kirigami's units or just go back to the old ones (although they need tweaking as well). It's more or less the same to me, the best solution would be to use FormLayout in specific plugins.
So,
FormLayout has api to align two or more of them with each other, which is the list property FormLayout.twinFormLayouts
I wonder if there is a way to make the parent and child formlayouts visible ot each other to use such property.
the parent is in plasma-desktop/desktoppackage/contents/configuration/ConfigurationContainmentAppearance.qml, loading the individual wallpapers config in the stackview at line 162.
so, we could try something like:
the wallpaper configs will expose a formLayout property.
the main formLayout has something like
twinFormLayouts: stack.item && stack.item.formLayout ? [stack.item.formLayout] : []
then the parent layout may be i guess injected at instantiation as a property of the wallpaper plugin, so the child formlayout can set twinformlayouts as well
this is all untested, so is a bit of R&D project, but if you would like to give a try it may be finally the properfix(tm)
This works!!! Thank you :D
For tonight I can only offer a sneak peak: the port for this "Positioning:" row, as well as the groundwork in D19932. But if the code is right we could port all of the wallpaper plugins pretty fast.
EDIT: Oh yes, the combobox width is busted now though.
Great! I see that also fixes the issue that the label vanished when the window got to narrow.
I've done a lot of clean up because: (1) some code is not needed for the FormLayout; (2) things worked the same without some code; (3) ported the combobox to QQC2 because the main layout is also using QQC2 comboboxes and everything still fits with normal scaling.
Let me know if I've gone too far.
This works great with D19932. Definitely seems like the correct fix. The rest of the code changes look good too.
I think it would be good if someone from Plasma could have a look if this is okay, I did do a lot of modifications.
Thanks for having a look. Vertical spacing in wallpaper plugins is now OK, but unfortunately the spacing is always too big where the master layout and the individual wallpaper plugin meet:
I tried removing a bunch of items in both files thinking they may be be adding to the height of the form, but it didn't seem to help.
I think the patch is fine, the extra spacing is probably in the shell package
plasma-desktop/desktoppackage/contents/configuration/ConfigurationContainmentAppearance.qml
you can take a look there and see where things are
maybe gammaray is also helpful in it