[image-wallpaper] Port to Kirigami.FormLayout and use twinFormLayouts
ClosedPublic

Authored by filipf on Mar 18 2019, 11:39 PM.

Details

Summary

Porting to Kirigami.FormLayout and using twinFormLayouts in order to ensure alignment with the main layout.

NOTE: Vertical spacing is still clearly wrong, but I could fix that as well if someone could offer a suggestion on how to do it.
Test Plan

D19932 needed before testing.

Before:

After:

Before:

After:

Everything still worked.

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10369
Build 10387: arc lint + arc unit
filipf created this revision.Mar 18 2019, 11:39 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 18 2019, 11:39 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
filipf requested review of this revision.Mar 18 2019, 11:39 PM
filipf edited the test plan for this revision. (Show Details)Mar 18 2019, 11:40 PM
filipf added reviewers: Plasma, ngraham.
filipf edited the test plan for this revision. (Show Details)

I think I'm still off by a pixel, will test better tomorrow.

ngraham requested changes to this revision.Mar 18 2019, 11:56 PM

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 revision now requires changes to proceed.Mar 18 2019, 11:56 PM

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 ?

abetts added a subscriber: abetts.Mar 19 2019, 2:09 PM

+1 visually

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.

filipf added a comment.EditedMar 19 2019, 2:54 PM

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.

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.

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.

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.

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

filipf added a subscriber: mart.Mar 19 2019, 3:47 PM

@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:

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:

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?

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.

mart added a comment.Mar 20 2019, 9:41 AM

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)

filipf updated this revision to Diff 54460.Mar 21 2019, 12:09 AM

Initial implementation of twinFormLayouts

filipf retitled this revision from [image-wallpaper] Fix horizontal alignment of the "Positioning:" row to [image-wallpaper] WIP: Port to Kirigami.FormLayout and use twinFormLayouts.Mar 21 2019, 12:10 AM
filipf added a comment.EditedMar 21 2019, 12:14 AM
In D19873#434949, @mart wrote:

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.

filipf edited the test plan for this revision. (Show Details)Mar 21 2019, 12:16 AM

Great! I see that also fixes the issue that the label vanished when the window got to narrow.

filipf updated this revision to Diff 54484.Mar 21 2019, 1:46 PM

port everything

filipf retitled this revision from [image-wallpaper] WIP: Port to Kirigami.FormLayout and use twinFormLayouts to [image-wallpaper] Port to Kirigami.FormLayout and use twinFormLayouts.Mar 21 2019, 1:56 PM
filipf edited the summary of this revision. (Show Details)
filipf edited the test plan for this revision. (Show Details)

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.

filipf edited the test plan for this revision. (Show Details)Mar 21 2019, 2:06 PM
filipf updated this revision to Diff 54554.Mar 22 2019, 12:57 PM

make the "Folders" string translatable

ngraham accepted this revision.Mar 25 2019, 12:18 AM

This works great with D19932. Definitely seems like the correct fix. The rest of the code changes look good too.

This revision is now accepted and ready to land.Mar 25 2019, 12:18 AM
filipf updated this revision to Diff 54790.Mar 25 2019, 5:49 PM

lose the unecessary QtQuick.Controls 1 import

filipf updated this revision to Diff 54797.EditedMar 25 2019, 6:24 PM

Don't break the slideshow wallpaper plugin by putting everything in a form layout

ngraham accepted this revision.Mar 26 2019, 1:12 PM
filipf added a comment.Apr 2 2019, 2:39 PM

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.

hein added a subscriber: hein.Apr 2 2019, 4:47 PM

Looks solid to me. Is your concern re vertical spacing still valid?

In D19873#442385, @hein wrote:

Looks solid to me. Is your concern re vertical spacing still valid?

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.

mart accepted this revision.Apr 3 2019, 5:56 PM

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

hein accepted this revision.Apr 3 2019, 5:59 PM
This revision was automatically updated to reflect the committed changes.