Ensure WM initializes before session starts
Needs RevisionPublic
Actions

Authored by esjeon on Jan 1 2020, 6:53 AM.

Details

Reviewers

Group Reviewers

Summary

This is an attempt to resolve bug 362857. A number of users have complained about restored application not supporting desktop effects, like transparency and blurring. This is caused by a race condition where restored applications start before KWin (and its compositor) finishes its initialization. Under this condition, applications fail to detect compositor and disable desktop effect on themselves.

This patch tries to avoid this race condition by inserting delay b/w initialization of KSM server, which starts KWin (or user-designated WM), and start-up phase 0. At the same time, the patch also tries to minimize the actual delay by detecting KWin and compositors using D-Bus and KWindowSystem repectively.

Test Plan

The nature of this bug (graphics!) forces the use of physical machines for testing, but not every machine can reproduce this. Old hardware w/ proprietary graphics driver is recommended.

Pre-requisite:
1. "Restore previous session" option enabled under "Desktop Session".
2. Konsole set up w/ transparent window
3. Verify that the bug is reproducible on your machine (Reboot might be required)

Test 1: session restores normally
1. Login, open Konsole, logout, and re-login
2. Konsole should be restored.

Test 2: KWin w/ compositor, restore w/ desktop effect.
1. Login, open Konsole, reboot, and login.
2. Konsole should be restored w/ transparent background.

Test 3: Other WM w/ compositor, restore w/ desktop effect.
1. Put the following into ~/.xinitrc (edit it to fit your env.): export KDEWM=/usr/bin/dwm; picom & exec startplasma-x11
2. From Linux console, run xinit.
3. Konsole should be restored w/ transparent background.

Diff Detail

Repository

R120 Plasma Workspace

Lint

Lint Skipped

Unit

Unit Tests Skipped

esjeon created this revision.Jan 1 2020, 6:53 AM

Restricted Application added a project: Plasma. · View Herald TranscriptJan 1 2020, 6:53 AM

Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

esjeon requested review of this revision.Jan 1 2020, 6:53 AM

Note that this is an RFC at the current state. This can be solved elsewhere (e.g. ksmserver), and is tested only on X11 (I don't have machine that runs wayland w/o problems). I also don't like that plasma-session is calling QGuiApplication, which is required to use KWindowSystem.

Also, I think it's safe to remove m_enabled here, for Qt terminates the object after calling emitResult, but I'm not really fluent in Qt to be sure about it.

Anyways, there's lots of room for discussion.

xyquadrat added a reviewer: Plasma.Jan 1 2020, 10:52 AM

asturmlechner added a subscriber: asturmlechner.Jan 1 2020, 12:47 PM

ngraham added a reviewer: davidedmundson.Jan 6 2020, 4:11 PM

On the conceptual level, does it make more sense for the relevant client (konsole) to monitor for compositing changed itself. It's something that could change at runtime regardless, so it needs to react to it anyway?
If it does need that code anyway, there's no point doing fixes in startkde

Code wise, this gives a timeout for anyone who doesn't run kwin composited, which would be a problem as is.

also I don't like the split that ksmserver is waiting for kwin to register on ICE, and startkde is waiting on kwin through a combo of both DBus and X atoms in KWindowSystem::compositingActive(). That's 3 different paths all used at once.

I've been wanting to move the kwin startup from ksmserver into startkde anyway, doing those things together would make the most sense.

This revision now requires changes to proceed.Jan 6 2020, 4:18 PM

In D26338#588960, @davidedmundson wrote:

On the conceptual level, does it make more sense for the relevant client (konsole) to monitor for compositing changed itself. It's something that could change at runtime regardless, so it needs to react to it anyway?
If it does need that code anyway, there's no point doing fixes in startkde

Technically, yes, apps can do that. But this will force changes to any apps that embed Konsole (i.e. dolphin, kate) or depend on desktop effects. Also, this issue will arise whenever an app implements such features. We can just solve this here and forget about it forever.

Also, the problem only occurs during session restoration, so it's clear that one change here can fix for all applications.

Code wise, this gives a timeout for anyone who doesn't run kwin composited, which would be a problem as is.

Welp, I agree, but there's just no way to figure out if user wants compositor or just WM. A possible solution is to make this toggleable through configuration or env, which isn't exactly beautiful.

Still, the impact can be minimized by moving wmjob to right before session restoration instead of stage0.

also I don't like the split that ksmserver is waiting for kwin to register on ICE, and startkde is waiting on kwin through a combo of both DBus and X atoms in KWindowSystem::compositingActive(). That's 3 different paths all used at once.

I've been wanting to move the kwin startup from ksmserver into startkde anyway, doing those things together would make the most sense.

I agree. Things will be much cleaner if WM is handled in startkde. WM isn't really a part of session, and ksmserver already treats WM as a special case anyway.

Revision Contents
Changeset List

			Path	Packages
M			startkde/plasma-session/CMakeLists.txt (1 line)
M			startkde/plasma-session/main.cpp (4 lines)
M			startkde/plasma-session/startup.h (13 lines)
M			startkde/plasma-session/startup.cpp (64 lines)