Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notifications and other popups are treated like "normal" windows on Plasma Wayland #118650

Closed
peterhoeg opened this issue Apr 6, 2021 · 56 comments · Fixed by #139459, #139537 or #121345
Closed
Assignees
Labels

Comments

@peterhoeg
Copy link
Member

Describe the bug
When running Plasma Wayland, notifications and krunner pop-downs are treated like first-class windows and therefore take focus and appear in the task manager.

This used to be an issue back in the day (https://bugs.kde.org/show_bug.cgi?id=365107) so this is in all likelyhood something we do (wrong) on our side.

To Reproduce
Steps to reproduce the behavior:

  1. follow the steps in nixos/plasma5: make it run using systemd and wayland #117102 to get wayland running
  2. log in to the plasma wayland session
  3. wait for a notification or trigger krunner
  4. observe the focus steal (for notifications) and appearance in the task manager

Expected behavior
Notifications and similar should not be treated as a regular window and therefore not show in task manager or receive focus (krunner should still have focus of course).

Notify maintainers
Cc: @NixOS/qt-kde

Metadata

  • system: "x86_64-linux"
  • host os: Linux 5.11.11, NixOS, 21.05.git.d3ff65233f9M (Okapi)
  • multi-user?: yes
  • sandbox: yes
  • version: nix-env (Nix) 2.4pre20210326_dd77f71
  • channels(root): ""
  • channels(peter): ""
  • nixpkgs: /nix/store/w9zhp284bnx3f1ma6mdbsw008jpnq15v-nixpkgs_unstable

References:

@peterhoeg peterhoeg added the 0.kind: bug Something is broken label Apr 6, 2021
@peterhoeg
Copy link
Member Author

@samueldr
Copy link
Member

samueldr commented Apr 6, 2021

Oh, my intuition tells me this could be the root cause of the totally-wonky Plasma Mobile notifications.

@peterhoeg
Copy link
Member Author

@samueldr - I found this: https://community.kde.org/Plasma/Notifications#DesktopEntry_in_notifyrc

I haven't looked through the code yet (I'm not at all familiar with the plasma code) but it sure sounds like there is a disconnect between kwin, application and desktop file.

However, KNotifications uses the application name for identification rather than the desktop entry, so a DesktopEntry hint must be placed in the respective notifyrc file, so they can be filed under "applications".

@samueldr
Copy link
Member

samueldr commented May 6, 2021

I don't think this is relevant to the "rendering" bits of the notifications. I'm not an expert either, but to me this sounds like it is concerned with the communication between apps and the daemon.

BUT, "However, KNotifications uses the application name for identification rather than the desktop entry" is possibly a similar issue to the one we're having. But it's likely to be something concerning the kwin/KNotifications relationship, rather than KNotifications/app relationship.

@pasqui23
Copy link
Contributor

So I've followed the instructions given to @peterhoeg in the 2nd bug report
Here are the properties of a notification window on Wayland:
2021_05_15_11_35_21
And here the same on X:
Screenshot_20210515_113831
Screenshot_20210515_113900

@pasqui23
Copy link
Contributor

Of note:Entire window class is plasmashell-wrapped.org.kde.plasmashell on Wayland, plasmashell.plasmashell on X
SImilarly windows type is Normal window on Wayland, and apparently empty on X

@samueldr
Copy link
Member

plasmashell-wrapped

I would bet this is the issue.

@pasqui23
Copy link
Contributor

@samueldr where do you think we shpuld look specifically?

@samueldr
Copy link
Member

samueldr commented May 15, 2021

My hunch is some special casing in kwin. I still haven't taken the time to look into that. That should be the first thing I do when I resume work on the PR monday, since that's a good clue. Nothing I can see is special casing on plasmashell.

@pasqui23
Copy link
Contributor

@samueldr any news?

@samueldr
Copy link
Member

samueldr commented Jun 2, 2021

(All looked at under Plasma Mobile...)

My current line of thoughts is that the window role is lost for some reason. This is supported by your previous screenshots, and my testing under Plasma Mobile shows me the same issue where window type is Normal.

AFAICT this condition is never true:

But if I force any m_plasmaShellSurface clients to not accept focus, while it breaks some things, the notification does stop taking over the focus.

--- a/xdgshellclient.cpp
+++ b/xdgshellclient.cpp
@@ -926,6 +926,7 @@ bool XdgToplevelClient::acceptsFocus() const
         }
         if (m_plasmaShellSurface->role() == PlasmaShellSurfaceInterface::Role::Notification ||
             m_plasmaShellSurface->role() == PlasmaShellSurfaceInterface::Role::CriticalNotification) {
+       return false;
             return m_plasmaShellSurface->panelTakesFocus();
         }
     }

