Polish Displays KCM UI
Needs ReviewPublic

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

Details

Reviewers
romangg
Group Reviewers
VDG
KWin
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

All functionality that was working before is still working now.

Diff Detail

Repository
R104 KScreen
Branch
clean-up-ui (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16338
Build 16356: arc lint + arc unit
ngraham created this revision.Mon, Sep 9, 2:06 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMon, Sep 9, 2:06 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Mon, Sep 9, 2:06 PM

I was not 100% sure about the method I chose to get the number of outputs, so I would appreciate if KWin folks could me know if this is wrong or there's a better way.

ngraham edited the summary of this revision. (Show Details)Mon, Sep 9, 2:22 PM
GB_2 added a subscriber: GB_2.Mon, Sep 9, 3:27 PM

+1

kcm/package/contents/ui/Screen.qml
65–66

Icon: "documentinfo"

72–73

"Center View"
Icon: "zoom-original"

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

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

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

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

+1

ngraham added a comment.EditedMon, Sep 9, 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.Mon, Sep 9, 4:00 PM
ngraham marked 2 inline comments as done.

Add icons to pushbuttons

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

...And also capitalize correctly

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

Use same slider style for the per-output Wayland version

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

Remove unnecessarily-added id for main layout

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

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.Mon, Sep 9, 5:58 PM
GB_2 added inline comments.Mon, Sep 9, 6:49 PM
kcm/package/contents/ui/main.qml
122

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.Mon, Sep 9, 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.Mon, Sep 9, 9:43 PM
ngraham marked 3 inline comments as done.

Address more review comments

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

element.count > 1

42

element.count > 1

49

element.count > 1

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

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.Tue, Sep 10, 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.Tue, Sep 10, 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.