D8519: do not make the context menu a Window
This revision was automatically updated to reflect the committed changes. Closed by commit R289:f2f65dee2962: do not make the context menu a Window (authored by mkoller). REPOSITORY R289 KNotifications CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8519?vs=21431=23999 REVISION DETAIL https://phabricator.kde.org/D8519 AFFECTED FILES src/kstatusnotifieritem.cpp To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window
davidedmundson accepted this revision. This revision is now accepted and ready to land. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8519 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window
mkoller added a comment. Ping! Anything wrong with this 2 liner patch ? REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8519 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window
mkoller retitled this revision from "do not make the context menu a Window; do not force raise a window" to "do not make the context menu a Window". mkoller edited the summary of this revision. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8519 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window; do not force raise a window
graesslin added a comment. 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. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8519 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window; do not force raise a window
davidedmundson added a comment. 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. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8519 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window; do not force raise a window
graesslin added a comment. In https://phabricator.kde.org/D8519#160909, @davidedmundson wrote: > In https://phabricator.kde.org/D8519#160879, @graesslin wrote: > > > 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. > > > 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. 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. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8519 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window; do not force raise a window
davidedmundson added a comment. In https://phabricator.kde.org/D8519#160879, @graesslin wrote: > 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. 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. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8519 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window; do not force raise a window
graesslin added a comment. 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. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8519 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window; do not force raise a window
graesslin added a comment. In https://phabricator.kde.org/D8519#160813, @davidedmundson wrote: > > 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. 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 ;-) REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8519 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window; do not force raise a window
mkoller updated this revision to Diff 21431. mkoller added a comment. In https://phabricator.kde.org/D8519#160813, @davidedmundson wrote: > > 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. 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 ? REPOSITORY R289 KNotifications CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8519?vs=21427=21431 REVISION DETAIL https://phabricator.kde.org/D8519 AFFECTED FILES src/kstatusnotifieritem.cpp To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window; do not force raise a window
davidedmundson added inline comments. INLINE COMMENTS > kstatusnotifieritem.cpp:453 > d->menu = menu; > -d->menu->setParent(nullptr); > +//d->menu->setParent(nullptr); > } 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. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8519 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window; do not force raise a window
davidedmundson requested changes to this revision. davidedmundson added a comment. This revision now requires changes to proceed. > 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. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8519 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8519: do not make the context menu a Window; do not force raise a window
mkoller created this revision. mkoller added reviewers: davidedmundson, graesslin. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY 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. The removal of the force activation is done since according to kwin maintainer it is wrong and must be done by the SysTray itself. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8519 AFFECTED FILES src/kstatusnotifieritem.cpp To: mkoller, davidedmundson, graesslin Cc: #frameworks