This is not a solution, far from it. I simply don't know how to debug this kind of issues, and am learning along the way how all of this is setup within kwin, plasma, and friends.

This also does not affect the window sizing in Plasma Mobile, but it changes something for sure since it won't put the notification halfway off-screen.

I would assume if the window never lost it

@peterhoeg
Copy link
Member Author

peterhoeg commented Jun 2, 2021 via email

@pasqui23
Copy link
Contributor

pasqui23 commented Jun 2, 2021

I suspect #107595 is connected

@samueldr
Copy link
Member

samueldr commented Jun 2, 2021

@peterhoeg I don't have much confidence in my findings.

Reading through kwin's and plasma-* and kwayland* projects a bit hapazardly I found out that maybe it's totally expected that window roles are normal under wayland. Though I'm not confident either in those findings.

EDIT: yeah, forcing the Notification role through shellSurface->setRole(KWayland::Client::PlasmaShellSurface::Role::Notification) for Dialog::Notification "type" dialogs in src/plasmaquick/dialog.cpp crashes Plasma Mobile :/. Whatever this means makes me think the window having the "Normal" window type is a red herring.

@samueldr
Copy link
Member

samueldr commented Jun 2, 2021

@pasqui23 AFAIK this patch solved that.

@samueldr
Copy link
Member

samueldr commented Jun 3, 2021

Looks like I'm stabbing in the dark, at random, and only hitting irrelevant bits. I'd need a KDE/Plasma person to take a better look at that. It is exhausting to look into, since I'm mostly learning everything from scratch on the spot.

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/plasma-wayland-session-not-available-from-sddm/13447/3

@pasqui23
Copy link
Contributor

pasqui23 commented Aug 9, 2021

Not yet fixed in 5.22
image
However I have also installed Grid WM and I found that the spectacle window got managed by Grid when on X11 it was not, and the window for unlocking kwallet does not get positioned in the center as it does in X11.

Those put thogether suggest that somehow KWin thinks every window is a normal window.
Also the properties of a notification window on wayland
image
image

@pasqui23
Copy link
Contributor

pasqui23 commented Aug 9, 2021

Also the Plasma entry in the tasks list shows up also on openin plasmoid menus like kickoff or the notification menu.
Also the sound menu is put in the top left instead of bottom center
image

@pasqui23
Copy link
Contributor

pasqui23 commented Aug 9, 2021

Also also no clock widget

@pasqui23
Copy link
Contributor

pasqui23 commented Aug 9, 2021

Ok how do we contact the KDE devs for guidance?

@pasqui23
Copy link
Contributor

pasqui23 commented Aug 9, 2021

So here are the 3 commits that fixed this problem back in the day
https://invent.kde.org/frameworks/kwayland/-/commit/2f4cb0613b1c31ef3a32d6a7d47dbf484e2b85ef
https://invent.kde.org/plasma/kwin/-/commit/c81d8204f9a3a9fc806fedf7a8bb915dc19a7892
https://invent.kde.org/plasma/kwayland-integration/-/commit/fdb09dc29a3d628a0c64db1ec5eba613d8728b52

@samueldr This suggest that window roles are not expected to be "Normal" for notification,
especially since PlasmaShellSurfaceInterface::Private::setRole remain unchanged in master
https://invent.kde.org/frameworks/kwayland/-/blob/master/src/server/plasmashell_interface.cpp#L218

@pasqui23
Copy link
Contributor

pasqui23 commented Aug 9, 2021

So controlling the window type on a notification turn out that on X11 is revealed as empty.
Follows wold,possibly incorrect speculation: this suggest that the Notification role somehow is broken and on X11 the broken role behaves like a notification but on Wayland it behaves like a normal window.

@haizaar
Copy link

haizaar commented Sep 22, 2021

Devs, how can we, community, help to give some traction to this issue?
Judging by the complexity of the issue, will setting up a bounty through https://issuehunt.io/ help?

@CertainLach
Copy link
Member

Starting session via startplasma-wayland with #139459 and #139537 applied on top of 39b5332 fixes this issue

@samueldr
Copy link
Member

