Rewrite KScreen KCM as ConfigModule with outputs model and Kirigami
ClosedPublic

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
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

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.Aug 5 2019, 8:58 AM

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

romangg updated this revision to Diff 63231.Aug 6 2019, 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.Aug 6 2019, 8:38 PM
  • Remove icons folder
romangg updated this revision to Diff 63234.Aug 6 2019, 8:56 PM
  • Add back global scale writing legacy code
GB_2 added a reviewer: VDG.Aug 6 2019, 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.Aug 6 2019, 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.Aug 6 2019, 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.Aug 7 2019, 7:04 PM

Okay, sure.

ngraham accepted this revision as: VDG.Aug 27 2019, 1:17 PM

(provided the UI tweaks can be made later)

FYI: I'm currently working on output duplication (and some preparations for that in KWin/KWayland) and will wait until this works fine before pushing this rewrite.

I just looked at the release schedule of frameworks and it will be difficult to get the required output duplication changes in KWayland 5.62 (tagging date 7th September), which Plasma 5.17 will depend on. Question is if we want to merge even without output duplication or try to squeeze the frameworks changes in before.

I'm quite interested getting the rewritten UI and Wayland fractional scaling in ASAP (VDG can always tweak and polish the UI once it's landed).

However according to https://community.kde.org/Schedules/Plasma_5, the Frameworks dependency version for Plasma 5.17 is 5.53, not 5.52. So I think you have another month.

Never mind, that was apparently a just-corrected typo (https://community.kde.org/index.php?title=Schedules%2FPlasma_5&type=revision&diff=85944&oldid=85406), and the dependency will be 5.62.

Given that, if there's not enough time to get the frameworks changes in, I would be in favor of splitting this out and landing everything that doesn't require a frameworks change

romangg updated this revision to Diff 65025.Aug 31 2019, 12:34 AM
  • Change visibility and reposition on enablement change

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.

This was not addressed.

davidedmundson added inline comments.Sep 4 2019, 12:54 AM
kcm/package/contents/ui/OutputPanel.qml
86

These need to be exclusive buttons. Currently you can select N at once.

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.

This was not addressed.

I don't see the current patch to discard any plans from the meeting. It just takes the current scale slider and puts it into the main ui since that is simple to do with QML , it's more in line with modern design paradigm of mobile apps and fits the "Outputs" and "Arrangement" split well. This does not mean the plans from the meeting can't be acted upon later. A font selection can still be added below the scale slider. It's just not part of this patch, since this is a rework, not adding additional features besides the long overdue fractional scaling slider on Wayland.

romangg updated this revision to Diff 65370.Sep 4 2019, 3:15 PM

Fix and simplify rotation buttons.

romangg added inline comments.Sep 4 2019, 3:17 PM
kcm/package/contents/ui/OutputPanel.qml
86

Thanks, the issue wasn't them being not exclusive buttons (this is automatically with ButtonGroup above), but that the RotationButtons were encapsulated into another item. Have removed the encapsulation now.

romangg updated this revision to Diff 65426.Sep 5 2019, 1:03 PM
  • Add scale debug output
This revision was not accepted when it landed; it landed in state Needs Review.Sep 5 2019, 3:55 PM
This revision was automatically updated to reflect the committed changes.
victorr added a subscriber: victorr.EditedSep 5 2019, 5:14 PM

Not all lines entered the localization file.
Text in patch

+ text: i18n("New global scale applied. " +
+ "Change will come into effect after restart.")
+ connectMsg.text = i18n("A new output has been added. " +
+ " Settings have been reloaded.");
+ } else {
+ connectMsg.text = i18n("An output has been removed. " +
+ " Settings have been reloaded.");

Line «Change will come into effect after restart.» is ignored
Line «Settings have been reloaded.» is ignored

Incorrect filename "kcm_displayconfiguration" in the /kscreen/kcm/Messages.sh file.
Translation is not displayed.
Attached a picture 1.

File /kscreen-5.16.4/kcm/kcm.cpp contains name
«KAboutData *about = new KAboutData(QStringLiteral("kcm_kscreen"),»
After renaming, the translation is displayed.
Attached a picture 2.

Since you found already the root issue want to write a quick patch to fix it?

Are you talking about this?

Are you talking about this?

I thought there was also markup missing in the QML files.

If you apply the attached file, the lines are added to the localization file, and the translation works.


Attached a picture

If you apply the attached file, the lines are added to the localization file, and the translation works.


Attached a picture

Applying this patch makes the kcm not start anymore

The CMakeLists.txt file lacks the packages needed for the build.
Installed many additional kf5 and plasa packages for assembly,
since I was too lazy to figure out which package is missing.

The CMakeLists.txt file lacks the packages needed for the build.
Installed many additional kf5 and plasa packages for assembly,
since I was too lazy to figure out which package is missing.

It builds but the KCM does fail to show anything with error:

"file:///usr/share/kpackage/kcms/kcm_kscreen/contents/ui/main.qml" 
 "Error loading QML file.\n80: Unexpected token `string literal'\n100: Unexpected token `string literal'\n103: Unexpected token `string literal'\n"

I collected another package with a new patch.

I collected another package with a new patch.

The string was composited so the 100 characters line limit is not broken. Is there a way to break it up such that it still can be translated easily?

I don’t understand what package is missing in CMakeLists.txt
I get an error during assembly

DEBUG: Warning: Could not locate service type file kservicetypes5/kcmodule.desktop, tried ("/builddir/.local/share", "/usr/local/share", "/usr/share") and ":/kservicetypes5/kcmodule.desktop" ((null):0, (null))
DEBUG: About to parse service type file "kcmodule.desktop"
DEBUG: make[2]: *** [kcm/CMakeFiles/kcm_kscreen_autogen.dir/build.make:65: kcm/kcm_kscreen.json] Error 1

Then install all the packages needed to build plasma-desktop.
After installing them, the kscreen package is built without errors.

I collected another package with a new patch.

The string was composited so the 100 characters line limit is not broken. Is there a way to break it up such that it still can be translated easily?

Like so:
xi18n("@info", "the first 100 characters<nl/>the next 100 characters<nl/>and so on");

Like so:
xi18n("@info", "the first 100 characters<nl/>the next 100 characters<nl/>and so on");

I meant the 100 chars line limit of the code source file.

victorr added a comment.EditedSep 6 2019, 8:21 AM

And such a patch with the separation of \n?

yurchor added a subscriber: yurchor.Sep 8 2019, 7:11 AM

Like so:
xi18n("@info", "the first 100 characters<nl/>the next 100 characters<nl/>and so on");

I meant the 100 chars line limit of the code source file.

It seems that there is no way to do this now (usual i18n("Sentence1 \\\nSentence2") does not work as expected). You should break the 100-chars limit or rewrite the strings as

i18n("Sentence 1") +
i18n("Sentence2")

Thanks in advance for fixing this problem.

This comment was removed by victorr.