rework kscreen's OSD logic
AbandonedPublic

Authored by sebas on Dec 5 2016, 5:15 PM.

Details

Summary

This changeset moves the various OSDs used in kscreen (output identifier, display button feedback) into the kded module.

The functional change in here is that instead of calling into plasmashell, now kscreen renders its own OSD. Plasmashell falls short since it doesn't offer the controls which kscreen needs, namely deciding what ends up on which output. The display button feedback is done entirely in the kded module, the output identification bits will be called over dbus by the new kscreen module. (I don't want to change the old one too much at this point.)

The ui code also now works better on high dpi displays.

I've tested this code on Wayland and X11, and it works on both. The previous OSD code doesn't.

Delaying the display button switch now also allows us to indicate what actually happened after pressing the button, so the user gets much better feedback (after the changes have been applied.)

Test Plan
  • tested on X11 and wayland, multi and single screen
  • changeset contains a test app

Diff Detail

Repository
R104 KScreen
Branch
sebas/osd
Lint
No Linters Available
Unit
No Unit Test Coverage
sebas updated this revision to Diff 8777.Dec 5 2016, 5:15 PM
sebas retitled this revision from to rework kscreen's OSD logic.
sebas updated this object.
sebas edited the test plan for this revision. (Show Details)
sebas added a reviewer: Plasma.
sebas added a subscriber: davidedmundson.
Restricted Application added a project: Plasma. · View Herald TranscriptDec 5 2016, 5:15 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik added a subscriber: broulik.Dec 5 2016, 6:11 PM

A bunch of nitpicks below. Haven't tried it yet, though :)

Is there a chance we could serve the novice user usecase of Meta+P and then *click* / choose the desired setup? As far as I can tell we only show "You now use $setup" but not "You can choose: [Clone, Left of, Internal only, External only, Right of]" (doesn't neccessarily have to be part of this patchset, obviously, just food for thought, and you know how obsessed I am with the Meta+P menu ;)

kded/daemon.cpp
242–243

Superfluous lines

283

static?

287

Not sure about "Embedded", perhaps "Internal"? Dunno.

Also I'd rather turn the wording around, rather than "internal off" and "external off", say "external only" and "internal only"?

291

const &

kded/osd.cpp
32

using namespace

37

You're only using it in the constructor, no need for it to be a member variable that stays around

38

Can we lazy-load this first time it is used? It's not like you often change screen setup

Edit: nvm, saw later that you actually create the entire thing on demand

53

Is always null here

85

QSize has a transpose() method "Swaps the width and height values."

120

Compare with QL1S - in case you have KWindowSystem you can ask it

kded/osd.h
47

Superfluous line

49

Doesn't have to be a slot with new connect syntax

kded/osdmanager.cpp
21

Unused?

31

nullptr, also s_ prefix since it's static

41

There's a a qDeleteAll(m_osds) that does begin() and end() for you

44

There are no signals in this class

78

QMap::contains(key), no need for keys()

Also, better use (const)Find to avoid double loop-up (once for contains and once for value)

99

Can you perhaps put that stuff into a separate function to avoid code duplication - the loops are almost identical

kded/qml/Osd.qml
57

Not really happy with this "rootItem" property but then David told us that randomly accessing properties outside a file is bad, too ;)

Perhaps try

Loader setSource(source, {rootItem: root})

so the item is already created with that property set, avoids warnings and/or foo ? foo.bar : null dance and re-evaluations

kded/qml/OsdItem.qml
55

;)

sebas updated this revision to Diff 8796.Dec 5 2016, 10:44 PM
sebas marked 15 inline comments as done.
  • improve osd test app
  • call the dbus service from the test app
  • also register the service
  • Addressing Kai's review, part 1
kded/osd.cpp
120

We only have KWindowSystem implicitly, I'm going with the QGuiApplication::platformName() comparison (changed to QL1S).

kded/osdmanager.cpp
21

It's used now. :)

It has the future wayland problem that MG wants to limit which applications can access which interfaces; you're implicitly using a plasma specific one in Dialog to position the window, which would make kded need to be a "trusted application"

So whatever this solves, I think we might end up going back and having to fix plasmashell in the future.
(or move this into low level kwin, which is the only way you'll be able to show the right output identifier on mirrored screens)

kded/osd.cpp
38

you're not getting and look and feel / theming in here.

kded/osdmanager.cpp
45

FWIW, You're already exporting Daemon in this application as

service: org.kde.kded5
path: /modules/kscreen
iface: org.kde.KScreen

I'm not convinced you really need a new service, and your new paths/iface don't match up the same style.

sebas marked an inline comment as done.Dec 6 2016, 1:37 PM
sebas added a subscriber: graesslin.

I actually talked about this with @graesslin, and this is the solution we came up with. When we limit the number of processes that are allowed to talk specific protocols to the Wayland server (the plasma surfaces, for example, but also the output configuration protocol), we'll move this code into its own daemon and kwin will authorize that daemon. THat's basically the same approach we'll have to take with powerdevil. This patch does actually help it since we reduce the number of entry points for the OSD to one (just kded) instead of two (kded and kcm).

Bottom line: Considered it, it's part of the plan. :)

mart added a subscriber: mart.Dec 12 2016, 9:05 AM
In D3598#67455, @sebas wrote:

protocol), we'll move this code into its own daemon and kwin will authorize that daemon. THat's basically the same approach we'll have to take with powerdevil.

so in the end the authorized process would be that separed daemon and not kded? (as kded can in turn run all sort of 3rd party stuff, would be better if itself is not an authorized process)

Correct, that's the idea.

sebas planned changes to this revision.Jul 11 2017, 12:31 PM
sebas added inline comments.
kded/osd.cpp
38

We do get theming, but not the OSD from the Look and Feel package. I feel it's overkill to include this in the Look and Feel package, as it would needlessly blow up the look and feel package spec. What do you think about it? Do you see a more elegant solution?

sebas abandoned this revision.Jan 18 2018, 2:58 PM

This branch is going to be merged as part of https://phabricator.kde.org/D9414