Implement OSD to select action when unknown monitors is connected
ClosedPublic

Authored by dvratil on Dec 19 2017, 8:46 PM.

Details

Summary

Building on top of the sebas/osd branch, this change introduces a special OSD that appears when a previously unknown monitor is connected and allows users to choose what should KScreen do with this screen.

This is what it looks like currently:

Tested with Wayland as well and seems to work quite well (requires some of my other libkscreen/kscreen patches related to various Wayland bugfixes and improvements)

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.
dvratil created this revision.Dec 19 2017, 8:46 PM
Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptDec 19 2017, 8:46 PM
dvratil requested review of this revision.Dec 19 2017, 8:46 PM

How is the interaction supposed to happen? KWin considers OSDs as a non-interactive on screen display (as the name says). Key events are not passed to OSDs and for mouse I'm not sure.

And also please test on Wayland: we have a requirement of new features on Wayland first.

dvratil updated this revision to Diff 25432.EditedJan 15 2018, 10:54 PM

Fixes and improvements, improved Wayland support (and actually tested on Wayland)

dvratil edited the summary of this revision. (Show Details)Jan 15 2018, 10:55 PM

How is the interaction supposed to happen? KWin considers OSDs as a non-interactive on screen display (as the name says). Key events are not passed to OSDs and for mouse I'm not sure.

This is built on top of sebas's OSD work which is not using the Plasma's OSD but is just a PlasmaCore.Dialog with PlasmaCore.Dialog.OnScreenDisplay type set and with PlasmaCore.Dialog.outputOnly set to false which allows mouse interaction with the dialog.

I think OnScreenDisplay implies being non-interactive on Wayland

I think OnScreenDisplay implies being non-interactive on Wayland

I could interact with the dialog on Wayland, so either it's not implied, or there's a bug in KWin :-)

sebas added a comment.Jan 16 2018, 4:27 PM

I think OnScreenDisplay implies being non-interactive on Wayland

I could interact with the dialog on Wayland, so either it's not implied, or there's a bug in KWin :-)

We might as well remove that flag, since it's confusing to Kai and Martin ;-)

According to KWin code, windows of type OnScreenDisplay do not accept focus and don't take keyboard input but they can still be clicked (unless set to outputOnly, obviously).

According to KWin code, windows of type OnScreenDisplay do not accept focus and don't take keyboard input but they can still be clicked (unless set to outputOnly, obviously).

The idea of an OnScreenDisplay is to be non-interactive. If KWin allows to pass pointer events to it, this is certainly a bug which needs fixing.

I just had a look at all the OSD's used and they all have outputOnly set to true. I have to say the idea here is clearly that OSDs should be non-interactive.

abetts added a subscriber: abetts.Jan 16 2018, 5:48 PM

Will this OSD need any kind of styling? Or will we just use what was shown above in the screenshot?

sebas added a comment.Jan 17 2018, 8:02 AM

Will this OSD need any kind of styling? Or will we just use what was shown above in the screenshot?

We're open to mockups, of course. :)

sebas added a comment.Jan 18 2018, 3:21 PM

I've tested this branch, and it's looking quite good. On X11, it works as expected now after doing a few changes:

  • The first two icons were mixed up, fix added to dvratil/osd branch
  • The OSD wouldn't dismiss when the cable got unplugged without selecting an action, also fixed in dvratil/osd

I'll give it some more testing on Wayland now.

sebas requested changes to this revision.Jan 18 2018, 4:08 PM
sebas added inline comments.
kded/qml/OsdSelector.qml
50

i18n() instead of qsTr()

55

i18n() instead of qsTr()

60

i18n() instead of qsTr(), perhaps clone or unify? (Unify is used elsewhere in the UI)

65

i18n() instead of qsTr()

70

i18n() instead of qsTr()

75

Perhaps "Leave unchanged"?

This revision now requires changes to proceed.Jan 18 2018, 4:08 PM

Cool.

kded/daemon.cpp
188

This makes me uneasy.

195

switch

kded/osd.cpp
44

Can we change it to create the QmlObject on demand the first time it's used?

94

Coding style, brace on next line

114–115

Can be simplified to

if (!m_output || !m_output->isConnected() || ...)
kded/osdmanager.cpp
51

Why do you pass this in the signal?

60

Shouldn't it rather do that in the plugin's registerTypes?

134

Can be simplified even further

KScreen::Osd *osd = m_osds.value(output->name());
if (!osd) {
    osd = new KScreen::Osd ...
...
202

Simplify here too

kded/qml/OsdSelector.qml
43

Repeater has a count property, avoids copying that JS Array over just to get its length

(also, urgh, I hoped ButtonRow was smarter than that)

80

Why do you need an IconItem inside? The Button can have an icon of its own

dvratil marked 15 inline comments as done.Jan 19 2018, 12:32 PM
dvratil added inline comments.
kded/osdmanager.cpp
60

We don't have a QML plugin :( This is the only C++ class that is exposed to the OSD QML.

kded/qml/OsdSelector.qml
80

It's a workaround for Button's iconSource displaying the icon as 24x24 regardless of the size of the button, so we end up with massive button with a tiny icon.

dvratil updated this revision to Diff 25635.Jan 19 2018, 12:40 PM

Please disregard the noise in this patch, sebas merged master into the dvratil/osd branch so I can't generate a proper diff against the base revision without the noise from master.

Please disregard the noise in this patch, sebas merged master into the dvratil/osd branch so I can't generate a proper diff against the base revision without the noise from master.

Sorry for that ... there we quite some changes in master which I wanted to test together with that, and I had some useful local patches added as well...

sebas accepted this revision.Feb 23 2018, 10:32 AM
This revision is now accepted and ready to land.Feb 23 2018, 10:32 AM

When landing, can you merge the whole branch into master, the history here is relevant and it will make continuing work on my kscreen kcm qmlification branch a lot easier. Thanks!

Is the screenshot in the Summary section still accurate? I'd like to feature this in the coming weekend's Usability & Productivity blog post (assuming it lands today or tomorrow; if not, next week's post), and people love looking at screenshots.

@ngraham Yes, the screenshot is still accurate. If you want a screenshot with more of background or another background, I can provide one.

This revision was automatically updated to reflect the committed changes.

Could this be implemented for bug 390096? Having the display button open this OSD would be an improvement over the current operation, in my opinion.