Fixes for applet configuration layout.
ClosedPublic

Authored by gvgeo on Feb 23 2020, 8:31 PM.

Details

Summary

Fix applet configuration overlap with top line.
Change the margins to move scrollbar so that it touches the edge.
and use margins for content.
Set content to use available width.
Prevent vertical scrollbar from poping on/off internally.
Change the layout so that categories column can get the full height.
Remove 2 unused properties.
Fix bind loop for pageFlickable's width.
Fix title alignment for RTL.

Test Plan

Open desktop and audio configurations and make windows small.
Select audio category and move content to the top. (If there is no
scrollbar, increase and decrease height to exploit bug.)

Before:
At the right side (the main) scrollbar will have a margin, and overlap
with content.


Desktop categories stop above buttons cancel, ok.

Main content, and category highlight are displayed on top of the line.

After:
Scrollbar touch the edge, and the content has correct margin.

Categories height is not limited.

Main content , and category highlight(restored, see photo above instead for highlight) touch the line below.

Diff Detail

Repository
R119 Plasma Desktop
Branch
configscollcat (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23230
Build 23248: arc lint + arc unit
gvgeo created this revision.Feb 23 2020, 8:31 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 23 2020, 8:31 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
gvgeo requested review of this revision.Feb 23 2020, 8:31 PM
gvgeo edited the test plan for this revision. (Show Details)Feb 23 2020, 8:41 PM
gvgeo planned changes to this revision.Feb 24 2020, 4:19 PM

Will add more related changes here.

gvgeo updated this revision to Diff 76319.Feb 24 2020, 6:46 PM
gvgeo retitled this revision from Fix applet configuration ovelap with top line to Fixes for applet configuration layout..
gvgeo edited the summary of this revision. (Show Details)
gvgeo edited the test plan for this revision. (Show Details)

Added all the related planned changes.

gvgeo edited the summary of this revision. (Show Details)Feb 24 2020, 6:54 PM
gvgeo edited the test plan for this revision. (Show Details)
gvgeo updated this revision to Diff 76320.Feb 24 2020, 6:58 PM

style restore

gvgeo edited the test plan for this revision. (Show Details)Feb 24 2020, 6:59 PM
ngraham added subscribers: filipf, ngraham.

Looks quite sane overall, modulo a few questions and comments. Adding @filipf as a reviewer since I know he's done some work here too.

desktoppackage/contents/configuration/AppletConfiguration.qml
122–123

Hmm, mking the separator always span the full width introduces a row of pixels on top of the sidebar that is slightly unsightly when the top category is focused:

157

What's this for?

gvgeo added inline comments.Feb 25 2020, 6:13 AM
desktoppackage/contents/configuration/AppletConfiguration.qml
122–123

I can make this side to draw on top of the separator, as it was. Will be incorrect again.

Maybe separator must leave. It looks nice for breeze dark, but if the theme says no line, why force it? It is not forced in system settings. And despite if it looks okay or not for most setups, it is wrong to display for all themes.

157

So that content does not touch the bottom edge, but was unsightly with double space..
I will remove, it. It looks so much better with the categories coming from the edge, and scrollbar having the same length with the rectangular.

Looks good to me and solves 3 of the 4 binding loops, although this one still remains:

file:///usr/share/plasma/shells/org.kde.plasma.desktop/contents/configuration/AppletConfiguration.qml:296:25: QML StackView: Binding loop detected for property "height"

desktoppackage/contents/configuration/AppletConfiguration.qml
122–123

Maybe separator must leave

I agree. I already tried doing it in D25728.

gvgeo updated this revision to Diff 76356.Feb 25 2020, 11:59 AM

Categories touch top and bottom.
Removed 1 unnecessary line.

gvgeo edited the test plan for this revision. (Show Details)EditedFeb 25 2020, 12:07 PM

Can't have everything. now will cut line again.

gvgeo marked 2 inline comments as done.Feb 25 2020, 12:10 PM

Visually +1.

I'm starting to agree that this is the wrong place to add the titlebar separator.

gvgeo added a comment.Mar 3 2020, 8:06 AM

I'm starting to agree that this is the wrong place to add the titlebar separator.

Can this go as is, for now? I don't believe there is an alternative in the near future.

ngraham requested changes to this revision.Mar 3 2020, 4:31 PM

So with this patch, I see a new visual issue: the first time I open an applet config window, the vertical separator is not visible:

It becomes it becomes visible (and thereafter stays visible) if I resize the window or switch categories. I don't see this issue with the current state of git master, so it seems to be a regression.

This revision now requires changes to proceed.Mar 3 2020, 4:31 PM
gvgeo updated this revision to Diff 76869.EditedMar 3 2020, 5:45 PM
gvgeo edited the test plan for this revision. (Show Details)

Restore code for vertical line.
Cannot find a reason this happens or replicate. But realized that the change was unnecessary. Did this fixed it?

ngraham accepted this revision.Mar 3 2020, 6:26 PM

Yep! Everything else looks good to me.

This revision is now accepted and ready to land.Mar 3 2020, 6:26 PM
gvgeo marked an inline comment as done.Mar 3 2020, 6:38 PM

I'll give a bit more time, so @filipf gets a chance to check again and accept if possible.

filipf added a comment.Mar 3 2020, 7:36 PM

Seems to run pretty well, but there is a tiny padding regression introduced.

Using plasmashell --reverse --replace to test a right to left layout we see that there is excessive padding between the content and the sidebar.

I.e. there is more padding than when using a left to right layout, and when resizing the window the padding actually increases some more (to level that's shown in the screenshot)

gvgeo updated this revision to Diff 76908.Mar 4 2020, 8:58 AM
gvgeo marked an inline comment as not done.

Fixed margins.
Unfortunatly the RTL layout has some issues, had to disable the use of available width for RTL layout, to stop the jumping from the scrollview.

filipf accepted this revision.Mar 4 2020, 9:08 AM

Fixes the regression. It's still possible to get this when resizing, but it was actually already like that before this patch.

gvgeo updated this revision to Diff 76910.Mar 4 2020, 9:16 AM

Cancel button was touching the edge in RTL.

gvgeo added a comment.Mar 4 2020, 9:24 AM

Strange, I don't get the resizing problem as it is now, or in master. Only managed to get this space by changing the theme, and was immediately fixed with resize.

Thanks Filip for double checking.

filipf added a comment.Mar 4 2020, 9:29 AM

Strange, I don't get the resizing problem as it is now, or in master. Only managed to get this space by changing the theme, and was immediately fixed with resize.

Thanks Filip for double checking.

No problem. It needs to be a specific kind of resize action I guess.

I'm using Kvantum but I can make it with happen Breeze as well. We probably also need to change heading so it's aligned to the right, but both of these things are material for a different patch.

Yeah, this seems fine to land as-is. :)

gvgeo added a comment.Mar 4 2020, 6:57 PM

There is a problem, I still try to fix...

gvgeo updated this revision to Diff 77017.EditedMar 5 2020, 1:05 PM

Prevent vertical scrollbar from popping on/off internally.
Fix title alignment for RTL.
Set RTL content to use available width .

I tried splitting into two patches, to separate the height loop changes, but could not find a good point to cut the patch.
All the loops messages are caused from the pageStack height. The loop already exist, but is more easily detected with the ScrollBar.vertical.policy.
The big spacing in the video is a FormLayout issue, among many other things. Now is obvious that width does not always refresh. For example, when expanding window vertical only, the width changing from the scrollbar(that disappears) does not update.

Setting scrollbar policy is not exactly correct. A better way would be to allow the content to choose between maxHeight vs contentHeight vs implicitHeight.

gvgeo edited the summary of this revision. (Show Details)Mar 5 2020, 1:07 PM
gvgeo edited the summary of this revision. (Show Details)
filipf accepted this revision.Mar 5 2020, 2:11 PM

Good stuff

gvgeo updated this revision to Diff 77046.Mar 5 2020, 3:38 PM

Remove title alingment for kirigami change.

This revision was automatically updated to reflect the committed changes.