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.
Details
Details
- Reviewers
filipf ngraham - Group Reviewers
Plasma VDG - Commits
- R119:d6731530bb43: Fixes for applet configuration layout.
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
Diff Detail
Diff Detail
- Repository
- R119 Plasma Desktop
- Branch
- configtop (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 22831 Build 22849: arc lint + arc unit
Comment Actions
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 | ||
|---|---|---|
| 124–125 | 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:  | |
| 185 | What's this for? | |
| desktoppackage/contents/configuration/AppletConfiguration.qml | ||
|---|---|---|
| 124–125 | 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. | |
| 185 | So that content does not touch the bottom edge, but was unsightly with double space.. | |
Comment Actions
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 | ||
|---|---|---|
| 124–125 |
I agree. I already tried doing it in D25728. | |
Comment Actions
Visually +1.
I'm starting to agree that this is the wrong place to add the titlebar separator.
Comment Actions
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.
Comment Actions
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.
Comment Actions
Restore code for vertical line.
Cannot find a reason this happens or replicate. But realized that the change was unnecessary. Did this fixed it?
Comment Actions
I'll give a bit more time, so @filipf gets a chance to check again and accept if possible.
Comment Actions
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)
Comment Actions
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.
Comment Actions
Fixes the regression. It's still possible to get this when resizing, but it was actually already like that before this patch.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.