(Tangentially related, I'll be trying with the plasma-mobile PR tomorrow. Hopefully this is what fixes the issues there too.)

@samueldr
Copy link
Member

samueldr commented Sep 26, 2021

@CertainLach do you have a good enough mental model of what is happening to know if, instead of (ab)using the environment through tons of wrappers, we'd instead allow loading Qt plugins for all dependencies intrinsically regardless of the environment?

Note that I'm not talking about "unwrapping" the name of /proc/self/exe. I'm talking about the general idea of making Qt aware of library paths through a declarative and pure (enough?) method that does not rely on an unreliable method like the environment and wrappers.

I still think that using wrappers and the environment is the worst misfeature in NixOS integration we're still doing. (Not specific to Qt.)

@jansol
Copy link
Contributor

jansol commented Sep 27, 2021

If the wrapper magic is the problem, I wonder if the "proper" solution would also make screensharing/recording via pipewire, ssh-agent and fcitx (and presumably other IMEs) finally work on wayland as well.

@samueldr
Copy link
Member

@jansol unlikely that it alone would be the fix, but it might be part of making things easier.

It seems that their design for Wayland application "authentication" relies, among other things, on looking at the application names (from /proc/self/exe), finding either the desktop file or dbus description for said app. This is where "unwrapping" is currently needed. Without unwrapping, either directly the application name is wrong, or the desktop file / dbus files for .$app-wrapped don't exist.

Then there's the possibility that ".so" Qt plugins aren't found if a wrapper is missing or misconfigured. Though that one is less likely to be an issue, assuming the wrappers are configured.

It's possible every service needs to be tracked down to their hardcoded special casing in the different Wayland components to know for sure how they work.

@samueldr
Copy link
Member

@CertainLach's fixes works for Plasma Mobile!

💻📷 Screenshot

image

@CertainLach
Copy link
Member

@samueldr

we'd instead allow loading Qt plugins for all dependencies intrinsically regardless of the environment?

I agree with that qt wrapping is a mess, but i see the better story with qt plugins much as something like:

  • Patch qt to load plugins from something like /run/current-system/<current derivation hash>/ (I.e /nix/store/aabbcc-kwindowsystem would load plugins from /run/current-system/aabbcc-kwindowsystem)
  • Add nixos module to define plugin directories:
enviroment.qtPlugins = {
  "${pkgs.kwindowsystem}" = [
    pkgs.kwayland-integration
  ];
};

This way it would be explicit, and pure enough to not break things with errorously loaded plugins

@samueldr
Copy link
Member

samueldr commented Sep 27, 2021

I agree with that qt wrapping is a mess, but i see the better story with qt plugins much as something like:

* Patch qt to load plugins from something like `/run/current-system/<current derivation hash>/` (I.e `/nix/store/aabbcc-kwindowsystem` would load plugins from `/run/current-system/aabbcc-kwindowsystem`)

* Add nixos module to define plugin directories:

Breaks on non-NixOS systems, user profiles or nix-shell.

This way it would be explicit, and pure enough to not break things with errorously loaded plugins

The way I implemented in that old PR was pure from the inputs of a given derivation, while still staying compatible with the less pure automatic loading of plugins from profile directories that is already in use.

Though this discussion should probably be had elsewhere than this specific issue... Sorry for starting to roll the ball about that in the wrong location :).

@samueldr
Copy link
Member

Reopening; I think it also requires #139459, though I never tested for myself.

@samueldr samueldr reopened this Sep 30, 2021
@CertainLach
Copy link
Member

CertainLach commented Sep 30, 2021

#139459 isn't required for this fix, but it fixes other possible problems, kwin has some plugins with hardcoded app_ids, i.e glide uses this to blacklist animation on shell surfaces

@haizaar
Copy link

haizaar commented Oct 1, 2021

Huge thanks for fixing this!
Can someone else confirm it solves the problem for them? (we need to release the bounty :)

@jansol
Copy link
Contributor

jansol commented Oct 1, 2021

With #139537 applied running sleep 10; notify-send "foo" in a terminal does indeed no longer steals focus from typing this comment in my browser. The notification also doesn't show up in the task manager.

@andrevmatos
Copy link
Member

I can also confirm several other plasma wayland issues I was experiencing got fixed by #139537 :

  • blur on plasmoids, panel, yakuake/konsole are working again; before, I was only getting normal transparency and no blur
  • some other plasmoids popups besides notifications were also stealing focus and having an entry on taskbar, including launcher
  • volume/brightness HUD were also stealing focus, and were always localized on the top-left corner of the screen; they now appear in the center, as on X11

@haizaar
Copy link

haizaar commented Oct 21, 2021

@CertainLach can you please submit your PR that fixed this at the bounty page as a solution (to claim the bounty)?

image

@CertainLach
Copy link
Member

I tried to do that before, but "The issue is closed on GitHub" check is missing, i though there is some confirmation required from issue creator
Screenshot_20211021_193138

@Artturin
Copy link
Member

@alexanmtz

@Artturin Artturin reopened this Oct 21, 2021
@Artturin Artturin reopened this Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment