Rewrite KScreen KCM as ConfigModule with outputs model and Kirigami
Needs RevisionPublic

Authored by romangg on Jul 15 2019, 2:16 AM.

Details

Summary

The KScreen KCM was still partly Qt Widgets based. Also it did not use a
model for the outputs but instead created objects in C++ for visual
representation of the outputs.

This patch bases the KScreen KCM on ConfigModule removing any dependency
on Qt Widgets. The former widgets based settings form is rewritten as
Kirigami form layout.

The already existing QML part has been also rewritten with a model as
data provider for the outputs and input handlers for interacting with the
outputs in the overview.

On hotplug events the settings dialog is reset and the user is informed.

Output retention now is set for the whole config instead of individual
outputs what simplifies it.

On Wayland output scaling is now fractional via a slider. This has not yet
been tested though and there might be some more work necessary in KWin.

The output unification has been removed for now. It was broken in certain
ways and we should try instead to improve the libkscreen backend to make
unficiation more easy to use. For now as a workaround the outputs can be
placed on top of each other to create a clone effect.

The KScreen maintainership is changed to me.

X11:

Wayland:

Test Plan

Manually on X and Wayland.

Diff Detail

Repository
R104 KScreen
Branch
qmlize
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14821
Build 14839: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Plasma. Β· View Herald TranscriptJul 15 2019, 2:16 AM
Restricted Application added a subscriber: plasma-devel. Β· View Herald Transcript
romangg requested review of this revision.Jul 15 2019, 2:16 AM
romangg edited the summary of this revision. (Show Details)Jul 15 2019, 2:19 AM
filipf added a subscriber: filipf.Jul 15 2019, 8:03 AM

Looks better, in particular I like the approach with buttons for orientation because it saves the second or two of having to remember what's clockwise or counter-clockwise.

Only some minor things I noticed in the screenshots:

  • can the margins on the view area be increased so that it's in line with where the text begins (red is what I'd remove)?

  • for every form data label I'd add ":" at the end
  • in the case of sliders I would simply add the label to the slider, not the column layout

Pretty cool!

kcm/kcm.cpp
49

Given this is for the KCM only, I would suggest to name the import org.kde.private.kcm.kscreen

kcm/output_model.cpp
46

switch?

158

You could also initialize it with QAbstractItemModel::roleNames() (which has "display") and then add yours to it

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

i18n and everywhere else

42

Try Kirigami.FormData.checkable: true to make the checkbox to the left instead of having a lonely tiny checkbox floating there

50

Use onClicked which only fires when the user explicitly clicks it, not in case a property/model role changes on its own, also everywhere else

57

When you have loads of resolutions, we change the Slider for a ComboBox. I don't really like that but a slider could be fiddly to operate with many steps

112

I think you might want to use a ButtonGroup here to make them semantically mutally exclusive (just a suggestion)

115

Please use enum names here instead of magic numbers

139

You sure this combobox should be full width?

141

They lack formatting (decimal separator and "Hz" suffix)

142

Why not just bind those properties directly?

143

Use onActivated which only fires when the user explicitly changes it

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

Have you tried twinFormLayouts?

89

Why is "Scale" under arrangement?

99

Can we make it not show all the tick marks but only the integer ones like in the current scale UI?

102

Use onMoved which blabla, see onClicked and onActivated

106

You can pretty-format the number like this:

globalScaleSlider.value.toLocaleString(Qt.locale(), "f", 1)

(If that doesn't work wrap it in Number(...).toLocaleString(...))

123

onClicked
I also think the operator priorities are a bit mixed up here

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

Don't use PlasmaComponents stuff in widget-like dialogs.
Instead, you want to be using QQC2.Button and then overriding its contentItem (I think). It is a bit convoluted as qqc2-desktop-style paints the entire button as background

ooooor you just turn the button on its side like you do below :D

The Scale slider UI position is different in X11 and Wayland. Is this intentional?

The Scale slider UI position is different in X11 and Wayland. Is this intentional?

Yea, on Wayland we can set the scale for each output individually. On X11 there is only a single scale factor to be set. But I plan to rename the slider label in the X11 case to Global scale to make this distinction more clear.

This looks really nice,

one question, having current values under the Slider does it follow a HIG rule or is it a proposition?

Looks better, in particular I like the approach with buttons for orientation because it saves the second or two of having to remember what's clockwise or counter-clockwise.

Only some minor things I noticed in the screenshots:

  • can the margins on the view area be increased so that it's in line with where the text begins (red is what I'd remove)?

