GNOME Bugzilla – Bug 689106
ScreenShield: try harder to become modal, and catch failures
Last modified: 2014-08-20 19:33:48 UTC
Downstream report https://bugzilla.redhat.com/show_bug.cgi?id=878736
Created attachment 229933 [details] [review] ScreenShield: try harder to become modal, and catch failures The screenshield was not checking the return value of pushModal(), meaning that it believed it was fully locked when it was not. Later, calling popModal() would fail, causing an exception and blocking the unlock. Now we do nothing for user initiated actions, and fail with an explanatory message at idle time.
Created attachment 229937 [details] [review] ScreenShield: try harder to become modal, and catch failures The screenshield was not checking the return value of pushModal(), meaning that it believed it was fully locked when it was not. Later, calling popModal() would fail, causing an exception and blocking the unlock. Now we do nothing for user initiated actions, and fail with an explanatory message at idle time. gnome 3-6 patch
Review of attachment 229937 [details] [review]: ::: js/ui/screenShield.js @@ +584,3 @@ + // screen, where we're not affected by grabs + Main.notifyError(_("Failed to activate screen lock"), + _("The activation of the screen lock was blocked by an application that captured the pointer and the keyboard.")); Would probably be more correct to say the pointer _or_ the keyboard ?
If I'm reading the patch right, the screen will not be locked if the keyboard/mouse are 'stolen'. And there's no re-try as well, if the focus couldn't be obtained. If that's the case, this needs more work. For example, this happens to me when using workrave, which prompts often for breaks, and steals keyboard/mouse focus so that the user is forced to take breaks.
Sorry, but if the application does that it is broken, and it should be changed to steal focus and/or warp the mouse, not actively grab it. There is no way in the X protocol to know when a grab is released, so we'd have to continuously poll until we obtain it. Yes, X11 grab are broken, but there is little we can do about it. By the way, gnome-screensaver had the same problem: in place of grabs, it just refused to lock.
Hm, I didn't face this problem till 3.2 (didn't try 3.4) -- so looks like a regression from that POV.
I tried the patch (just patched /usr/share/gnome-shell/js/ui/screenShield.js in my Fedora install), and even when workrave isn't active the screen doesn't lock. Any pointers?
Just now reproduced with workrave not running, and without the patch. So it's not related to workrave.
Review of attachment 229937 [details] [review]: The code looks reasonable, but I'm unsure about the UI. Can you please run this by the designers? ::: js/ui/screenShield.js @@ +451,3 @@ + + // We failed to get a pointer grab, it means that + // something else has it. Try with a keyboard grab only Not sure about using a fallback here rather than failing directly @@ +584,3 @@ + // screen, where we're not affected by grabs + Main.notifyError(_("Failed to activate screen lock"), + _("The activation of the screen lock was blocked by an application that captured the pointer and the keyboard.")); With the fallback, only a keyboard grab is relevant, otherwise what Matthias says. Also I don't think using notifyError() is very useful here - this is triggered on idle, so the notification is almost certainly closed before the user has seen it. @@ +884,3 @@ + // No effect if we can't become modal + if (!this._becomeModal()) + return; Not sure silently ignoring a user request is a good idea.
*** Bug 687940 has been marked as a duplicate of this bug. ***
*** Bug 690195 has been marked as a duplicate of this bug. ***
*** Bug 690307 has been marked as a duplicate of this bug. ***
> The code looks reasonable, but I'm unsure about the UI. Can you please run this > by the designers? What is the UI here ? Screenshot ?
There is not much UI here, just a notification to be shown after coming back, explaining the issue. Following Florian's suggestion, I also added another notification, for when the lock is requested manually.
Created attachment 233805 [details] [review] ScreenShield: try harder to become modal, and catch failures The screenshield was not checking the return value of pushModal(), meaning that it believed it was fully locked when it was not. Later, calling popModal() would fail, causing an exception and blocking the unlock. Now when we fail we include an explanatory message, pointing the user to the actual cause of the issue.
Review of attachment 233805 [details] [review]: Makes sense.
I'd still want a UI review for this.
*** Bug 693546 has been marked as a duplicate of this bug. ***
Created attachment 235645 [details] [review] ScreenShield: try harder to become modal, and catch failures Rebased to master
Created attachment 235646 [details] Error after explicit lock from user menu
Created attachment 235647 [details] Error after implicit lock on idle
After applying the patch here http://bugzilla-attachments.gnome.org/attachment.cgi?id=229937 it seems to block the screen locking and suspending by the user completely. Now after clicking on "Lock" from the user-menu, nothing happens. Any suggestions? Thanks. System: openSUSE 12.3 RC1, gnome-shell-3.6.2.
The patch uses API that has been only introduced during the 3.7.x cycle, so applying it unmodified to the 3.6.2 version is not supposed to work. There are probably some errors in ~/.cache/gdm/session.log which hint at what exactly is not working. (For what it's worth, I do have a backported version of the patch locally that I plan to apply (and include in a 3.6.3 release) once we get designer approval)
I took the patch attached with comment 2 (and mentioned as gnome 3.6), but I understand. Would be very thankful if you attached the backported patch for 3.6.x here (or emailed me), as this would fix a rather important issue for users of openSUSE 12.3 (which comes with GNOME 3.6). Thanks a lot for the quick response!
(In reply to comment #24) > Would be very thankful if you attached the backported patch for > 3.6.x here (or emailed me), as this would fix a rather important issue for > users of openSUSE 12.3 (which comes with GNOME 3.6). Sorry, but I prefer to land the patch upstream first (the code is fine, and the designers are reviewing the strings as we speak, so I expect this to land later today), at which point they'll appear on the stable gnome-3-6 branch as well.
The strings from those two screenshots: "Failed to activate screen lock The screen lock is inhibited by an application capturing the keyboard." "Failed to activate screen lock The activation of the screen lock was blocked by an application that captured the keyboard." They both seem to say the same thing, so I don't think we need two different notifications. I'd probably go with: "Unable to lock Lock was blocked by an application"
Created attachment 235909 [details] [review] ScreenShield: try harder to become modal, and catch failures The screenshield was not checking the return value of pushModal(), meaning that it believed it was fully locked when it was not. Later, calling popModal() would fail, causing an exception and blocking the unlock. Now when we fail we include an explanatory message, pointing the user to the actual cause of the issue.
Pushed with Allan's proposed text. Attachment 235909 [details] pushed as 4dc9540 - ScreenShield: try harder to become modal, and catch failures
*** Bug 691094 has been marked as a duplicate of this bug. ***
*** Bug 692991 has been marked as a duplicate of this bug. ***
Thanks, I can confirm this fixes duplicate Bug 691094. With gnome-shell 3.6.3, locking now fails gracefully when a virtualbox guest has focus.
*** Bug 353437 has been marked as a duplicate of this bug. ***