D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Filip Fila]

filipf updated this revision to Diff 54460.
filipf added a comment.


  Initial implementation of twinFormLayouts

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19873?vs=54288=54460

BRANCH
  fix-hor-alignment (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19873

AFFECTED FILES
  wallpapers/image/imagepackage/contents/ui/config.qml

To: filipf, #plasma, ngraham
Cc: davidre, mart, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Marco Martin]

mart added a comment.


  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 project, but if you would like to 
give a try it may be finally the properfix(tm)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: davidre, mart, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Filip Fila]

filipf added a comment.


  In D19873#434571 , @davidre wrote:
  
  > 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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: davidre, mart, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[David Redondo]

davidre added a comment.


  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?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: davidre, mart, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Filip Fila]

filipf added a comment.


  In D19873#434568 , @davidre wrote:
  
  > Mhm if I change it to use units like Color instead of Kirigami units it 
seems aligned properly:
  >  F6702138: grafik.png 
  
  
  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: 
  F6702149: image.png 
  
  F6702152: image.png 

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: davidre, mart, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[David Redondo]

davidre added a comment.


  Mhm if I change it to use units like Color instead of Kirigami units it seems 
aligned properly:
  F6702138: grafik.png 

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: davidre, mart, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[David Redondo]

davidre added a comment.


  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: F6702121: grafik.png  color: 
F6702124: grafik.png 
  Color seems to do:
  
Row {
spacing: units.largeSpacing / 2

QtControls.Label {
width: formAlignment - units.largeSpacing
[...]
}
KQuickControls.ColorButton {

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: davidre, mart, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Filip Fila]

filipf added a subscriber: mart.
filipf added a comment.


  @mart would you happen to know how to trick one FormLayout into thinking its 
content is as wide as another FormLayout's content?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: mart, abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Filip Fila]

filipf added a comment.


  In D19873#434468 , @ngraham wrote:
  
  > In D19873#434441 , @filipf wrote:
  >
  > > 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`

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Nathaniel Graham]

ngraham added a comment.


  In D19873#434441 , @filipf wrote:
  
  > 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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Filip Fila]

filipf added a comment.


  In D19873#434426 , @ngraham wrote:
  
  > In D19873#434232 , @filipf wrote:
  >
  > > 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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Nathaniel Graham]

ngraham added a comment.


  In D19873#434232 , @filipf wrote:
  
  > 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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Andres Betts]

abetts added a comment.


  +1 visually

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: abetts, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Filip Fila]

filipf added a comment.


  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` ?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Filip Fila]

filipf added a comment.


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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D19873

To: filipf, #plasma, ngraham
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19873: [image-wallpaper] Fix horizontal alignment of the "Positioning:" row

[Filip Fila]

filipf created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
filipf requested review of this revision.

REVISION SUMMARY
  Master currently has bad horizontal alignment of the "Positioning:" row since 
it's not a part of the FormLayout as everything else.
  This patch tweak the values a bit so it's at least horizontally aligned.
  
  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.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  fix-hor-alignment (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D19873

AFFECTED FILES
  wallpapers/image/imagepackage/contents/ui/config.qml

To: filipf
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart