Review Request 123294: Fix check to see if we are the active window before showing notification in polkit agent

[Martin Gräßlin]

> On April 8, 2015, 4:08 p.m., Martin Gräßlin wrote:
> > AuthDialog.cpp, lines 320-321
> > <https://git.reviewboard.kde.org/r/123294/diff/1/?file=360483#file360483line320>
> >
> >     one event cycle might not be sufficient to figure out whether it's the active window.
> >     
> >     And honestly I have no idea how one could get the information whether the window manager processed it in a reliable way. What might be the most reliable is to wait for the window being added to KWindowSystem::stackingOrder (there's a changed signal). It would give at least the hint that the window manager processed the window being shown.
> 
> David Edmundson wrote:
>     The check to see if we're on top is currently broken. It looks at kwindowsystem::activeWindow which indirectly comes from looking a property on the root window. KWindowsystem only updates changes from the root window system every event loop. So we can't use the value directly after we've opened our dialog.
>     
>     Given we're calling forceWindowActive in policykitlistener when we show, so I don't actually need to see what the window manager has to say as I know we'll always be on top. To some extent we don't need this notification code at all, and I could just remove it.
>     
>     However, for the case that forceWindowActive doesn't work (such as running on Wayland) I left the code in.
> 
> Martin Gräßlin wrote:
>     forceWindowActive does not guarantee that you will have focus afterwards. It will also not (even less) guarantee that it's the top most window.
> 
> David Edmundson wrote:
>     If forceWindowActive doesn't force the window to be active that's a bug in kwin.
>     
>     In any case, for now I'll just delete that notification.

no, it isn't. Nothing in forceWindowActive doesn't guarantee it. What if at the same time another window calls it? You are interacting with a different system which doesn't give you the guarantee you expect. And this has nothing to do with KWin, but with how X11 works. No window manager in this world can guarantee this.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123294/#review78671
-----------------------------------------------------------


On April 8, 2015, 3:16 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123294/
> -----------------------------------------------------------
> 
> (Updated April 8, 2015, 3:16 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: polkit-kde-agent-1
> 
> 
> Description
> -------
> 
> KWindowSystem::activeWindow result is cached, we need to process an
> event loop for it to update.
> 
> 
> Diffs
> -----
> 
>   AuthDialog.h 561c6a0 
>   AuthDialog.cpp 16ee64d 
> 
> Diff: https://git.reviewboard.kde.org/r/123294/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20150409/a7fe45b2/attachment-0001.html>