Polish Displays KCM UI
ClosedPublic

Authored by ngraham on Sep 9 2019, 2:06 PM.

Details

Reviewers
romangg
GB_2
Group Reviewers
VDG
KWin
Maniphest Tasks
T7254: Displays (WIP?)
Commits
R104:d0dc9a7a1c1e: Polish Displays KCM UI
Summary

This patch improves and polishes the new KScreen KCM's UI in a variety of ways. A picture
says a thousand words, so look at the pictures. :)

BUG: 348126
FIXED-IN: 5.17.0

Test Plan

Due to an unrelated bug, my second screen isn't showing up in the visualization, so please excuse the fact that only one appears there in these screenshots:

All functionality that was working before is still working now.

Diff Detail

Repository
R104 KScreen
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
GB_2 added inline comments.Sep 9 2019, 3:27 PM
kcm/package/contents/ui/Screen.qml
65–66

Icon: "documentinfo"

73–74

"Center View"
Icon: "zoom-original"

ndavis added a subscriber: ndavis.Sep 9 2019, 3:35 PM

What are "Values of an output"? I don't understand what these 2 settings do.

filipf added a subscriber: filipf.Sep 9 2019, 3:42 PM

What are "Values of an output"? I don't understand what these 2 settings do.

+1

ngraham added a comment.EditedSep 9 2019, 3:42 PM

What are "Values of an output"? I don't understand what these 2 settings do.

I agree that this part is rather confusing.

@romangg can confirm, but as far as I understand it, this is asking you whether you want the settings for the current display (resolution, rotation, etc) to always be applied whenever this display is in use, or only to take effect when this display is in the current display arrangement.

For example, maybe you want to rotate your laptop's screen only when it is plugged into an external display, so you can put the laptop vertically and have more vertical space.

If we can come up with a better way to express this, that would be awesome.

ngraham updated this revision to Diff 65683.Sep 9 2019, 4:00 PM
ngraham marked 2 inline comments as done.

Add icons to pushbuttons

ngraham updated this revision to Diff 65684.Sep 9 2019, 4:01 PM

...And also capitalize correctly

ngraham updated this revision to Diff 65685.Sep 9 2019, 4:04 PM

Use same slider style for the per-output Wayland version

ngraham updated this revision to Diff 65687.Sep 9 2019, 4:06 PM

Remove unnecessarily-added id for main layout

ngraham added inline comments.Sep 9 2019, 4:06 PM
kcm/package/contents/ui/main.qml
125

And I could use some help with the issue detailed in the comment here.

GB_2 retitled this revision from Polish KCM UI to Polish Displays KCM UI.Sep 9 2019, 5:58 PM
GB_2 added inline comments.Sep 9 2019, 6:49 PM
kcm/package/contents/ui/main.qml
125

You don't need any of that. Just do kcm.outputModel.count > 1. BTW, I'd also use a different minimum height if this component is hidden.

GB_2 added a comment.Sep 9 2019, 6:55 PM

I have a suggestion for the "save output properties" option.

kcm/package/contents/ui/Panel.qml
90

"Save output properties:"

96

"For any setup"

103

"Only for this specific setup"

ngraham updated this revision to Diff 65706.Sep 9 2019, 9:43 PM
ngraham marked 3 inline comments as done.

Address more review comments

ngraham edited the test plan for this revision. (Show Details)Sep 9 2019, 9:48 PM
GB_2 added inline comments.Sep 10 2019, 1:55 PM
kcm/package/contents/ui/OutputPanel.qml
33

element.count > 1

43

element.count > 1

50

element.count > 1

kcm/package/contents/ui/main.qml
125

Or even better: outputs.count > 1

Did you test the patch with those suggestions? They don't work.

ngraham updated this revision to Diff 65779.Sep 10 2019, 3:39 PM

Reduce minimum height of screen grid a tiny bit so there's no vertical scrollbar in System Settings when showing multiple displays

GB_2 added a comment.Sep 10 2019, 3:46 PM

Did you test the patch with those suggestions? They don't work.

Oh, sorry then. I didn't actually have two screens to test this. It just looked like it worked for one.

Yeah, all the things that I would have expected to work didn't, which is why I went with the nuclear option of creating a new function exposed to the QML side. Definitely needs input from KWin people or @romangg. Everyone's probably busy at Akademy right now though, so it can wait a bit.

Reducing the screen area height makes it difficult to place outputs on top of each other. Making it a bit smaller is fine (and improving the logic of how much this it in comparison to overall kcm size) but I think this is too much. Alternatively the size of the outputs could be reduced.

kcm/package/contents/ui/OutputPanel.qml
33

element or model is not the whole model but only the current element of the model displayed by the view delegate. So it's a single element of the model without a count.

What you need is to take the count on the overall model. As the OutputPanel is part of Panel you can use for example here panelView.count. This way an additional getter is not required. Also you could use OutputModel::rowCount as in: kcm.outputModel.rowCount() (it is already Q_INVOKABLE, see docs).