Do you know if there is an "official" margin value for that?

This looks really nice,

one question, having current values under the Slider does it follow a HIG rule or is it a proposition?

I copied this style from Night Color KCM. I didn't consult the HIG on that and to be honest I don't really like the look of it. Lone numbers feels unconnected to the slider and margins are kind of messed up. Imo slider components should provide an interface to add current value labels to them in a convenient and standardized fashion. Maybe as a Kirigami component?

This looks really nice,

one question, having current values under the Slider does it follow a HIG rule or is it a proposition?

I copied this style from Night Color KCM. I didn't consult the HIG on that and to be honest I don't really like the look of it. Lone numbers feels unconnected to the slider and margins are kind of messed up. Imo slider components should provide an interface to add current value labels to them in a convenient and standardized fashion. Maybe as a Kirigami component?

I agree it looks weird, especially because it breaks visual vertical alignment with the label on the left.

@ngraham is there any HIG proposition how current values of sliders should be positioned?

Yeah I may have improvised that with the Night Color KCM 😊

The HIG way (and looking at other places) is to put the value to the right of the slider.

Looks better, in particular I like the approach with buttons for orientation because it saves the second or two of having to remember what's clockwise or counter-clockwise.

Only some minor things I noticed in the screenshots:

  • can the margins on the view area be increased so that it's in line with where the text begins (red is what I'd remove)?

Do you know if there is an "official" margin value for that?

Looks like smallSpacing but not quite so I'm going to check it out live and report back.

romangg added inline comments.Jul 15 2019, 9:50 AM
kcm/package/contents/ui/OutputPanel.qml
42

i18n and everywhere else

I saw in other KCMs the Kirigami label being set without the i18n macro and I hoped Kirigami would do this internally. But apparently not, so I will add them.

142

When I try it with model.refreshRateIndex directly it takes the ComboBox model and not the output model. Is there another solution? Thinking of it I could add a outputModel property to the whole OutputPanel component and bind to that here then instead. Does this sound better?

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

It's the global scale on X11 affecting every output in the arrangement and not only a single one. On X11 there are no scale values per output. Note to self: Change this label to "Global scale".

Yeah I may have improvised that with the Night Color KCM 😊

The HIG way (and looking at other places) is to put the value to the right of the slider.

Looks better, in particular I like the approach with buttons for orientation because it saves the second or two of having to remember what's clockwise or counter-clockwise.

Only some minor things I noticed in the screenshots:

  • can the margins on the view area be increased so that it's in line with where the text begins (red is what I'd remove)?

Do you know if there is an "official" margin value for that?

Looks like smallSpacing but not quite so I'm going to check it out live and report back.

Couldn't get any changes shown with kdesrc-build, but I zoomed in on the screenshot and it's 6px of space so smallSpacing * 1.5

On X11 I had problems moving the outputs in the overview. For some reason the click was not caught by the ui but KWin tries to move the whole window instead. On Wayland everything works fine. Might be a Qt bug in input handlers directly and not the code here. Other ideas?

On Wayland everything works fine. Might be a Qt bug in input handlers directly and not the code here. Other ideas?

It'll be our code. Try running with --style=fusion and see if the problem magically goes away.

If so, it'll be us not accepting the input event, so it propagates up to the window, and hits the QWidget underneath the QML which starts a system move. Something that doesn't work on wayland.

On Wayland everything works fine. Might be a Qt bug in input handlers directly and not the code here. Other ideas?

It'll be our code. Try running with --style=fusion and see if the problem magically goes away.

If so, it'll be us not accepting the input event, so it propagates up to the window, and hits the QWidget underneath the QML which starts a system move. Something that doesn't work on wayland.

Ha, you're totally right. Amazing! Is this something to fix in Plasma frameworks?

Ha, you're totally right. Amazing! Is this something to fix in Plasma frameworks?

Breeze kstyle. I bet the issue is that handlers don't accept the events and/or process them differently from how we expect it from MouseArea

