[kcmkwin/options] Improve the look of the KWin options KCM UIs
ClosedPublic

Authored by GB_2 on Aug 31 2019, 6:40 PM.

Details

Summary

Apply the KDE HIG, use form layouts, make desktop files consistent and make the KCMs look better.





Test Plan

Open the Window Behavior KCMs. All options should still work

Diff Detail

Repository
R108 KWin
Branch
arcpatch-D23615_1
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 16143
Build 16161: arc lint + arc unit
GB_2 created this revision.Aug 31 2019, 6:40 PM
Restricted Application added a project: KWin. · View Herald TranscriptAug 31 2019, 6:40 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
GB_2 requested review of this revision.Aug 31 2019, 6:40 PM
zzag added a subscriber: zzag.Aug 31 2019, 6:51 PM

I think the first screenshot is not up to date because "Multiscreen behavior" stuff is missing. ;-)

GB_2 added a comment.Aug 31 2019, 6:53 PM
In D23615#523098, @zzag wrote:

I think the first screenshot is not up to date because "Multiscreen behavior" stuff is missing. ;-)

I only have one screen :-)

zzag added a comment.Aug 31 2019, 6:57 PM
In D23615#523100, @GB_2 wrote:

I only have one screen :-)

Oh, never mind then. :D

GB_2 updated this revision to Diff 65067.Aug 31 2019, 7:27 PM

Add focus warning message

I don't like that the detailed description of the focus policy is effectively lost. It's quite important.
VDG decided in another thread no-one uses whatsthis, which I kinda agree with.

kcmkwin/kwinoptions/main.cpp
92–93

Where does this object name get used?

kcmkwin/kwinoptions/mouse.cpp
261

why is this swapped?

GB_2 added a comment.Sep 3 2019, 11:49 AM

I don't like that the detailed description of the focus policy is effectively lost. It's quite important.
VDG decided in another thread no-one uses whatsthis, which I kinda agree with.

Yeah, I'll try to come up with something.

kcmkwin/kwinoptions/main.cpp
92–93

In the tab name.

kcmkwin/kwinoptions/mouse.cpp
261

I wanted to order the combobox items alphabetically or the default item to be first. I can revert this if you want or if it's not needed here.

GB_2 added inline comments.Sep 3 2019, 11:51 AM
kcmkwin/kwinoptions/main.cpp
92–93

Oops, sorry, that's not really shere it's used. I'll have to see where.

GB_2 updated this revision to Diff 65471.Sep 5 2019, 6:54 PM

Add focus policy description

GB_2 edited the summary of this revision. (Show Details)Sep 5 2019, 6:55 PM
GB_2 added inline comments.Sep 5 2019, 7:00 PM
kcmkwin/kwinoptions/main.cpp
92–93

I can't see where it is used, it's probably not visible anywhere.

ngraham added a subscriber: ngraham.Sep 5 2019, 7:28 PM

I wonder if using a KMessageWidget for the description might be appropriate. KWin already does this for the Compositor KCM.

zzag added inline comments.Sep 5 2019, 7:49 PM
kcmkwin/kwinoptions/windows.cpp
329–330

Why is it here?

GB_2 updated this revision to Diff 65495.Sep 6 2019, 8:01 AM

Use dynamically changing description label under combobox and remove unrelated change

GB_2 edited the summary of this revision. (Show Details)Sep 6 2019, 8:01 AM
GB_2 marked an inline comment as done.Sep 6 2019, 8:05 AM
GB_2 updated this revision to Diff 65496.EditedSep 6 2019, 8:06 AM

Use placeholder for window activation policy description label in UI file

GB_2 updated this revision to Diff 65497.Sep 6 2019, 8:08 AM

Don't translate window activation policy description label in UI file

zzag added inline comments.Sep 6 2019, 8:33 AM
kcmkwin/kwinoptions/windows.cpp
176

Style: a switch statement should look like

switch (foo) {
case Foo:
    break;
case Bar:
    break;
}

i.e. cases are not indented and opening brace is kept on the same line as switch keyword.

GB_2 updated this revision to Diff 65498.Sep 6 2019, 8:40 AM

Fix switch code style

GB_2 marked an inline comment as done.Sep 6 2019, 8:40 AM
davidedmundson added inline comments.Sep 8 2019, 8:33 AM
kcmkwin/kwinoptions/main.cpp
92–93

It won't be in the ui.

An object name is an internal ID.

Which means either its used in some code and changing it will break things....or its unused.

From what I could see it happens to be unused.

GB_2 added inline comments.Sep 8 2019, 8:34 AM
kcmkwin/kwinoptions/main.cpp
92–93

Should I keep this change then?

GB_2 updated this revision to Diff 65641.Sep 8 2019, 8:51 AM
GB_2 marked 6 inline comments as done.

Keep "KWin Moving" object name (just in case)

Cool, generally much better.

This could be further cleaned up:

kcmkwin/kwinoptions/actions.ui
26

Call this "Left click action"

66

Call this "Middle click action"

106

Call this "Right-click action"

kcmkwin/kwinoptions/mouse.ui
176

Call this "Left click action"

199

Call this "Middle click action"

212

Call this "Right click action"

GB_2 added inline comments.Sep 8 2019, 11:04 AM
kcmkwin/kwinoptions/actions.ui
26

I'll add the word action to the group box headers instead, so the labels aren't so long and there's less duplication.

GB_2 updated this revision to Diff 65649.Sep 8 2019, 1:18 PM

Address comments

GB_2 edited the summary of this revision. (Show Details)Sep 8 2019, 1:19 PM
GB_2 marked 7 inline comments as done.

Code is fine.
Needs VDG to sign off their side.

There are still a few more instances of using "button" instead of "click" on the Window Actions tab:

GB_2 updated this revision to Diff 65688.Sep 9 2019, 4:06 PM

Rename more "button" to "click"

ngraham accepted this revision.Sep 9 2019, 7:28 PM

Shipit!

This revision is now accepted and ready to land.Sep 9 2019, 7:28 PM
zzag accepted this revision.Sep 9 2019, 7:41 PM

Change the subject line to something like "[kcmkwin/options] Blah-blah"

GB_2 retitled this revision from Improve the look of the KWin options KCM UIs to [kcmkwin/options] Improve the look of the KWin options KCM UIs.Sep 9 2019, 7:44 PM
This revision was automatically updated to reflect the committed changes.