What I'm seeing in the screenshot is that the two form layouts aren't aligned. Can you interlink them with twinFormLayouts?

ngraham updated this revision to Diff 66265.Sep 16 2019, 8:48 PM
ngraham marked an inline comment as done.

Remove unneeded accessor and use kcm.outputModel.rowCount() instead

ngraham marked 3 inline comments as done.Sep 16 2019, 8:49 PM
ngraham updated this revision to Diff 66266.Sep 16 2019, 9:16 PM
ngraham marked 3 inline comments as done.

Fix the model count binding in main.qml

ngraham updated this revision to Diff 66267.Sep 16 2019, 9:17 PM

Fix FormLayout alignment

ngraham updated this revision to Diff 66269.Sep 16 2019, 10:00 PM
  • Reduce height of screen visualizations
  • Conditionalize height of kcmshell window
  • Harmonize paddings
ngraham edited the test plan for this revision. (Show Details)Sep 16 2019, 10:02 PM

Need a hand with the TODO: in main.qml. Otherwise, this is ready for re-review, @romangg and VDG

The orientation toolbuttons seem oversized.

romangg added a comment.EditedSep 17 2019, 7:28 AM

Need a hand with the TODO: in main.qml.

This can't be really fixed since the config and by that the output model is received asynchronously later on and to my knowledge the KCM's implicitHeight must be set directly in the beginning.

To be honest I'm no friend of hiding the preview/overview when there is only one output connected. Reasons are that people are used to seeing it and that if a second monitor is connected/removed suddenly some huge ass control pattern gets added/removed. This will also always compromise the current window size when connecting a second monitor since the window will be too small suddenly. Also imo it brings value to have a miniature depiction of the output setup even if there is only one. Instead we could think of making the monitor in the overview not movable when only one monitor is connected.

The orientation toolbuttons seem oversized.

That's an issue with Kirigami styling as in my opinion with many other white space inconsistencies when looking at FormLayouts. What could be improved is that not the whole button should be rotated but only the icon such that the border on hover and shading do not change the orientation.

romangg added inline comments.Sep 17 2019, 7:56 AM
kcm/package/contents/ui/Output.qml
49

You can't change these parameters here. Afterwards the positioning will be broken. You can multiply the screen.relativeFactor directly but I would look out for keeping anything in the outputs still readable. Multiplying it with 2 gives me only few letters in the output depiction anymore.

romangg added inline comments.Sep 17 2019, 8:09 AM
kcm/package/contents/ui/Panel.qml
77

Options below does not only affect the currently shown output above but all outputs in the current configuration. I don't think this is clear anymore with only the horizontal line.

90

This option does not only impact the currently shown output above but all others as well. Therefore the plural displays/outputs would be more correct than a singular "display".

Also a question is if we want to call it display, output, screen or monitor in user facing texts and then do it everywhere like this. I assume there are pros and cons for all of these nouns.

ngraham updated this revision to Diff 66301.Sep 17 2019, 3:27 PM
ngraham marked 3 inline comments as done.

Address all outstanding review comments

ngraham edited the test plan for this revision. (Show Details)Sep 17 2019, 3:28 PM
ngraham updated this revision to Diff 66302.Sep 17 2019, 3:33 PM

Make visualization width smarter so it matches the layout's width better

ngraham edited the test plan for this revision. (Show Details)Sep 17 2019, 3:34 PM
ngraham added inline comments.
kcm/package/contents/ui/Panel.qml
77

I think the fact that the slider's label says "Global scale" communicates that it's a global setting. As for the other control in this group, I think we can make it clearer with better wording.

90

"Output" is a bit too technical I think, and"Monitor" doesn't cover the case of projectors. Personally I like "Display" but I'm open to being convinced otherwise.

GB_2 added inline comments.Sep 17 2019, 3:37 PM
kcm/package/contents/ui/Panel.qml
90

+1 for display

ngraham updated this revision to Diff 66303.Sep 17 2019, 3:46 PM

Unify name and caption between desktop file and kaboutdata

ngraham edited the test plan for this revision. (Show Details)Sep 17 2019, 3:46 PM
GB_2 accepted this revision.Sep 17 2019, 4:13 PM

UI LGTM!

This revision is now accepted and ready to land.Sep 17 2019, 4:13 PM

Thanks! Needs final approval from @romangg or someone else from KWin before landing.

GB_2 added a comment.Sep 17 2019, 4:18 PM

Thanks! Needs final approval from @romangg or someone else from KWin before landing.

Of course.

romangg accepted this revision.Sep 17 2019, 7:13 PM
romangg added inline comments.
kcm/package/contents/ui/Panel.qml
44

visible: count > 1

ngraham updated this revision to Diff 66328.Sep 17 2019, 7:21 PM
ngraham marked an inline comment as done.

Address final review comment

This revision was automatically updated to reflect the committed changes.