Fix window height of Screen Locking KCM
ClosedPublic

Authored by tigrang on Mar 28 2019, 6:31 AM.

Details

Summary

An implicitHeight must be set on QML components to prevent window size being too small.

BUG: 400355
FIXED-IN: 5.16.0

Test Plan
  1. Open Screen Locking settings
  2. Go to Appearance tab
  3. Two rows of thumbnails with labels should be shown

Diff Detail

Repository
R133 KScreenLocker
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
tigrang created this revision.Mar 28 2019, 6:31 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 28 2019, 6:31 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
tigrang requested review of this revision.Mar 28 2019, 6:31 AM
davidedmundson accepted this revision.Mar 28 2019, 9:14 AM
This revision is now accepted and ready to land.Mar 28 2019, 9:14 AM
filipf added a subscriber: filipf.Mar 28 2019, 12:08 PM

Good fix, the lack of implicitHeight was leading to confusion before if there are even any wallpapers present.

One question though: wouldn't it be better to define the height in units.gridUnit instead of raw pixels, or even absolutely by doubling the height of one grid item? From my understanding, when scaling is changed the grid items are also resized so the window won't open showing 2 rows of wallpapers anymore, as was desired.

@filipf Here's a diff with units.gridUnit https://phabricator.kde.org/differential/diff/54998/ which doesn't quite work out to 2 rows for every scaling factor but is a bit better than hard-coded pixel size. I'm not sure how to do doubling the height of one grid item.

Hardcoding pixel size won't impact scaling.


You will never get it spot on two rows with any combination of pixel size or units or whatever you add here. The correct size is dependent on the exact children.
The only way you'll get it to be correct is to bind this stack view to currentItem.implicitHeight and making sure that the implicit size propagates upwards correctly.

@tigrang would you like this landed now, or do you wanna try @davidedmundson's idea?

Thanks for the patch!

ngraham accepted this revision.Mar 30 2019, 7:27 PM

Let's land this now and then you can try the other approach in another patch (if you want to).

ngraham edited the summary of this revision. (Show Details)Mar 30 2019, 7:29 PM
This revision was automatically updated to reflect the committed changes.

@ngraham Sorry for the late response.

Let's land this now and then you can try the other approach in another patch (if you want to).

Sounds good.