[wayland] Add new XdgOutput properties
ClosedPublic

Authored by davidedmundson on Mar 24 2020, 12:24 AM.

Details

Summary

AbstractOutput::name() behaviour is changed so that it matches the X11
behaviour, showing an identifier like "HDMI-0".

XdgOutput.name is set to this name.

XdgOutput.description is currently set to the manufacturer name and
model, but it's not exposed to Qt so we probably don't care too much.

This should fix plasmashell changing applets when switching between X11
and wayland.

Test Plan

Relevant unit test
I still need to run it on my laptop.

Diff Detail

Repository
R108 KWin
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24138
Build 24156: arc lint + arc unit
Restricted Application added a project: KWin. · View Herald TranscriptMar 24 2020, 12:24 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Mar 24 2020, 12:24 AM
zzag added a subscriber: zzag.Mar 24 2020, 7:55 AM

What patch do I need to apply to KWayland to test this change?

abstract_wayland_output.cpp
288

Please rename AbstractOutput::name() to AbstractOutput::description() to keep AbstractOutput::name() and XdgOutputInterface::name() in sync.

davidedmundson added inline comments.Mar 24 2020, 8:57 AM
abstract_wayland_output.cpp
288

I had looked. Without bringing in further behavioural changes it makes things worse.

You then need to change screens to be in sync, and that one gets used a lot.

Then instead of having one piece of code refer to name when they mean description, we have dozens refer to description when they just want a simple readable name.

davidedmundson planned changes to this revision.Mar 24 2020, 9:41 AM
davidedmundson added inline comments.
abstract_wayland_output.cpp
288

Maybe it's not so bad.

It seems on X11 the name is in the form HDMI-0 which is what we call "name" here.

So this actually is an opportunity to make things more consistent.

zzag added inline comments.Mar 24 2020, 9:48 AM
abstract_wayland_output.cpp
288

So, I assume you're talking about output mapping for touch devices. If that's the case, we could set the udev 'WL_OUTPUT` property on the input device, but nobody does it. Alternatively, we could follow libinput's documentation [1] and use something else.

My main concern about this code is that we start mixing identifiers, names, and descriptions, which makes code more difficult to work with and reason about.

[1] https://wayland.freedesktop.org/libinput/doc/latest/api/group__device.html#gaf48626f6190e9c9bc14abb704e66cc22

zzag added inline comments.Mar 24 2020, 10:24 AM
abstract_wayland_output.cpp
288

We probably don't even need to bother with libinput_device_get_output_name() as long as our output names match weston's output names.

I also believe that libinput_device_get_output_name() returns NULL in many cases since I don't see who sets WL_OUTPUT. Either way, worth checking...

davidedmundson edited the summary of this revision. (Show Details)
davidedmundson removed a subscriber: zzag.

update

abstract_wayland_output.cpp
288

No I was referring to OutputScreens::name(int screen)

Following that, it gets used in the UI in context menus.

I initially was trying to avoid a behavioural change on that, but it turns out we probably want that.

zzag accepted this revision.Mar 24 2020, 10:55 AM
zzag added inline comments.
plugins/platforms/fbdev/fb_backend.cpp
43

nit: Wrap "FB-0" in QStringLiteral.

This revision is now accepted and ready to land.Mar 24 2020, 10:55 AM
This revision was automatically updated to reflect the committed changes.