Bug 438312

I can confirm this issue.

(In reply to Oded Arbel from comment #0)
> SUMMARY
> When kwin is restarted (due to crash or on purpose - I some times need to do
> that on X11 to fixed kwin display issues), the assignment of windows to
> virtual desktop is retained, but assignment of activities is lost and all
> windows become "show on all activities".

I can also confirm it. And still continues to be an issue in 5.23.2

Yes, this it is really annoying and makes activities hard to use as they start getting in your way when the windows just hang there.

It was so annoying I made a script to fix it manually:
https://github.com/KnowZero/Rebind-KWin-Activities

But an official fix would be nice.

PS I fix most window display issues by just switching the compositor from opengl 2 to 3 and back. Fixes most issues without restarting kwin. But sometimes kwin just crashes, especially when developing.

Can confirm with git master.

*** Bug 435575 has been marked as a duplicate of this bug. ***

*** Bug 425105 has been marked as a duplicate of this bug. ***

As of neon unstable kwin X11 build 5.25.4+p22.04+tunstable+git20220815.0018-0 , this issues seems to be resolved - on session restart or kwin_x11 restart, the existing windows (or session restored windows) are mapped to the correct activities.

I'm not sure if a specific work was done to address this issue, but I'm closing this issue for now.

(In reply to Oded Arbel from comment #6)
> As of neon unstable kwin X11 build
> 5.25.4+p22.04+tunstable+git20220815.0018-0 , this issues seems to be
> resolved - on session restart or kwin_x11 restart, the existing windows (or
> session restored windows) are mapped to the correct activities.
> 
> I'm not sure if a specific work was done to address this issue, but I'm
> closing this issue for now.

Apparently it was a fluke? Maybe I missed something during tests? But after another upgrade and restart, now running kwin-x11 5.25.4+p22.04+tunstable+git20220816.0249-0, activities are again not remembered after session restart or kwin_x11 restart.

Ok, sorry for the bug spam, but last update for today:

Previously (i.e. as of last week, before 5.25.4+p22.04+tunstable+git20220815.0018-0) kwin_x11 would not remember windows activity assignment when it was shutdown - regardless if it was by logout/shutdown, `systemctl --user restart plasma-kwin_x11.service` or `kwin_x11 --replace`

As of this week update, logging out and logging back puts the windows back into their assigned activities - the session log in is now a bit weird, because during log in - after the splash screen is hidden - the session switches to each activity to restore the windows to it, so if I logged out while in activity B, when I log back in it starts in activity B and then switches to activity A to restore the windows there, and stays there. It is a bit jarring, I may open another ticket on that.

But restarting kwin (using either `systemctl --user restart plasma-kwin_x11.service` or `kwin_x11 --replace`) causes the activity assignment to be lost and all windows assigned to "all activities" - which is the original issue reported.

Worthy of note - on my own build of KWin running on Kubuntu 22.04, the "kwin_x11 --restart" does result in the windows showing on the wrong activities, but switching between activities reveals that the windows appear to still be assigned to their respective activities properly (you can right-click an application button, hover over the "Show in Activities" menu, and see that the window is still assigned to the right activity). They're simply displaying on all activities, even though they seem to be assigned correctly. This is just from my own brief testing and work so far, so this might not be totally correct, but I noticed it and thought it might be useful to know.

OK, I've read through the code and I think I see why this bug is occurring. My understanding of C++ is not all that great since I don't actually know the language, so this could be partially wrong. It appears that KWin tells ksmserver to restore the previous session when the user logs back in, relaunching programs that were open when the user logged out. However, when "kwin_x11 --replace" is used, the applications are already launched, so the session restoration code isn't run and the apps end up visible on all activities.

In kwin/src/activities.cpp, bool Activities::start(const QString &id), lines 117-123:

>     QDBusInterface ksmserver("org.kde.ksmserver", "/KSMServer", "org.kde.KSMServerInterface");
>     if (ksmserver.isValid()) {
>         ksmserver.asyncCall("restoreSubSession", id);
>     } else {
>         qCDebug(KWIN_CORE) << "couldn't get ksmserver interface";
>         return false;
>     }

ksmserver's "restoreSubSession" member function is called.

In https://invent.kde.org/plasma/plasma-workspace/-/blob/master/ksmserver/server.cpp, the member function void KSMServer::restoreSubSession(const QString &name) calls void KSMServer::tryRestoreNext(), which appears to go through a list of apps that need relaunched, determine whether they are already launched or not, relaunches them if they are not launched, and skips over them when they are already launched. Particularly, lines 986-994 of the above mentioned file:

> bool alreadyStarted = false;
>         foreach (KSMClient *c, clients) {
>             if (QString::fromLocal8Bit(c->clientId()) == clientId) {
>                 alreadyStarted = true;
>                 break;
>             }
>         }
>         if (alreadyStarted)
>             continue;

I believe this is where the bug is. The "if (alreadyStarted)" block needs to determine which activity the window(s) of the already launched app are supposed to be on, and specifically set the window(s) of that app to that activity. Otherwise, when "kwin_x11 --replace" is called, none of the session restoration really works, and thus you end up with a big lump of windows on all your activities. I'd guess the reason that the activities still seem to be set correctly for each app (as described in my previous comment) would be because the windows themselves still retain memory of what activity they're supposed to be on, but the restarted kwin process isn't taking that into account.

This might not be the best place for the fix to go, and it's possible I've misunderstood the code, but this is what I have so far. I'm not quite sure how to implement the proposed fix, I'm still working on that part. But here's the progress I've made, hopefully this will be helpful.

The kwin/src/activities.cpp file is https://invent.kde.org/plasma/kwin/-/blob/master/src/activities.cpp, sorry to accidentally fail to specify that.

> However, when "kwin_x11 --replace" is used, the applications are already launched, so the session
> restoration code isn't run and the apps end up visible on all activities.
This sounds like a similar issue to Bug 439159 with Plasma's --replace argument. In both cases that arg causes a different codepath to be taken starting in KDbusAddons.

Interesting. Using "killall kwin_x11 && kwin_x11" in KDE neon Unstable Edition results in identical behavior as when using "kwin_x11 --replace".

Ok, then I guess it's not that.

Maybe it would be better if, rather than implementing the fix in ksmserver, we implemented it in kwin/src/activities.cpp? I'm realizing it might be rather trivial to do something like this:

> bool Activities::start(const QString &id)
> {
>     Workspace *ws = Workspace::self();
>     //pseudocode starts here
>     foreach(Windows w in ws) {
>         toggleWindowOnActivity(w, id, true);
>         toggleWindowOnActivity(w, id, true);
>     }
>     //pseudocode ends here
>     if (ws->sessionManager()->state() == SessionState::Saving) {
>         return false; // ksmserver doesn't queue requests (yet)
>     }
> ...

That might fix the problem.

Wow, in retrospect that code is SO BAD. A typo, I'm probably missing C++ syntax, and the pseudocode logic is itself flawed. :facepalm:
I think I meant something more like this:

> bool Activities::start(const QString &id)
> {
>     Workspace *ws = Workspace::self();
>     //pseudocode starts here
>     foreach(Window w in ws.WindowList) {
>         if(w.IsOnActivity(id) {
>             toggleWindowOnActivity(w, id, true);
>             toggleWindowOnActivity(w, id, true);
>         }
>     }
>     //pseudocode ends here
>     if (ws->sessionManager()->state() == SessionState::Saving) {
>         return false; // ksmserver doesn't queue requests (yet)
>     }
> ...

Still probably messed up the syntax horribly, but at least the logic is right this time.

OK, nevermind, upon closer inspection I'm looking at the wrong code. No changes I make to bool Activities::start(const QString &id) seem to have any effect (not even a blatant null pointer dereference or an std::terminate()). So I'm not sure when this code gets called, but I don't think it's called when kwin_x11 is launched. Either that or else KWin is insanely robust.

(In reply to Aaron Rainbolt from comment #17)
> OK, nevermind, upon closer inspection I'm looking at the wrong code. No
> changes I make to bool Activities::start(const QString &id) seem to have any
> effect (not even a blatant null pointer dereference or an std::terminate()).
> So I'm not sure when this code gets called, but I don't think it's called
> when kwin_x11 is launched. Either that or else KWin is insanely robust.

And it looks like even this comment was thanks to me not knowing what I was doing when I originally wrote it. Thankfully, though, it looks like this bug has finally been figured out for the most part. A PR to fix it is here: https://invent.kde.org/plasma/kwin/-/merge_requests/2941

Git commit ad95b495acb93549632422791a41940c65e5e61e by Vlad Zahorodnii, on behalf of David Edmundson.
Committed on 30/09/2022 at 07:10.
Pushed by vladz into branch 'master'.

Sync activities after kwin restart

Activities are loaded async. During this time any fetch of activity
information is incorrect as we will treat any settings as invalid.

We need to ignore attempts to set activities during this time, but also
refresh Window's concept of activities once we are loaded.

M  +15   -0    src/activities.cpp
M  +1    -0    src/activities.h
M  +3    -0    src/window.cpp
M  +6    -0    src/window.h
M  +1    -1    src/x11window.h

https://invent.kde.org/plasma/kwin/commit/ad95b495acb93549632422791a41940c65e5e61e

Git commit d47137679d0821722e3c3f528709fc6650b5d19a by Vlad Zahorodnii, on behalf of David Edmundson.
Committed on 30/09/2022 at 08:32.
Pushed by vladz into branch 'Plasma/5.26'.

Sync activities after kwin restart

Activities are loaded async. During this time any fetch of activity
information is incorrect as we will treat any settings as invalid.

We need to ignore attempts to set activities during this time, but also
refresh Window's concept of activities once we are loaded.


(cherry picked from commit ad95b495acb93549632422791a41940c65e5e61e)

M  +15   -0    src/activities.cpp
M  +1    -0    src/activities.h
M  +3    -0    src/window.cpp
M  +6    -0    src/window.h
M  +1    -1    src/x11window.h

https://invent.kde.org/plasma/kwin/commit/d47137679d0821722e3c3f528709fc6650b5d19a

I see the fix has not yet been cherry-picked to be in 5.24 for release in 5.24.7 (the current LTS release of Plasma). I highly suggest and urge this be done to keep with the 2-year LTS promise.

A possibly relevant merge request was started @ https://invent.kde.org/plasma/kwin/-/merge_requests/3041

Git commit 16c6c3d84795257e1576a20775d8e15f09c5aad3 by Aaron Rainbolt, on behalf of David Edmundson.
Committed on 12/10/2022 at 23:00.
Pushed by ngraham into branch 'Plasma/5.24'.

Sync activities after kwin restart

Activities are loaded async. During this time any fetch of activity
information is incorrect as we will treat any settings as invalid.

We need to ignore attempts to set activities during this time, but also
refresh Window's concept of activities once we are loaded.

(cherry picked from commit d4713767 and adapted for KWin 5.24.6)

M  +3    -0    src/abstract_client.cpp
M  +6    -0    src/abstract_client.h
M  +15   -0    src/activities.cpp
M  +1    -0    src/activities.h
M  +1    -1    src/x11client.h

https://invent.kde.org/plasma/kwin/commit/16c6c3d84795257e1576a20775d8e15f09c5aad3