D8416: really raising window when shown from systray
mkoller abandoned this revision. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8416 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8416: really raising window when shown from systray
graesslin added inline comments. INLINE COMMENTS > mkoller wrote in kstatusnotifieritem.cpp:653 > "without checking who sends it" > > Who shall be allowed to do so ? > > As said, the original problem is that a click in SysTray does not raise it, > but as I understand it, the KStatusNotifierItem is owned by the application > which receives the "Activate" trigger sent via DBus, so it would be the > application which would raise itself. Am I right ? Yes you got that right. And yes: if the systray would use the force active window request it would've fine and correct. From X11 perspective it is a tool which activated windows of other applications. Just like e.g. the Taskmanager. And it is the window which was interacted with. So from focus stealing prevention everything is fine. If that StatusNotifier on the other hand activated itself it looks like focus stealing to the window manager. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8416 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8416: really raising window when shown from systray
mkoller added inline comments. INLINE COMMENTS > graesslin wrote in kstatusnotifieritem.cpp:653 > It only works on X11 because KWin has a bug. KWin allows the force (which is > in X11 Terms a "from tool" request) without checking who sends it. I will > look into hardening this in KWin. Then the existing code won't work anymore. "without checking who sends it" Who shall be allowed to do so ? As said, the original problem is that a click in SysTray does not raise it, but as I understand it, the KStatusNotifierItem is owned by the application which receives the "Activate" trigger sent via DBus, so it would be the application which would raise itself. Am I right ? REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8416 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8416: really raising window when shown from systray
graesslin added inline comments. INLINE COMMENTS > mkoller wrote in kstatusnotifieritem.cpp:653 > I'm a bit confused now. > The patch solves my issue for X11 as it does some lines below already, but > you say that this is wrong for Wayland but a solution for wayland does not > exist right now. > I don't use Wayland, I'm on X11 and on X11 the solution works. > Why can't we simply add this 2 lines until there will be "the correct way" > for wayland as you suggest ? > Since you say the current code is wrong anyway, it needs some adaption anyway. > So until then, the current code plus my fix improves the situation for all > X11 users, and for Wayland you can change the code when all APIs are in place. It only works on X11 because KWin has a bug. KWin allows the force (which is in X11 Terms a "from tool" request) without checking who sends it. I will look into hardening this in KWin. Then the existing code won't work anymore. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8416 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8416: really raising window when shown from systray
mkoller added inline comments. INLINE COMMENTS > graesslin wrote in kstatusnotifieritem.cpp:653 > The correct approach would be sni telling the host to activate the window. > The host could then communicate with the X11 window manager/Wayland > compositor through a special interface that the window should be activated. > > Yes, such an interface does not yet exist. But we need it for Wayland anyway. > An app requesting to get activated just does not exist in a Wayland world. > The action is performed on the host, so the host is the only one who has > enough rights to activate another window. I'm a bit confused now. The patch solves my issue for X11 as it does some lines below already, but you say that this is wrong for Wayland but a solution for wayland does not exist right now. I don't use Wayland, I'm on X11 and on X11 the solution works. Why can't we simply add this 2 lines until there will be "the correct way" for wayland as you suggest ? Since you say the current code is wrong anyway, it needs some adaption anyway. So until then, the current code plus my fix improves the situation for all X11 users, and for Wayland you can change the code when all APIs are in place. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8416 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8416: really raising window when shown from systray
graesslin added inline comments. INLINE COMMENTS > mkoller wrote in kstatusnotifieritem.cpp:653 > Do you mind telling me the correct approach ? The correct approach would be sni telling the host to activate the window. The host could then communicate with the X11 window manager/Wayland compositor through a special interface that the window should be activated. Yes, such an interface does not yet exist. But we need it for Wayland anyway. An app requesting to get activated just does not exist in a Wayland world. The action is performed on the host, so the host is the only one who has enough rights to activate another window. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8416 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8416: really raising window when shown from systray
mkoller added inline comments. INLINE COMMENTS > graesslin wrote in kstatusnotifieritem.cpp:653 > The other code is also not correct and should be removed. Do you mind telling me the correct approach ? REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8416 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8416: really raising window when shown from systray
graesslin added inline comments. INLINE COMMENTS > mkoller wrote in kstatusnotifieritem.cpp:653 > This code change will work as good as the other part of this file in wayland, > since just about 30 lines below my change you find exactly the same 2 lines > of code. > Check the "else" branch in case the window is already mapped. > > So why is the existing code correct but my change not ? > > "The proper way is to ask the systray to raise the window, ..." > In my case, I AM the systray (I implement my own systray) and what I do when > the Status Notifier Item is clicked, I just call the DBus interface class > method Activate(pos.x(), pos.y()); > What's wrong with this approach ? > Do you say it's the SysTray which should call these 2 lines I added ? > I think the SysTray should not even know that the client application shows a > window when the status notifier item was clicked. It just asks for the > "Activate" action - whatever the client does with that. > > "If that code worked it means KWin's focus stealing prevention is buggy." > Then it's buggy since it works. (my setting is "Low") The other code is also not correct and should be removed. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8416 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8416: really raising window when shown from systray
mkoller added inline comments. INLINE COMMENTS > graesslin wrote in kstatusnotifieritem.cpp:653 > Please read the documentation of this API call. It is not meant for > applications to use this! > > Please also note that this won't work on Wayland at all. > > The proper way is to ask the systray to raise the window, which then talks to > the window manager / wayland compositor to activate it. > > Activating from client side is nothing else than focus stealing. If that code > worked it means KWin's focus stealing prevention is buggy. This code change will work as good as the other part of this file in wayland, since just about 30 lines below my change you find exactly the same 2 lines of code. Check the "else" branch in case the window is already mapped. So why is the existing code correct but my change not ? "The proper way is to ask the systray to raise the window, ..." In my case, I AM the systray (I implement my own systray) and what I do when the Status Notifier Item is clicked, I just call the DBus interface class method Activate(pos.x(), pos.y()); What's wrong with this approach ? Do you say it's the SysTray which should call these 2 lines I added ? I think the SysTray should not even know that the client application shows a window when the status notifier item was clicked. It just asks for the "Activate" action - whatever the client does with that. "If that code worked it means KWin's focus stealing prevention is buggy." Then it's buggy since it works. (my setting is "Low") REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8416 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8416: really raising window when shown from systray
graesslin requested changes to this revision. graesslin added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kstatusnotifieritem.cpp:653 > +KWindowSystem::raiseWindow(associatedWidget->winId()); > +KWindowSystem::forceActiveWindow(associatedWidget->winId()); > emit q->activateRequested(true, pos); Please read the documentation of this API call. It is not meant for applications to use this! Please also note that this won't work on Wayland at all. The proper way is to ask the systray to raise the window, which then talks to the window manager / wayland compositor to activate it. Activating from client side is nothing else than focus stealing. If that code worked it means KWin's focus stealing prevention is buggy. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8416 To: mkoller, davidedmundson, graesslin Cc: #frameworks
D8416: really raising window when shown from systray
mkoller created this revision. mkoller added reviewers: davidedmundson, graesslin. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY I have akregator running and an icon is shown in the systray. When the main window is currently not shown (only the systray icon) and I click on the systray icon, the main window is restored but not brought to the foreground, so it opens just behind the other windows I have open, which is not what I expect. This patch makes sure to raise the window. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D8416 AFFECTED FILES src/kstatusnotifieritem.cpp To: mkoller, davidedmundson, graesslin Cc: #frameworks
