Add platform detection to KWorkspace library to adjust QT_QPA_PLATFORM
ClosedPublic

Authored by graesslin on Feb 25 2018, 8:23 AM.

Details

Summary

This is a preparation step to unset QT_QPA_PLATFORM from the wayland
startup session script. Setting QT_QPA_PLATFORM breaks 3rd-party Qt
software which does not bundle QtWayland. Most prominent example is
the Qt installer itself (see
https://bugreports.qt.io/browse/QTBUG-60222).

On the other hand our Plasma workspace applications need to be forced to
Wayland on a Wayland system. So we have a conflict between we want to
set QT_QPA_PLATFORM and we don't want to set QT_QPA_PLATFORM.

This change adds new API to KWorkspace to address this problem. The new
method adjusts the QT_QPA_PLATFORM based on the XDG_SESSION_TYPE
enviornment variable. It is completely opt-in. Meaning applications need
to explicitly add the call prior to creating the QGuiApplication and if
the user specifies either QT_QPA_PLATFORM env variable or any of the
-platform command line argument variants, the platform detection is
skipped.

The change also adjusts all plasma-workspace applications which should
use Wayland on Wayland to use the new API. The startup script on the
other hand still sets QT_QPA_PLATFORM. We also have applications outside
of plasma-workspace which needs this detection. Examples are:

  • powerdevil
  • systemsettings
  • kinfocenter

Once this change is merged those applications can be adjusted by linking
against PW::KWorkspace and afterwards QT_QPA_PLATFORM can be unset from
startplasmacompositor.

Test Plan

See added autotest

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
graesslin created this revision.Feb 25 2018, 8:23 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 25 2018, 8:23 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
graesslin requested review of this revision.Feb 25 2018, 8:23 AM
broulik added inline comments.
libkworkspace/kworkspace.h
161

Can you assert this?

164

detectPlatform?

Also I think all of this will become obsolete with Qt 5.11 where it supports multiple platform plugins as a fallback chain: https://codereview.qt-project.org/#/c/220294/

Also I think all of this will become obsolete with Qt 5.11 where it supports multiple platform plugins as a fallback chain: https://codereview.qt-project.org/#/c/220294/

Yes, this is only intended to be used till Plasma depends on Qt 5.11.

libkworkspace/kworkspace.h
161

I'll rephrase it, it's not a "must", it's just a "must happen before if one wants to use it". Given that I don't think an assert is required.

graesslin marked 2 inline comments as done.Mar 4 2018, 8:14 AM
graesslin updated this revision to Diff 28556.Mar 4 2018, 8:16 AM
  • Reword documentation to make it more clear when it has to be called
  • rename to detectPlatform
romangg added a subscriber: romangg.Mar 7 2018, 6:02 PM

Shouldn't this function go directly in KWayland as some utily function xdgSessionToQpaPlatform? I believe it's generic enough for that and even though Qt 5.11 will have detection for QT_QPA_PLATFORM the function could be of interest to other apps as well, which can't yet rely on Qt 5.11. In particular of course KDE apps themselves.

Shouldn't this function go directly in KWayland as some utily function xdgSessionToQpaPlatform? I believe it's generic enough for that and even though Qt 5.11 will have detection for QT_QPA_PLATFORM the function could be of interest to other apps as well, which can't yet rely on Qt 5.11. In particular of course KDE apps themselves.

I think we don't need it in frameworks as application using our flatpack runtime get the behavior for free.

romangg accepted this revision.Mar 7 2018, 7:52 PM

Hmm, what about application distributed as distro packages? Anyways,if you think this is a non-issue, feel free to push.

This revision is now accepted and ready to land.Mar 7 2018, 7:52 PM

Hmm, what about application distributed as distro packages? Anyways,if you think this is a non-issue, feel free to push.

Let's be realistic. Many applications we ship don't even have the desktop file name fixed for Wayland. I doubt we would see adjustments for this prior to distros shipping Qt 5.11.

interest to other apps as well, which can't yet rely on Qt 5.11

The 5.11 changes are about runtime Qt, not any new API. Apps will have to do nothing.
The app itself can still rely on Qt5.0.


We discussed in the meeting, I still think it's a bad idea to waste time bodging an already properly solved problem - and in exchange for fixing some obscure bundled apps we break other stuff and make wayland app-dev harder.

But I also said I wouldn't block this specific proposal as it is here.

Code is fine +0

Well we need to stop exporting the QT_QPA_PLATFORM value in startup script in any case, right? Or when we depend on Qt 5.11 and then set it to "wayland;x11" does it not break apps depending on Qt 5.y, y<11? So the question is just if we want to wait till we depend on 5.11 with this removal or do it now (and using the helper function proposed in this diff instead).

Putting this helper function into some framework is unnecessary though, that I can see now.

This revision was automatically updated to reflect the committed changes.
fvogt added a subscriber: fvogt.Mar 24 2018, 6:32 PM

XDG_SESSION_TYPE is not set if you run startplasmacompositor from a tty - so should this also check for WAYLAND_DISPLAY or startplasmacompositor set XDG_SESSION_TYPE=wayland?

XDG_SESSION_TYPE is not set if you run startplasmacompositor from a tty - so should this also check for WAYLAND_DISPLAY or startplasmacompositor set XDG_SESSION_TYPE=wayland?

No, I think that's a cornercase. Running from a tty is not the way users interact with the system and from developers we can expect to set this variable.

Basing the change on the availability of WAYLAND_DISPLAY or DISPLAY has disadvantages including that we don't know what the door is. We could have both: Wayland on top of X of X on top of Wayland.

fvogt added a comment.Mar 24 2018, 7:13 PM

XDG_SESSION_TYPE is not set if you run startplasmacompositor from a tty - so should this also check for WAYLAND_DISPLAY or startplasmacompositor set XDG_SESSION_TYPE=wayland?

[...] from developers we can expect to set this variable.

Any reason not to set it in startplasmacompositor then?

XDG_SESSION_TYPE is not set if you run startplasmacompositor from a tty - so should this also check for WAYLAND_DISPLAY or startplasmacompositor set XDG_SESSION_TYPE=wayland?

[...] from developers we can expect to set this variable.

Any reason not to set it in startplasmacompositor then?

It's not the task of startplasmacompositor to set it.