The setParent() call in this patch was changing the window flags from Popup to Window, which leads to show the popup with a window frame when activated via a SysTray DBus call.
I don't know why setParent() is done here in the first place (could not find the change in git history).
If setParent() should still be done, at least the window flag must be set back to Popup.
Details
Details
Diff Detail
Diff Detail
- Repository
- R289 KNotifications
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
The removal of the force activation is done since according to kwin maintainer it is wrong and must be done by the SysTray itself.
But according the the status notifier item maintainer (defacto me), we cannot do this. It will break too many things.
The SNI spec is used by Unity 7, Gnome, and Plasma. Can we guarauntee their trays do this? (in fact even Plasma doesn't currently do this!)
I know it's putting you in the middle of an argument, which is unfair on you, sorry.
forceActiveWindow may be bad, but it works. It's not really focus stealing if we explicitly know the user performed the action. In an ideal world the original SNI spec would have passed a timestamp...and blah blah, but yeah.
For Wayland this won't work and will do nothing, which is fine. We do need to fix that properly in a different manner, but lets not break X in the meantime.
| src/kstatusnotifieritem.cpp | ||
|---|---|---|
| 453 | make setContextMenu safe against null pointers, prevents creating a dbus menu exporter when we don't care for it, prevents re-doing work when calling setLegacySystemTrayEnabled when the object is already in the desired state and prevents creating multiple KDBusMenuExporter objects if setContextMenu is called with the same menu more than once over the lifetime of the menu (which can actually happen with this patch). svn path=/trunk/KDE/kdelibs/; revision=1120745 (yes, it really was back in SVN) We take ownership of the menu in this method, and it's trying to prevent accidental double deletions. It's a bit quirky. If this was new code I wouldn't do it, but again we can't risk breaking existing stuff. It's weird that it changes the window type...that sounds like a bug; I'll happily approve setting it back. | |
Comment Actions
I thought this will be the case ...
And I still think my previous patch makes it even better than it is now (I don't understand why the current code raises the window only when it's already shown
but not otherwise)
So since you are the maintainer, what is your stance on this ?
Comment Actions
I plan to harden the window activation so that incorrect forceActiveWindow doesn't steal focus any more. It is unfortunately a common pattern and KWin in that area just sucks. I have a plan to move that forward without breaking stuff and addressing Wayland at the same time. Once again thinking in Wayland terms helps ;-)
Comment Actions
Actually even on X it is quite simple to get it right. We have the window Id, so the host needs to update the xTime in the window. The only question is whether Qt reads it back. With an up to date xTime a simple activate without a force should work.
Comment Actions
You're basing off an assumption. That clicking should raise the window. It isn't necessarily the case.
There could be no window, (see current keyboard layout item), or new windows or something completely different.
We could guess some heuristic based on if WindowId is set, dont' call activated, but do stuff, but that's in random guessing client-behaviour territory.
Comment Actions
Sorry I didn't express myself correctly. If we have the window id the host should update the xTime on the window whenever the associated item is clicked. Then the window can raise/activate itself as it has an up to date xTime.
This is for example done in kglobalaccell: the timestamp of the shortcut is sent to the application through dbus. The application updates the xTime and activates the window. The window manager is happy, the xTime allows it.
Comment Actions
Ah, I 1000% agree it /should/ work like that.
My very first comment on here ended with:
In an ideal world the original SNI spec would have passed a timestamp...
So bringing discussion back to this patch.
- We need forceActiveWindow for other hosts/WMs
- From Plasma. I could pass the timestamp to the SNI item via some sideloaded extra interface
- This would allow you to ignore forceActiveWindow in kwin without breaking this specific thing.
Comment Actions
Maybe we can drop the force activate for all hosts? Are there any implementations left which are on X11? At least Ubuntu is Wayland based.