gvarsanyi added inline comments.
kcm/package/contents/ui/OutputPanel.qml
97

stepSize settings confuse me a bit. For perOutput it's 0.2, for global, it's 0.1.

I recommend setting both to 0.25 increments.

Both Windows and Gnome devs decided to do 0.25 increments for fractional scaling, and OSX also only has 3 fractional scales for every integer (albeit in logarithmic increments).
Although KDE doesn't have to follow suit, they all might have been onto something.
Advantages of 0.25 steps:

  • less prone to weird pixel-problems and artifacts that would come with scales like 1.6 or 1.1
  • does not skip the important 1.5 factor (like 0.2 steps would)
  • users probably want the 1.25 and 1.75 scale options more than 1.1, 1.2, 1.3, 1.4, 1.6, 1.7, 1.8, 1.9 - but they are unavailable with the 0.1 steps
  • much easier to run test apps against less (4 vs 10) variants

On the other hand, 0.25 should be good enough granularity for most.

For the scale slider, I'd put the numbers underneath, and have threenumbers; one on each end and one in the middle. So if the maximum value was 2x, it would look like this:

|-----------o-----------|
1x         1.5x        2x

I'm not sold on the proposal to use 1.25x increments, personally. For my personal use case, I like using 1.1x (13" 1080p screen) and I wouldn't want to lose that. Screen size and DPI combinations are so variable that I think it makes sense to support a higher level of precision. Apple can get away with their very low configurability because of tight control over the hardware that allows them to avoid selling machines where a given option wouldn't work.

That said, I could get behind 0.25x increments if there's some "advanced" page or mods in the GUI that exposes 0.1 incrememebts, or even a spinbox or text box or something.

Either is better than 0.2 increments though. As pointed out, that missed the all-important 1.5x.

Regardless, +100 for turning on fractional scaling on Wayland!

For the scale slider, I'd put the numbers underneath, and have threenumbers; one on each end and one in the middle. So if the maximum value was 2x, it would look like this:

|-----------o-----------|
1x         1.5x        2x

In that case what will be the vertical alignment of the Scale text? I am asking because for this case the vertical center alignment for Scale text looks weird in my opinion

I guess I was thinking that it should be aligned to the slider itself, like this:

Scale:  |-----------o-----------|
        1x         1.5x        2x

There is an example in the Fuzzy Clock config:

I'm not sold on the proposal to use 1.25x increments, personally. For my personal use case, I like using 1.1x (13" 1080p screen) and I wouldn't want to lose that. Screen size and DPI combinations are so variable that I think it makes sense to support a higher level of precision.

Strongly agree.

I guess I was thinking that it should be aligned to the slider itself, like this:

Scale:  |-----------o-----------|
        1x         1.5x        2x

Yep! Looks fine!

+1

Screen size and DPI combinations are so variable that I think it makes sense to support a higher level of precision.

Talking of which, there were some updated plans with regards to the UI for X scaling in the meeting that everyone in this thread was at.
I don't want to have made a plan in a meeting, and then discard it one week later. I think it's easier to leave that in an external dialog for the X case, then you don't have to worry about it in this patch.

I'm not sold on the proposal to use 1.25x increments, personally. For my personal use case, I like using 1.1x (13" 1080p screen) and I wouldn't want to lose that. Screen size and DPI combinations are so variable that I think it makes sense to support a higher level of precision. Apple can get away with their very low configurability because of tight control over the hardware that allows them to avoid selling machines where a given option wouldn't work.

That said, I could get behind 0.25x increments if there's some "advanced" page or mods in the GUI that exposes 0.1 incrememebts, or even a spinbox or text box or something.

Either is better than 0.2 increments though. As pointed out, that missed the all-important 1.5x.

I think we'd all like to support everyone's use cases. My argument is that 0.25/0.75 steps are almost as important as 0.5. If devs/users are not concerned about too much granularity (e.g. artifacts, testing), this can be fixed easily by doing 0.05 increments - it's just a slider anyway.
I also like the idea of adding a simple stepper with few predefined values, and an "advanced" configuration option for very precise settings.

This makes me think: current KCM partially breaks if I set fractional scaling on Wayland to a non-integer value (the scale is not represented on the preview pane, and there is no selection in the scale dropdown). Probably a slider can better represent a value like 1.375x, even if it doesn't allow for picking it.

Ha, you're totally right. Amazing! Is this something to fix in Plasma frameworks?

The problem relates to use of Handlers instead of old style MouseArea.

I haven't finished investigating, but I have a bad feeling QQuickWindow/QQuickWidget is the part that will need fixing :/

Screen size and DPI combinations are so variable that I think it makes sense to support a higher level of precision.

Talking of which, there were some updated plans with regards to the UI for X scaling in the meeting that everyone in this thread was at.
I don't want to have made a plan in a meeting, and then discard it one week later. I think it's easier to leave that in an external dialog for the X case, then you don't have to worry about it in this patch.

Here's what I wrote in https://notes.kde.org/p/usability-productivity-sprint-2019:

  • Discuss with Plasma folks on unifying our scaling methods; maybe remove or hide the Force Fonts DPI setting to avoid confusing people: https://phabricator.kde.org/T9500. Conclusion:
    • Move the Force Fonts DPI control into the scaling window and make them more visually connected to one another; see task for more details
  • Also discuss how we can eventually make Plasma use the global scale factor on X11: https://bugs.kde.org/show_bug.cgi?id=356446. Conclusion:
    • We can do it! See https://bugs.kde.org/show_bug.cgi?id=356446 We discussed this at the Plasma sprint and came to the following conclusion:

      We will re-investigate doing this, but first need to resolve the following blockers:
  • Fix the Task Manager's minimize animation zooming into out of the wrong location
  • A few other Task Manager-related positioning and screen-related issues

    We have a path forward and the blocking issues will be marked appropriately.
romangg updated this revision to Diff 61910.Jul 17 2019, 1:17 PM
romangg marked 23 inline comments as done.
  • Message on backend error
  • Use switch statements in OutputModel
  • Initialize roleNames from super class
  • i18n labels
  • Checkboxes to the left
  • Set onClicked
  • Combobox for resolutions
  • Use 0.1 scale step size
  • Name label Global scale
  • Button group for orientations
  • Use Rotation enum
  • Do not fill width on comboboxes
  • Refresh rate formatting
  • Use twinFormLayouts property
  • Use onMoved and onClicked
  • Output retention enum in QML
  • Do not use PlasmaComponents
  • Pretty-format scale text
  • Change retention description
  • Add colons to labels
romangg added inline comments.Jul 17 2019, 1:17 PM
kcm/package/contents/ui/OutputPanel.qml
42

Then there are two checkboxes. Looked at Night Color KCM, which does it by just setting the text instead of the FormData.label. Changed it to that for now

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

Nice!

99

Not with the default Slider style. Maybe needs a custom slider style or something?

GB_2 added a subscriber: GB_2.Mon, Aug 5, 8:58 AM

The icons folder can be deleted since it's not used anymore I think.

romangg updated this revision to Diff 63231.Tue, Aug 6, 8:30 PM

Rebase on master.

In D22468#506807, @GB_2 wrote:

The icons folder can be deleted since it's not used anymore I think.

True, thanks!

romangg updated this revision to Diff 63232.Tue, Aug 6, 8:38 PM
  • Remove icons folder
romangg updated this revision to Diff 63234.Tue, Aug 6, 8:56 PM
  • Add back global scale writing legacy code
GB_2 added a reviewer: VDG.Tue, Aug 6, 9:02 PM
GB_2 added a project: VDG.
GB_2 added a subscriber: VDG.

Pretty sweet!

  1. Any chance you'd be interested in implementing the combined scaling/font DPI design we discussed at the sprint, as detailed in D22468#496149?
  2. How does one test this? After building kscreen and installing its bits to /usr (in a VM of course) I still see the old KCM.

Pretty sweet!

  1. Any chance you'd be interested in implementing the combined scaling/font DPI design we discussed at the sprint, as detailed in D22468#496149?

Not in this patch. It should be possible to later add the field and logic to Panel.qml though.

  1. How does one test this? After building kscreen and installing its bits to /usr (in a VM of course) I still see the old KCM.

When you compile-install you need to manually delete the old library. On my system it's at: /usr/lib/x86_64-linux-gnu/qt5/plugins/kcm_kscreen.so

ngraham requested changes to this revision.Tue, Aug 6, 11:54 PM

Thanks Roman! This is really excellent overall. A big improvement over the current one . Nevertheless I think we can make it even better and I have some UI review comments:

  1. The "Output Settings" section header should contain the name of the output ("Settings for <name>") and then you can remove the output name that's in the FormLayout.
  2. The visualization at the top of the KCM with the white background needs a visible frame around it. Just give the Rectangle in Screen.qml an appropriate border color and radius.
  3. The padding between the refresh rate combo box and the horizontal spacer is way too high (and it's not consistent in the amount of spacing above and below the spacer)
  4. Need to add labels below the slider and also a label to the right that shows its current value like this:
Global Scale:  |--------------o--------| 1.6x
               1x         1.5x        2x
  1. You could save some vertical space in the visualization by putting the Identify button within each output in the visualization and putting the Center View button in the corner and making it only appear on hover (on the desktop at least)
  2. For that matter, you can hide the visualization entirely when there's only a single output. That will make the display much simpler and easier to parse for the common case of one screen.
This revision now requires changes to proceed.Tue, Aug 6, 11:54 PM

Thanks Roman! This is really excellent overall. A big improvement over the current one . Nevertheless I think we can make it even better and I have some UI review comments:

  1. The "Output Settings" section header should contain the name of the output ("Settings for <name>") and then you can remove the output name that's in the FormLayout.
  2. The visualization at the top of the KCM with the white background needs a visible frame around it. Just give the Rectangle in Screen.qml an appropriate border color and radius.
  3. The padding between the refresh rate combo box and the horizontal spacer is way too high (and it's not consistent in the amount of spacing above and below the spacer)
  4. Need to add labels below the slider and also a label to the right that shows its current value like this: ` Global Scale: |--------------o--------| 1.6x 1x 1.5x 2x `
  5. You could save some vertical space in the visualization by putting the Identify button within each output in the visualization and putting the Center View button in the corner and making it only appear on hover (on the desktop at least)
  6. For that matter, you can hide the visualization entirely when there's only a single output. That will make the display much simpler and easier to parse for the common case of one screen.

I currently concentrate on libkscreen backend work and output duplication and have based other patches on this rewrite here. So I only want to touch this diff again in case of critical issues but not add more detail work on top of what's already there. Instead after merge of the rewrite please create a task with your list of ideas so we can go with multiple small patches afterwards. Also some of these points should be discussed some more and a task is better suited for that.

I currently concentrate on libkscreen backend work and output duplication and have based other patches on this rewrite here. So I only want to touch this diff again in case of critical issues but not add more detail work on top of what's already there. Instead after merge of the rewrite please create a task with your list of ideas so we can go with multiple small patches afterwards. Also some of these points should be discussed some more and a task is better suited for that.

I can do that, but in principle I think when a patch involves a UI rewrite, it's fair game to offer UI suggestions. If you don't want to change the new UI you've written in this patch, it might be better to break this patch up into a backend component that you can use as a base for other patches, and a front-end leaf node patch where we can do the UI review independently of the other dependent backend work.

I currently concentrate on libkscreen backend work and output duplication and have based other patches on this rewrite here. So I only want to touch this diff again in case of critical issues but not add more detail work on top of what's already there. Instead after merge of the rewrite please create a task with your list of ideas so we can go with multiple small patches afterwards. Also some of these points should be discussed some more and a task is better suited for that.

I can do that, but in principle I think when a patch involves a UI rewrite, it's fair game to offer UI suggestions. If you don't want to change the new UI you've written in this patch, it might be better to break this patch up into a backend component that you can use as a base for other patches, and a front-end leaf node patch where we can do the UI review independently of the other dependent backend work.

Problem is that you can't really break this patch up since the front- and back-end changes are dependent on each other. But it's definitely fine to further refine the Ui afterwards. I see the Ui more as a first revision than the end goal. And because of that I want to break up continuous work into smaller chunks as soon as possible after a large rewrite involving both front- and back-end. This gives other front-end contributors a chance to put in their own ideas and me concentrate on the back-end again.

ngraham resigned from this revision.Wed, Aug 7, 7:04 PM

Okay, sure.