GNOME Bugzilla – Bug 619955
Add pin support to the lock screen
Last modified: 2014-09-01 04:24:39 UTC
I think it makes a lot of sense to have the Shell implement the screen lock. * We want the appearance and behavior to be consistent with the Shell. * In some cases notification messages should appear over/on the locked screen * Need careful coordination with the Overview * Shell should handle hotkeys both to lock screen and to pass some through the locked screen * There are things we've wanted to do with screensavers that really need a compositor Perhaps something like: http://live.gnome.org/GnomeShell/Design/Whiteboards/ScreenLock
The idle detection and inhibit interfaces are handled by gnome-session these days. The remaining roles are: * Service Lock() dbus requests from the session * Service Lock() and UnLock() requests from ConsoleKit / system * Show animations/themes when idle and not asleep or on battery power * Block access to the session when idle if configured to do so or after an explicit Lock() * When locked display an authentication screen to authorize the unlock
Hi William, are the items you mentionned in your last comment still true? If not, what are the remaining things blocking this? With the new GDM, the screen lock sticks out like a sore thumb :)
I suppose things may change once ConsoleKit is replaced by systemd. Not sure.
*** Bug 660945 has been marked as a duplicate of this bug. ***
I think that screen-lock and gdm should be merged. They are exactly the same. - Offer to log a given selected user by typing his password - Offer to switch to another user - Indicating if a given user is already connected The rational is simple: take someone unused to computer. Explain him/her to shut down the computer and to put it into sleep (people quickly understand the difference). Then, try to answer : why is the "password screen" different in both cases?
This is being worked on: https://live.gnome.org/ThreePointThree/Features/ScreenLock
I'm making this a meta bug for the 3.6 feature. It is mainly intended to set Blocks/Depends on for bugs affecting other products.
*** Bug 678284 has been marked as a duplicate of this bug. ***
Created attachment 218268 [details] [review] screenShield: add initial functionality We are replacing the gnome-screensaver module with with a screen shield that is part of gnome-shell. This patch fades out the screen on idle and shows a shield with a background image when there is activity again. The shield can be removed with a key or button press.
Created attachment 218269 [details] [review] Add UnlockDialog for unlocking the screen shield When the screenshield is deactivated, instead of going back to the session immediately, prompt the user for authentication. This essentially reinstates what used to be provided by gnome-screensaver.
Created attachment 218270 [details] [review] ScreenShield: improve locking/modal policy Ensure that the lightbox is above everything (including the screenlock itself) when fading in - this allows for fading while showing the unlock dialog. Also, don't pushModal again when already locked, or we won't get out of it.
Created attachment 218271 [details] [review] Modal stack: fix handling of destroyed actors Destroyed modal actors should be completely removed from the modal stack automatically, including leaving modality if needed. This allows for destroying modal dialogs without calling close().
Created attachment 218272 [details] [review] ScreenShield: reset isLocked flag when unlocking Otherwise further locking will get us confused and cause "incorrect pop" JS errors (as well as keeping the shell modal forever).
Created attachment 218273 [details] [review] LayoutManager: reverse the visibleInFullscreen flag Change visibleInFullscreen to be trackFullscreen. If true, visibility is fully bound to fullscreen status, if false, no change is made. This allows to avoid set_skip_paint(), while not messing with visibility of actors that are sometimes hidden for other reasons. The flag was reversed because only the panel uses it, so false is a more useful default.
Created attachment 218274 [details] [review] ScreenShield: use LayoutManager for creating the actor This ensures that the screen shield is created at the right stacking level, so the message tray is visible in the lock screen (showing PAM messages, critical notifications and the on screen keyboard)
Created attachment 218275 [details] [review] LayoutManager: don't track the status of the screensaver on DBus We are the screensaver now, and internal objects can track the locking status better themselves. And to do so, add two signals to ScreenShield. Also, kill some overly verbose debug logging.
Created attachment 218276 [details] [review] MessageTray: filter notifications in the screen lock Track screen lock status and filter notifications according to a per-notification boolean "privacySensitive" status. This allows to show the keyboard and shell's transient notifications while still hiding most notifications.
Created attachment 218277 [details] [review] Show the panel above the screenshield when locked Track locked status and use it to provide a reduced version of the panel in the locked screen (essentially, it shows the date menu without events, battery, volume and network without settings)
Created attachment 218278 [details] [review] Remove usage of ScreenSaver DBus interface There is no gnome-screensaver on the bus now, all users should be updated to the internal screen shield.
Created attachment 218279 [details] [review] ScreenShield: implement curtain design This separates the screen shield into two main screens. One is the lock screen, and it is shown when coming back from idle status and when failing authentication. The other is the actual unlock dialog. Moving from the first to the second is possible by pressing Escape or by dragging an arrow on the bottom on the screen.
Created attachment 218280 [details] [review] Do not use constraints for alignment ClutterConstraints on non-toplevel actors cause a lot of allocation cycle warnings, and can potentially impact performance.
Created attachment 218281 [details] [review] Consolidate creation of login and unlock dialog in the screenshield The design calls for the curtain to appear in the gdm greeter too. Implement this by having the screenshield manage the login dialog (delegating its creation to SessionMode).
Created attachment 218282 [details] [review] ShellEntry: make isPassword param changeable at runtime Make it possible to control the visibility of "Show/hide text" item at runtime, to reuse the same entry for both password and non-password prompts.
Created attachment 218283 [details] [review] UnlockDialog: a few usability improvements Make sure that key focus is on the entry rather than the button, add a context menu and make it possible to unlock with Enter.
Created attachment 218284 [details] [review] Add a clock to the lock screen Start implementing the lock screen design by adding a big white clock in the middle.
Created attachment 218285 [details] [review] ScreenShield: show notifications in the locked screen Rework lock handling in the message tray to separate summary handling and notification handling. Now the message tray is completely hidden when the screen is locked, but exceptions can be made for individual transient notifications. Non transient sources are shown in the middle of the lock screen. Resident notifications (such as those from Rhythmbox) are shown in full, while dpersistent ones are displayed as icon and message count.
Created attachment 218286 [details] [review] Screen Shield: animate manual locking When the screen shield is activated from the user menu, animate it instead of showing it abruptly.
Created attachment 218287 [details] [review] Show more of the status area in the locked screen Restore showing the user name in the upper right corner. Show keyboard layout switcher and show accessibility menu, to ensure that all features needed for successful unlocking can be enabled.
Created attachment 218288 [details] [review] Login Dialogs: update styling to match mockups Remove backgrounds, change button styling, reduce font sizes.
Created attachment 218289 [details] [review] UnlockDialog: add the "Login with other user" link Add a small link to summon gdm and the real login screen. Also, change layout and styling of this and the "Not listed" link in gdm to match the mockups.
Created attachment 218290 [details] [review] ScreenShield: make the entire lock screen draggable The arrow is small and can be difficult to target. Instead allow dragging the background to show the unlock dialog.
Created attachment 218291 [details] [review] ScreenShield: add a watchdog process in case the shell crashes If the shell crashes while the screenshield is active, the lock screen is teared down, and that's potentially a security issue. To mitigate that, add a process that keeps a full black screen on top of everything (except the shell itself) and grabs the mouse and keyboard if the shell crashes.
Created attachment 218292 [details] [review] Layout: ignore fullscreen windows when the lock screen is active With the new watchdog, there will always be a fullscreen window on every monitor, so the panel would be hidden. As we already have policy on what to show when locked, ignore fullscreen windows in that case.
Created attachment 218293 [details] [review] User Menu: wait for the animation before suspending Suspending right away after starting the lock means the animation runs after the computer is restarted, and for an instant you can the see the running session. Not animating at all doesn't work because you need to go back to main loop to draw, and it looks bad anyway. Also, don't animate when switching session.
Created attachment 218294 [details] [review] ShellDBus: export screensaver interface gnome-session and gnome-settings-daemon rely on the screensaver interface to know the locked state. Since gnome-screensaver is no longer running, it's up to gnome-shell to provide it.
Are you attaching this patch stack because it's ready, or because you want preliminary feedback? I see lots of FIXMEs and logging all over it.
Review of attachment 218268 [details] [review]: Sure.
Review of attachment 218269 [details] [review]: ::: js/ui/sessionMode.js @@ +88,3 @@ createSession: Main.createUserSession, + // FIXME: merge back in main stylesheet + extraStylesheet: global.datadir + '/theme/gdm.css', Do so now. ::: js/ui/unlockDialog.js @@ +20,3 @@ + +function _fadeInActor(actor) { + if (actor.opacity == 255 && actor.visible) You already check for this when calling _fadeInActor. @@ +24,3 @@ + + actor.show(); + let [minHeight, naturalHeight] = actor.get_preferred_height(-1); No. Especially because the thing you are fading is a label. @@ +27,3 @@ + + actor.opacity = 0; + actor.set_height(0); ??? @@ +34,3 @@ + transition: 'easeOutQuad', + onComplete: function() { + this.set_height(-1); ??? @@ +59,3 @@ +// A widget showing the user avatar and name +const UserWidget = new Lang.Class({ + Name: 'UserWidget', Can we create a common widget between this and the user menu? @@ +96,3 @@ + + destroy: function() { + // clean up signal handlers Remove. @@ +118,3 @@ + iconFile = null; + } else { + this._label.text = ""; Single quotes. @@ +130,3 @@ + _setIconFromFile: function(iconFile) { + this._iconBin.set_style('background-image: url("' + iconFile + '");' + + 'background-size: contain;'); I hope we can get rid of this awful junk in the future. @@ +236,3 @@ + + _onReset: function() { + // I'm not sure this is emitted for external greeters... Find out. @@ +245,3 @@ + this._haveFingerprintReader = false; + + // FIXME: the greeter has a GSettings key for disabling fingerprint auth ... @@ +284,3 @@ + if (serviceName != GdmLoginDialog.PASSWORD_SERVICE_NAME) + return; + Main.notifyError(problem); No.
Created attachment 218295 [details] [review] Fix notifications from PAM Since the introduction of the watchdog, we are always in fullscreen when the lock screen is active, and this prevents showing PAM messages. Fix that by making those messages critical. Also, fix showing the notification when the queue contains something else before the allowed ones.
Review of attachment 218270 [details] [review]: ::: js/ui/screenShield.js @@ +43,3 @@ this._settings = new Gio.Settings({ schema: SCREENSAVER_SCHEMA }); + this._isModal = false; Christ. When can this be re-entrant? *Why* can this be re-entrant? @@ +121,3 @@ + this._dialog._group.raise_top(); + } catch(e) { + // FIXME: this is for debugging purposes This needs to go away. @@ +141,3 @@ _onUnlockSucceded: function() { this._dialog.destroy(); + this._dialog = null; Squash with original patch.
Review of attachment 218271 [details] [review]: I'm not sure I like the implicit destroy approach. Modal is something we should take very carefully.
Review of attachment 218272 [details] [review]: Squash this with whatever patch you had before.
Review of attachment 218273 [details] [review]: You cannot skip set_skip_paint. It will not work. Trust me on this. ::: js/ui/layout.js @@ +38,3 @@ + this.addChrome(this.panelBox, { affectsStruts: true, + trackFullscreen: true + }); braces go on the same line @@ +698,3 @@ else visible = true; + actorData.actor = !visible; ??? I'm quite sure you have this wrong.
Review of attachment 218274 [details] [review]: Yeah, I like this. The "actors creating things in Main.uiGroup whenever they feel like it" code is a bit messy. It would be nice if we could move everything to layout.js (and hopefully remove all explicit interaction with Main.uiGroup out), but that's a separate patch
Review of attachment 218275 [details] [review]: I'd still like some separation of concerns between the two components. I wouldn't be the biggest fan if random components started messing with Main.screenShield just because they can. > Also, kill some overly verbose debug logging. ... You do know about git rebase --interactive, right? ::: js/ui/layout.js @@ +695,3 @@ else visible = true; + actorData.actor.visible = visible; ...
Review of attachment 218276 [details] [review]: Not the biggest fan of the message tray randomly changing modes while in a screen shield. What happens if a notification is currently showing, and then we go to a screen shield? Is this just talking about banner notifications, or are we hiding sources too? ::: js/ui/keyboard.js @@ +560,3 @@ + + get privacySensitive() { + return false; ??? ::: js/ui/messageTray.js @@ +819,3 @@ + setPrivacySensitive: function(sensitive) { + this.privacySensitive = sensitive; Not the biggest fan of the silly getters/setters, but they're already in place, so why not. @@ +1579,3 @@ + this._isScreenLocked = true; + Main.screenShield.connect('lock-status-changed', Lang.bind(this, this._onScreenLockStatusChanged)); And lookie here.
Review of attachment 218277 [details] [review]: No. Let's stop this. It's probably just better to implement a new session mode and spawn off a new gnome-shell. This is just awful. ::: js/ui/main.js @@ +207,3 @@ magnifier = new Magnifier.Magnifier(); statusIconDispatcher = new StatusIconDispatcher.StatusIconDispatcher(); + screenShield = new ScreenShield.ScreenShield(); ???
Created attachment 218297 [details] [review] Allow the shell to run without the screenshield The screenshield requires gdm 3.5, which can be problematic in jhbuild configurations. Allow transparent fallback to gnome-screensaver in that case.
Review of attachment 218284 [details] [review]: ::: js/ui/dateMenu.js @@ +193,2 @@ setLockedState: function(locked) { + this.actor.visible = !locked; wtf ::: js/ui/screenShield.js @@ +39,3 @@ + + // I wonder why dateMenu.js doesn't use this, it was designed exactly + // for that It does. @@ +51,3 @@ + _updateClock: function() { + // Ignore the :clock property of GnomeWallClock, as we want + // to format it differently ughhhhh
Review of attachment 218279 [details] [review]: ::: js/ui/layout.js @@ +35,3 @@ this.screenShieldGroup = new St.Widget({ name: 'screenShieldGroup', + visible: false, + clip_to_allocation: true, Why? ::: js/ui/screenShield.js @@ +60,3 @@ + + // This is reimplemented manually instead of using a ClutterAction because + // we want the ability to cancel the action partway through How is that not possible with a ClutterDragAction? You just remove the meta.
Review of attachment 218278 [details] [review]: .
I'm done reviewing for the day. Somebody else can pick up the pieces if they really want to.
(In reply to comment #36) > Are you attaching this patch stack because it's ready, or because you want > preliminary feedback? I see lots of FIXMEs and logging all over it. I'm attaching it because Marina asked, and because I thought it was ready. I didn't remember this many FIXMEs... (In reply to comment #38) > Review of attachment 218269 [details] [review]: > > ::: js/ui/unlockDialog.js > @@ +20,3 @@ > + > +function _fadeInActor(actor) { > + if (actor.opacity == 255 && actor.visible) > > You already check for this when calling _fadeInActor. (Here and in the next ones) _fadeInActor was copy-pasted from loginDialog.js. > @@ +59,3 @@ > +// A widget showing the user avatar and name > +const UserWidget = new Lang.Class({ > + Name: 'UserWidget', > > Can we create a common widget between this and the user menu? I don't think so... The one in the user menu needs to be a PopupMenuItem, and needs to hold the combobox. This one doesn't. > @@ +245,3 @@ > + this._haveFingerprintReader = false; > + > + // FIXME: the greeter has a GSettings key for disabling fingerprint > auth > > ... ... is my feeling too. I'd like to hear from people dealing with lockdown, if this is needed (and how). After all, if you can lock a GSetting key, you can also put a pam_deny.so at the beginning of gdm-fingerprint. > @@ +284,3 @@ > + if (serviceName != GdmLoginDialog.PASSWORD_SERVICE_NAME) > + return; > + Main.notifyError(problem); > > No. Why no? This is identical to the login screen behaviour. (In reply to comment #40) > Review of attachment 218270 [details] [review]: > > ::: js/ui/screenShield.js > @@ +43,3 @@ > this._settings = new Gio.Settings({ schema: SCREENSAVER_SCHEMA }); > > + this._isModal = false; > > Christ. When can this be re-entrant? *Why* can this be re-entrant? Locking can be explicit, or triggered by session presence changing on the bus. Both things can happen at any time, and be interleaved in any way. This seemed the easiest way to deal with it to me. (In reply to comment #43) > Review of attachment 218273 [details] [review]: > > You cannot skip set_skip_paint. It will not work. Trust me on this. Uhm... Because? Also, isn't Shell.GenericContainer (and set_skip_paint()) generally deprecated? (In reply to comment #45) > Review of attachment 218275 [details] [review]: > > I'd still like some separation of concerns between the two components. I > wouldn't be the biggest fan if random components started messing with > Main.screenShield just because they can. This is JS, you can already mess with anything. And if your concern is for extensions, they can already Util.killall('gnome-screensaver') (In reply to comment #46) > Review of attachment 218276 [details] [review]: > > Not the biggest fan of the message tray randomly changing modes while in a > screen shield. What happens if a notification is currently showing, and then we > go to a screen shield? > > Is this just talking about banner notifications, or are we hiding sources too? When the screen shield is activated, the active notification (if not showWhenLocked) is closed, and replaced by the next showWhenLocked one, if any. The summary item notification and summary are forced to close. (In reply to comment #47) > Review of attachment 218277 [details] [review]: > > No. Let's stop this. > > It's probably just better to implement a new session mode and spawn off a new > gnome-shell. This is just awful. Sorry, spawing a new gnome-shell is not possible, if you want transparent notifications (expecially for resident ones like rhythmbox). Also, running gnome-shell without mutter is just not possible, so instead of a nice fading effect you'd see the compositor replaced and windows undecorated.
(In reply to comment #50) > > ::: js/ui/screenShield.js > @@ +60,3 @@ > + > + // This is reimplemented manually instead of using a ClutterAction > because > + // we want the ability to cancel the action partway through > > How is that not possible with a ClutterDragAction? You just remove the meta. Uhm, I didn't think of that. I don't think it changes much, anyway: you rename button-release-event to drag-end, motion-event to drag-motion and button-press-event to drag-begin...
(In reply to comment #53) > Sorry, spawing a new gnome-shell is not possible, if you want transparent > notifications (expecially for resident ones like rhythmbox). Also, running > gnome-shell without mutter is just not possible, so instead of a nice fading > effect you'd see the compositor replaced and windows undecorated. Yeah, my solution isn't workable at all. Really, the thing that I'm concerned is that we have a lot of complicated interdependencies in the code, and none of it is tested at all. *None of it is tested at all*. There have been bugs that have existed from day 1 of GNOME 3.0 that we've only found recently, where the code is just outright broken. It never worked. That shouldn't ever happen. Adding lots of tightly coupled components is the exact thing that we don't want to do if we eventually want to get the Shell code to be automatically tested. There's lots of potential gotchas where this sort of "lockdown mode" thing could trip us up. What if a panel menu goes away for a minute, and then comes back? e.g. somebody flips the HW bluetooth switch, or if somebody adds/removes a keyboard layout. Are we sure that lockdown mode will be set/unset properly? (In reply to comment #54) > Uhm, I didn't think of that. I don't think it changes much, anyway: you rename > button-release-event to drag-end, motion-event to drag-motion and > button-press-event to drag-begin... So, I'm actually curious: when do you need to stop the drag early? It just seems like a silly UI: I'm dragging my mouse and suddenly the entire curtain drops right back down to the bottom, mouse still being pressed down. I can't imagine any scenario when that's good. Also, you wouldn't have to have the drag-motion handler, as Clutter moves the actor automatically.
(In reply to comment #53) > (Here and in the next ones) _fadeInActor was copy-pasted from loginDialog.js. Welp. > I don't think so... The one in the user menu needs to be a PopupMenuItem, and > needs to hold the combobox. This one doesn't. I simply meant the thing that shows the user avatar. That's all you have here, right? We can share those two, no? > Why no? This is identical to the login screen behaviour. Sorry, I didn't realize we had a lot of code that was already broken. I don't see how using notifyError to expose a programmer error to the user is helpful in any way (what are they going to do about it?), but sure. If only GNOME had an automatic bug reporting service, integrated into the system itself... ( and then I can switch gnome-shell-extensions to use it :) ) > Locking can be explicit, or triggered by session presence changing on the bus. > Both things can happen at any time, and be interleaved in any way. This seemed > the easiest way to deal with it to me. Can you give an example of how this would happen in real life? > Uhm... Because? > Also, isn't Shell.GenericContainer (and set_skip_paint()) generally deprecated? No. I will be very surprised if visible works. I've tried it before, and I remember it breaking various things... what it broke, I do not remember. > (In reply to comment #45) > > Review of attachment 218275 [details] [review] [details]: > > > > I'd still like some separation of concerns between the two components. I > > wouldn't be the biggest fan if random components started messing with > > Main.screenShield just because they can. > > This is JS, you can already mess with anything. And if your concern is for > extensions, they can already Util.killall('gnome-screensaver') My concern is for code clarity and cleanliness. Just because we have the opportunity to use exceptions as gotos in JS and make everything one big file doesn't mean we should. Maybe disallowing access to Main.screenShield is a bit harsh, but if we need some form of "session state", it should be global. Maybe we could have the session mode be able to be changed at runtime, and let components adapt? Main.sessionMode.connect('changed', function() { /* ... */ }); Main.sessionMode.setNewSessionMode(SessionMode.LOCKED); I wonder how Florian feels about that... maybe it's not the best solution, but the two feel similar enough that I'm concerned we're duplicating efforts here. > (In reply to comment #46) > > Review of attachment 218276 [details] [review] [details]: > > > > Not the biggest fan of the message tray randomly changing modes while in a > > screen shield. What happens if a notification is currently showing, and then we > > go to a screen shield? > > > > Is this just talking about banner notifications, or are we hiding sources too? > > When the screen shield is activated, the active notification (if not > showWhenLocked) is closed, and replaced by the next showWhenLocked one, if any. > The summary item notification and summary are forced to close. OK.
Created attachment 218450 [details] [review] UnlockDialog: show a numeric keypad when using PIN authentication When using pam_pin.so to login with a numeric PIN, show a numeric keypad to make it easier to type, especially on touch devices. This is the last "feature" patch. I'll start integrating review comments next, although I'd prefer not to attach the updates, to avoid losing the sequence.
(In reply to comment #46) > Review of attachment 218276 [details] [review]: > > ::: js/ui/keyboard.js > @@ +560,3 @@ > + > + get privacySensitive() { > + return false; > > ??? MessageTray.Source has a privacySensitive getter based on notifications, but KeyboardSource doesn't show notifications, so it needs to override it. (privacySensitive == false means "show me in the locked screen", I couldn't come up with a better name)
(In reply to comment #47) > Review of attachment 218277 [details] [review]: > ::: js/ui/main.js > @@ +207,3 @@ > magnifier = new Magnifier.Magnifier(); > statusIconDispatcher = new StatusIconDispatcher.StatusIconDispatcher(); > + screenShield = new ScreenShield.ScreenShield(); > > ??? The screenShield must be created before the panel, or the latter cannot connect to lock-status-changed signals.
(In reply to comment #49) > Review of attachment 218284 [details] [review]: > > ::: js/ui/dateMenu.js > @@ +193,2 @@ > setLockedState: function(locked) { > + this.actor.visible = !locked; > > wtf We use a different clock in the lock screen, the date menu is not needed.
(In reply to comment #60) > We use a different clock in the lock screen, the date menu is not needed. It seems like you really want to use a session mode so bad, but can't because of the current architecture.
(In reply to comment #61) > (In reply to comment #60) > > We use a different clock in the lock screen, the date menu is not needed. > > It seems like you really want to use a session mode so bad, but can't because > of the current architecture. Generally speaking, yeah, I want a different session mode. But also I don't want to destroy and recreate session components, as some of them (network menu/agent, bluetooth menu, polkit agent) have non-trivial setup costs. Also, just to complicate matters, the screenshield is used by GDM mode too...
Created attachment 218636 [details] [review] Add UnlockDialog for unlocking the screen shield When the screenshield is deactivated, instead of going back to the session immediately, prompt the user for authentication. This essentially reinstates what used to be provided by gnome-screensaver. It is implemented by factoring out common code from gdm/loginDialog.js and ui/userMenu.js
Created attachment 218637 [details] [review] ScreenShield: improve locking/modal policy Ensure that the lightbox is above everything (including the screenlock itself) when fading in - this allows for fading while showing the unlock dialog. Also, don't pushModal again when already locked, or we won't get out of it.
Created attachment 218638 [details] [review] LayoutManager: reverse the visibleInFullscreen flag Change visibleInFullscreen to be trackFullscreen. If true, visibility is fully bound to fullscreen status, if false, no change is made. This allows to avoid set_skip_paint(), while not messing with visibility of actors that are sometimes hidden for other reasons. The flag was reversed because only the panel uses it, so false is a more useful default.
I'm only uploading patches for which I made substantial changes. The complete stack (including rebases) is in the git branch, as usual.
Created attachment 218641 [details] [review] ScreenShield: implement curtain design This separates the screen shield into two main screens. One is the lock screen, and it is shown when coming back from idle status and when failing authentication. The other is the actual unlock dialog. Moving from the first to the second is possible by pressing Escape or by dragging an arrow on the bottom on the screen. Using ClutterDragAction now.
Created attachment 218642 [details] [review] ScreenShield: show notifications in the locked screen Rework lock handling in the message tray to separate summary handling and notification handling. Now the message tray is completely hidden when the screen is locked, but exceptions can be made for individual transient notifications. Non transient sources are shown in the middle of the lock screen. Resident notifications (such as those from Rhythmbox) are shown in full, while dpersistent ones are displayed as icon and message count.
Review of attachment 218641 [details] [review]: ::: js/ui/screenShield.js @@ +42,3 @@ + reactive: true, + can_focus: true, + layout_manager: new Clutter.BinLayout Missing parens. @@ +98,3 @@ + _onLockScreenKeyRelease: function(actor, event) { + if (event.get_key_symbol() == Clutter.KEY_Escape) { + if (this._arrowGrabbed) { This doesn't exist anymore. @@ +122,3 @@ + // to handle borders too + // or maybe even better, make it into a + // shell_util_render_arrow(StWidget, cairo_t) (There actually was a shell_util_render_arrow at some point) It doesn't seem that awful to have BoxPointer.drawArrow();, which takes a cr. @@ +145,3 @@ + transition: 'linear', + onComplete: function() { + this.fixed_position_set = false; ? @@ +259,3 @@ + _resetLockScreen: function() { + this._lockScreenGroup.fixed_position_set = false; this._lockScreenGroup.set_position(0, 0);
Review of attachment 218638 [details] [review]: Sure.
Review of attachment 218642 [details] [review]: This patch needs to be rebased better. ::: js/ui/messageTray.js @@ +818,3 @@ }, + setShowWhenLocked: function(show) { Why does this name change happen here, instead when it's originally introduced?
Review of attachment 218637 [details] [review]: log statements are still around. Kill those. Also, please find a better strategy for this. I still don't like the modal tracking.
Review of attachment 218636 [details] [review]: ::: js/ui/unlockDialog.js @@ +20,3 @@ + +const Fprint = imports.gdm.fingerprint; +const GdmUtil = imports.gdm.util; We do not import imports.gdm from imports.ui. ::: js/ui/userMenu.js @@ +42,3 @@ // Copyright (C) 2008,2009 Red Hat, Inc. +const UserAvatarWidget = new Lang.Class({ Can you put this into a separate patch on its own? @@ +63,3 @@ + if (iconFile) { + let file = Gio.File.new_for_path(iconFile); + this.actor.gicon = new Gio.FileIcon({ file: file }); background-image is used so that CSS styling works. Are you sure this will work? @@ -240,2 @@ - if (iconFile) - this._setIconFromFile(iconFile); You should remove _setIconFromFile/_setIconFromName.
(Note that I don't think the bug is getting too crowded or too long, but if you think so, you can always clone the bug and start over again with a new set of patches.)
(In reply to comment #69) > Review of attachment 218641 [details] [review]: > @@ +145,3 @@ > + transition: 'linear', > + onComplete: function() { > + this.fixed_position_set = false; > > ? > > @@ +259,3 @@ > > + _resetLockScreen: function() { > + this._lockScreenGroup.fixed_position_set = false; > > this._lockScreenGroup.set_position(0, 0); fixed-position-set is set to false so that the LayoutManager on the screenShieldGroup kicks in again, otherwise it is ignored. (In reply to comment #72) > Review of attachment 218642 [details] [review]: > > This patch needs to be rebased better. > > ::: js/ui/messageTray.js > @@ +818,3 @@ > }, > > + setShowWhenLocked: function(show) { > > Why does this name change happen here, instead when it's originally introduced? Because it implies a change in semantics too: originally, privacySensitive was meant to be a setting on each notification or source, configurable using an appropriate panel. Now the visibility of notifications is controlled by a global setting, and showWhenLocked only controls if the notification is shown as a banner (rather than cumulating in the center of the lock screen). I may just squash the two patches though. (In reply to comment #74) > Review of attachment 218636 [details] [review]: > > ::: js/ui/unlockDialog.js > @@ +20,3 @@ > + > +const Fprint = imports.gdm.fingerprint; > +const GdmUtil = imports.gdm.util; > > We do not import imports.gdm from imports.ui. So how do we share code between gdm and ui? misc.gdmUtils? (I'm ok on misc.fingerprint, fwiw) > @@ +63,3 @@ > + if (iconFile) { > + let file = Gio.File.new_for_path(iconFile); > + this.actor.gicon = new Gio.FileIcon({ file: file }); > > background-image is used so that CSS styling works. Are you sure this will > work? Seems to work here (it's a StIcon, after all).
(In reply to comment #56) > (In reply to comment #53) > > Why no? This is identical to the login screen behaviour. > > Sorry, I didn't realize we had a lot of code that was already broken. I don't > see how using notifyError to expose a programmer error to the user is helpful > in any way (what are they going to do about it?), but sure. > > If only GNOME had an automatic bug reporting service, integrated into the > system itself... ( and then I can switch gnome-shell-extensions to use it :) ) Just to be clear, that particular notifyError is not used for programmer errors: Problem is just a bad name for a PAM warning or error, such as authentication failed or password change required.
(In reply to comment #69) > Review of attachment 218641 [details] [review]: > > @@ +122,3 @@ > + // to handle borders too > + // or maybe even better, make it into a > + // shell_util_render_arrow(StWidget, cairo_t) > > (There actually was a shell_util_render_arrow at some point) > > It doesn't seem that awful to have BoxPointer.drawArrow();, which takes a cr. It does, once you read boxpointer.js... Better to keep this code simple for now.
(In reply to comment #73) > Review of attachment 218637 [details] [review]: > > log statements are still around. Kill those. I killed one but left the other, as it is useful to have in logs a reason for the screensaver not working > Also, please find a better strategy for this. I still don't like the modal > tracking. Sorry, I don't have a better idea, other that some complex state machine that would be even less readable. gnome-session can go IDLE again after the lock screen is shown, there is nothing we can do about it, and we can't pushModal again in that case.
Created attachment 218816 [details] [review] UserMenu: split user avatar into its own widget This way, it can be reused in the lock screen without code duplication.
Created attachment 218817 [details] [review] Add UnlockDialog for unlocking the screen shield When the screenshield is deactivated, instead of going back to the session immediately, prompt the user for authentication. This essentially reinstates what used to be provided by gnome-screensaver. It is implemented by factoring out common code from gdm/loginDialog.js and ui/userMenu.js
Created attachment 218818 [details] [review] ScreenShield: improve locking/modal policy Ensure that the lightbox is above everything (including the screenlock itself) when fading in - this allows for fading while showing the unlock dialog. Also, don't pushModal again when already locked, or we won't get out of it.
Created attachment 218819 [details] [review] Don't track the status of the screensaver on DBus We are the screensaver now, and internal objects can track the locking status better themselves. And to do so, add two signals to ScreenShield.
Created attachment 218820 [details] [review] ScreenShield: implement curtain design This separates the screen shield into two main screens. One is the lock screen, and it is shown when coming back from idle status and when failing authentication. The other is the actual unlock dialog. Moving from the first to the second is possible by pressing Escape or by dragging an arrow on the bottom on the screen.
Created attachment 218821 [details] [review] Show the panel above the screenshield when locked Track locked status and use it to provide a reduced version of the panel in the locked screen. Accessibility, input sources and volume menus are preserved, without the link to the control center. Network, battery and user menu are reduced to pure indicators, with no menu. This is similar to the design but not exactly, because designers in IRC said that network needs more analysis before exposing, and because the design didn't account for a11y and IM (so the one menu metaphor is not really appropriate).
Created attachment 218822 [details] [review] ScreenShield: show notifications in the locked screen Track screen lock status in the message tray, and filter banner notifications. The message tray is completely hidden when the screen is locked, but exceptions can be made for individual transient notifications, such as shell messages and the on screen keyboard. Non transient sources are shown in the middle of the lock screen. Resident notifications (such as those from Rhythmbox) are shown in full, while persistent ones are displayed as icon and message count.
Review of attachment 218821 [details] [review]: ::: js/ui/panel.js @@ +762,3 @@ + for (index = children.length - 1; index >= 0; index--) { + if (children[index].visible) + break; Is this a bug fix for something? Make it a separate patch. @@ +787,3 @@ + for (index = 0; index < children.length; index++) { + if (children[index].visible) + break; Is this a bug fix for something? Make it a separate patch. @@ +1178,3 @@ + this._activities.visible = !locked; + if (this._appMenu) + this._appMenu._sync(); Add a setLockedState to the app menu ::: js/ui/popupMenu.js @@ +914,3 @@ + for (let id in this._settingsActions) { + let item = this._settingsActions[id]; + No whitespace. ::: js/ui/status/accessibility.js @@ +86,3 @@ + setLockedState: function(locked) { + this.menu.setSettingsVisibility(!locked); + }, Can you provide this as a default implementation in PopupMenu? ::: js/ui/status/power.js @@ +85,3 @@ + this.menu.close(); + this.actor.reactive = !locked; + this.actor.can_focus = !locked; Hide the entire actor, don't do this.
Review of attachment 218822 [details] [review]: Not the biggest fan of this. I think it would be nice to split out the message tray model (where there are the active sources and things), and the current message tray view (the actor and behavior involved with that), so that we don't fake a message tray with resident sources and things. I think it would make the code a lot cleaner, but feel free to prove me wrong :)
Review of attachment 218820 [details] [review]: Mostly fine. ::: js/ui/screenShield.js @@ +57,3 @@ + y_align: Clutter.ActorAlign.END, + // HACK: without these, ClutterBinLayout + // ignores alignment properties on the actor Yeah, that's a ClutterBinLayout bug. I have a patch for it, not sure if it's filed anywhere. You only need one of the expands, though, but nothing wrong with both. @@ +162,2 @@ let shouldLock = lightboxWasShown && this._settings.get_boolean(LOCK_ENABLED_KEY); + if (this._isLocked || shouldLock) { ??? @@ -131,3 @@ }, - get locked() { Why are these moved around? Put them where they're supposed to be originally.
Review of attachment 218819 [details] [review]: ::: js/ui/layout.js @@ -700,3 @@ continue; - if (this._screenSaverActive) Any reason you're outright removing it, instead of just changing visibility? ::: js/ui/screenShield.js @@ +131,3 @@ }, + + get locked() { Can you put these additions and signals in the initial patch? @@ +137,3 @@ + lock: function() { + if (!this._isModal) { + Main.pushModal(this.actor); A commit like this (stop tracking objects on DBus) should not introduce changes like "now push the modal". ::: js/ui/userMenu.js @@ +789,3 @@ this._session.ShutdownRemote(); } else { + Main.screenShield.lock(); Don't we have still have to wait for the fade out tween to finish?
Review of attachment 218818 [details] [review]: I don't quite understand the this._isLocked thing. I'm not overly happy with this, but it works. ::: js/ui/screenShield.js @@ +62,2 @@ if (status == GnomeSession.PresenceStatus.IDLE) { + if (this._dialog) { This should be done when you first introduce the dialog.
Review of attachment 218817 [details] [review]: GdmUtil should be a separate patch. Your commit message mentions userMenu.js, but this isn't touched at all. A lot of the specific review comments below are just thoughts I had while reviewing the code. Most of them aren't your fault. I'd be fine landing this if none of the "not your faults" were addressed. ::: js/gdm/loginDialog.js @@ +1260,2 @@ _fadeOutNotListedButton: function() { + return GdmUtil.fadeOutActor(this._notListedButton); This code is all a bit silly. Not that it's your fault, of course. ::: js/gdm/util.js @@ +12,3 @@ +const BANNER_MESSAGE_TEXT_KEY = 'banner-message-text'; + +const LOGO_KEY = 'logo'; Not touched at all. @@ +19,3 @@ + + actor.show(); + let [minHeight, naturalHeight] = actor.get_preferred_height(-1); I know this isn't your code, but it's still dirty and awful, and will break for multiline labels, which there are plenty of in gdm :) @@ +27,3 @@ + height: naturalHeight, + time: FADE_ANIMATION_TIME, + transition: 'easeOutQuad', I've been wondering about about something like: Tweener.defaultParams.transition = 'easeOutQuad'; @@ +37,3 @@ + if (!actor.visible || actor.opacity == 0) { + actor.opacity = 0; + actor.hide(); There's a missing return; here. ::: js/ui/unlockDialog.js @@ +38,3 @@ + this._label = new St.Label({ style_class: 'unlock-dialog-user-name' }); + this.actor.add(this._label, + { expand: true, Awkwardly indented. @@ +87,3 @@ + this._user = this._userManager.get_user(this._userName); + + this._greeterClient = GdmGreeter.Server.new_for_display_sync(null, null); (This will need to be rebased when the rename is done) @@ +109,3 @@ + + // Note: this represents user settings for the login screen, not gdm ones, + // and so it's only useful with dconf lockdown I don't understand the comment. @@ +172,3 @@ + _updateOkButton: function(sensitive) { + this._okButton.button.reactive = sensitive; + this._okButton.button.can_focus = sensitive; Hm. We probably should adjust navigate_focus to skip unreactive actors. @@ +176,3 @@ + this._okButton.button.remove_style_pseudo_class('disabled'); + else + this._okButton.button.add_style_pseudo_class('disabled'); We should probably track this automatically in StWidget, but no big deal. @@ +185,3 @@ + + _startFingerprintConversationIfNeeded: function() { + this._haveFingerprintReader = false; I wonder if there's more code here that can be shared between gdm and this. Hopefully everything that potentially needs a login won't need to have fprintd conversation code. It's not that much, but still.
Review of attachment 218816 [details] [review]: endSessionDialog.js has a copy of the widget, too, that you should probably replace. If this works, sure. It would also fix the issue with the texture cache, I believe. Two birds with one stone :)
Review of attachment 218282 [details] [review]: Sure.
Review of attachment 218297 [details] [review]: I like it. ::: js/ui/unlockDialog.js @@ +22,3 @@ +function isSupported() { + // Old GDM has GdmGreeter.Client, new has GdmGreeter.Server; + return 'Server' in GdmGreeter; Given that we're going to be renaming "GdmGreeter" to just "Gdm", you could switch based on that: let _newGdm; try { const Gdm = imports.gi.Gdm; _newGdm = true; } except(e) { const Gdm = imports.gi.GdmGreeter; _newGdm = false; }
Review of attachment 218281 [details] [review]: I think this has been obsoleted.
Review of attachment 218288 [details] [review]: Sure, why not. ::: js/gdm/loginDialog.js @@ +1065,3 @@ function() { this.setButtons(buttons); + buttons[1].button.add_style_pseudo_class('default'); This should probably be in modalDialog. ::: js/ui/screenShield.js @@ +597,3 @@ time: SHORT_FADE_TIME, transition: 'easeOutQuad' + }); ?? ::: js/ui/unlockDialog.js @@ +75,3 @@ y_fill: true }); + this._label = new St.Label({ style_class: 'login-dialog-username' }); Not sure it's right to re-use the 'login-dialog' classes.
Review of attachment 218450 [details] [review]: Mostly clean ::: js/gdm/loginDialog.js @@ +143,3 @@ } +const PinPadWidget = new Lang.Class({ js/ui/pinPad.js? @@ +151,3 @@ + this.actor = new St.Table({ style_class: 'pin-pad' }); + + this.actor.add(this._makeButton(0), { col: 1, row: 3 }); do we need # and * ? :) @@ +155,3 @@ + // XXX: This arrangement matches that of a phone, not that + // of a PC numeric keypad + // I'm not sure which one is better designers? ::: js/ui/unlockDialog.js @@ +329,3 @@ + // Special untranslated marker from pam_pin.so + if (secretQuestion == 'PIN') { + _fadeInActor(this._promptPinPad.actor); see what I said about sharing code?
Review of attachment 218291 [details] [review]: Mostly fine. ::: js/ui/screenShield.js @@ +460,3 @@ + let [success, pid] = GLib.spawn_async(null, argv, null, + GLib.SpawnFlags.DO_NOT_REAP_CHILD, + null, null); Sounds like a case for GSubprocess :) ::: src/shell-screenshield-watchdog.c @@ +77,3 @@ + name, error->message); + g_error_free (error); + return TRUE; That is the wrong return value. @@ +80,3 @@ + } + + g_variant_get (res, "(u)", &pid); You need to free 'res' @@ +167,3 @@ +select_popup_events (void) +{ + XWindowAttributes attr; indent @@ +175,3 @@ + XGetWindowAttributes (GDK_DISPLAY_XDISPLAY (gdk_display_get_default ()), GDK_ROOT_WINDOW (), &attr); + + events = SubstructureNotifyMask | attr.your_event_mask; Why Substructure? You don't care about CreateNotify. @@ +195,3 @@ + gdk_window_raise (gdk_window); + default: + ; It doesn't do anything, but let's prevent fallthrough and insert explicit breaks here and above. @@ +216,3 @@ + gtk_widget_override_background_color (window, + GTK_STATE_FLAG_NORMAL, + &black); This should really done by a patch to gnome-themes-standard.
Review of attachment 218292 [details] [review]: There was a patch before where you removed this for some reason. This should be squashed with that.
Review of attachment 218294 [details] [review]: We need to own the org.gnome.ScreenSaver bus name, right? ::: js/ui/main.js @@ +225,3 @@ statusIconDispatcher = new StatusIconDispatcher.StatusIconDispatcher(); screenShield = new ScreenShield.ScreenShield(); + screenSaverDBus = new ShellDBus.ScreenSaverDBus(); For the extensions stuff, I stuck an "extensionImpl" in the main DBus interface. We should do that here.
Review of attachment 218293 [details] [review]: Called it. Squash.
Review of attachment 218271 [details] [review]: Yeah, not a fan of this. That said, I'm less of a fan of the current, completely broken code. We should probably complain loudly when this happens.
Comment on attachment 218297 [details] [review] Allow the shell to run without the screenshield Not needed (see bug 677118)
(In reply to comment #96) > Review of attachment 218281 [details] [review]: > > I think this has been obsoleted. Not really (unless the design changed...). The point of this patch is to place both the login dialog and the unlock dialog under the curtain. (In reply to comment #87) > Review of attachment 218821 [details] [review]: > > ::: js/ui/status/accessibility.js > @@ +86,3 @@ > + setLockedState: function(locked) { > + this.menu.setSettingsVisibility(!locked); > + }, > > Can you provide this as a default implementation in PopupMenu? You mean in PanelMenu.Button? There is already a default implementation there, which does something more sensible IMHO (it hides the actor completely). > ::: js/ui/status/power.js > @@ +85,3 @@ > + this.menu.close(); > + this.actor.reactive = !locked; > + this.actor.can_focus = !locked; > > Hide the entire actor, don't do this. No, we need to show the non reactive battery and network icon. (In reply to comment #88) > Review of attachment 218822 [details] [review]: > > Not the biggest fan of this. I think it would be nice to split out the message > tray model (where there are the active sources and things), and the current > message tray view (the actor and behavior involved with that), so that we don't > fake a message tray with resident sources and things. > > I think it would make the code a lot cleaner, but feel free to prove me wrong > :) You're right, indeed it would, but I didn't want to make more invasive changes to the message tray code (which I'm not very familiar with). (In reply to comment #89) > Review of attachment 218820 [details] [review]: > > @@ +162,2 @@ > let shouldLock = lightboxWasShown && > this._settings.get_boolean(LOCK_ENABLED_KEY); > + if (this._isLocked || shouldLock) { > shouldLock means that we finished the fading out (lightboxWasShown) and lock is enabled in settings. If we go back to idle and out again but the lock is disabled, shouldLock would be false, even if the screenshield was activated manually from the user menu. (In reply to comment #97) > Review of attachment 218288 [details] [review]: > > Sure, why not. > > ::: js/gdm/loginDialog.js > @@ +1065,3 @@ > function() { > this.setButtons(buttons); > + buttons[1].button.add_style_pseudo_class('default'); > > This should probably be in modalDialog. Really? Login and unlock are the only dialogs to have special styling for the default button. > ::: js/ui/unlockDialog.js > @@ +75,3 @@ > y_fill: true }); > > + this._label = new St.Label({ style_class: 'login-dialog-username' }); > > Not sure it's right to re-use the 'login-dialog' classes. Well, this patch is meant to unify styling between the two, so it makes sense to use the same classes. (In reply to comment #98) > Review of attachment 218450 [details] [review]: > > Mostly clean > > ::: js/gdm/loginDialog.js > @@ +143,3 @@ > } > > +const PinPadWidget = new Lang.Class({ > > js/ui/pinPad.js? js/gdm/util.js for now (rebased but didn't upload)
(In reply to comment #99) > Review of attachment 218291 [details] [review]: > > Mostly fine. > > ::: js/ui/screenShield.js > @@ +460,3 @@ > + let [success, pid] = GLib.spawn_async(null, argv, null, > + > GLib.SpawnFlags.DO_NOT_REAP_CHILD, > + null, null); > > Sounds like a case for GSubprocess :) > > ::: src/shell-screenshield-watchdog.c > @@ +77,3 @@ > + name, error->message); > + g_error_free (error); > + return TRUE; > > That is the wrong return value. Yes and no. If check_is_parent_shell() returns TRUE, the watchdog stays alive and ignores the dbus name, otherwise it terminates. In case obtaining the PID failed (which means it crashed again before we could even check), we want to continue showing the watchdog window. > @@ +80,3 @@ > + } > + > + g_variant_get (res, "(u)", &pid); > > You need to free 'res' > > @@ +167,3 @@ > +select_popup_events (void) > +{ > + XWindowAttributes attr; > > indent > > @@ +175,3 @@ > + XGetWindowAttributes (GDK_DISPLAY_XDISPLAY (gdk_display_get_default > ()), GDK_ROOT_WINDOW (), &attr); > + > + events = SubstructureNotifyMask | attr.your_event_mask; > > Why Substructure? You don't care about CreateNotify. I care about MapNotify and CreateNotify for children of the root window. (X11 code is copied from gnome-screensaver, btw)
Created attachment 219047 [details] [review] Panel: fix finding leftmost and rightmost buttons Previous code would access the array element before checking that the index was within bounds, and therefore cause a TypeError. It wasn't noticed earlier because at least one visible children is in each panel box in all session modes.
Created attachment 219048 [details] [review] Split some common code out of gdm/loginDialog This will be reused by session unlocking.
Created attachment 219049 [details] [review] Add UnlockDialog for unlocking the screen shield When the screenshield is deactivated, instead of going back to the session immediately, prompt the user for authentication. This essentially reinstates what used to be provided by gnome-screensaver.
Created attachment 219050 [details] [review] ScreenShield: improve locking/modal policy Ensure that the lightbox is above everything (including the screenlock itself) when fading in - this allows for fading while showing the unlock dialog. Also, don't pushModal again when already locked, or we won't get out of it.
Created attachment 219051 [details] [review] Consolidate creation of login and unlock dialog in the screenshield The design calls for the curtain to appear in the gdm greeter too. Implement this by having the screenshield manage the login dialog (delegating its creation to SessionMode).
Created attachment 219052 [details] [review] Add a clock to the lock screen Start implementing the lock screen design by adding a big white clock in the middle.
Created attachment 219053 [details] [review] Show the panel above the screenshield when locked Track locked status and use it to provide a reduced version of the panel in the locked screen. Accessibility, input sources and volume menus are preserved, without the link to the control center. Network, battery and user menu are reduced to pure indicators, with no menu. This is similar to the design but not exactly, because designers in IRC said that network needs more analysis before exposing, and because the design didn't account for a11y and IM (so the one menu metaphor is not really appropriate).
Created attachment 219054 [details] [review] Don't track the status of the screensaver on DBus We are the screensaver now, and internal objects can track the locking status better themselves. And to do so, add two signals to ScreenShield.
Created attachment 219055 [details] [review] Screen Shield: animate manual locking When the screen shield is activated from the user menu, animate it instead of showing it abruptly. Also, ensure that the animation had time to finish before calling UPower to suspend, to avoid showing it when resuming.
Created attachment 219056 [details] [review] ScreenShield: add a watchdog process in case the shell crashes If the shell crashes while the screenshield is active, the lock screen is teared down, and that's potentially a security issue. To mitigate that, add a process that keeps a full black screen on top of everything (except the shell itself) and grabs the mouse and keyboard if the shell crashes.
Created attachment 219057 [details] [review] ShellDBus: export screensaver interface gnome-session and gnome-settings-daemon rely on the screensaver interface to know the locked state. Since gnome-screensaver is no longer running, it's up to gnome-shell to provide it.
Created attachment 219058 [details] [review] UnlockDialog: show a numeric keypad when using PIN authentication When using pam_pin.so to login with a numeric PIN, show a numeric keypad to make it easier to type, especially on touch devices.
Review of attachment 219047 [details] [review]: Maybe this should use get_first_child / get_last_child and get_next_sibling / get_prev_sibling ? Fine otherwise.
Review of attachment 219048 [details] [review]: Sure.
(This is an old comment I started writing before you uploaded the new stack) (In reply to comment #106) > I care about MapNotify and CreateNotify for children of the root window. > (X11 code is copied from gnome-screensaver, btw) You can just use StructureNotifyMask in that case. All SubstructureNotifyMask adds is the CreateNotify event, and you don't do anything in the CreateNotify case. (In reply to comment #105) > (In reply to comment #96) > > Review of attachment 218281 [details] [review] [details]: > > > > I think this has been obsoleted. > > Not really (unless the design changed...). The point of this patch is to place > both the login dialog and the unlock dialog under the curtain. It seemed like all of its changes were parts of other, later patches. > (In reply to comment #88) > You're right, indeed it would, but I didn't want to make more invasive changes > to the message tray code (which I'm not very familiar with). I sort of want to say "yeah, that makes sense, we'll clean it up later", but I know that that won't ever happen. We should clean it up now. There's already a lot of good stuff around making the message tray model and message tray views separate and reusable -- createNotificationIcon and all. I don't care about extreme cleanliness, but hijacking the message tray like this is not OK. > (In reply to comment #97) > Really? Login and unlock are the only dialogs to have special styling for the > default button. Is that intentional? I think highlighting the default button on all dialogs should be done. > (In reply to comment #98) > js/gdm/util.js for now (rebased but didn't upload) Why not?
Review of attachment 219049 [details] [review]: Cool.
Review of attachment 219050 [details] [review]: Sure.
Review of attachment 219052 [details] [review]: It seems like this sort of formatting should be done as part of GnomeDesktop.WallClock: an IGNORE_DATE property or something.
Review of attachment 219051 [details] [review]: Interesting. Any reason you just can't split out the curtain behaviour and use it in both places? I doubt we need everything the screen shield does. ::: js/ui/main.js @@ +97,3 @@ function createGDMSession() { + GLib.idle_add(GLib.PRIORITY_DEFAULT, function() { + screenShield.showDialog(); ??? ::: js/ui/screenShield.js @@ +220,1 @@ + _showUnlockDialog: function(animate) { Seems like this should be a separate patch?
Review of attachment 219053 [details] [review]: ::: js/ui/panel.js @@ +1178,3 @@ + _onLockStateChanged: function(shield, locked) { + if (this._activities) + this._activities.visible = !locked; Again, this should be a setLockedState here. no reason to break encapsulation here. ::: js/ui/panelMenu.js @@ +147,3 @@ + setLockedState: function(locked) { + // default behaviour is to hide completely bluetooth is the only button that has the default implementation (well, besides extensions).
Review of attachment 219054 [details] [review]: There was patch that provided a dummy fallback screen shield. Squash with this one, please.
Review of attachment 219055 [details] [review]: Looks fine.
Review of attachment 219058 [details] [review]: Cool. ::: js/gdm/util.js @@ +67,3 @@ + // XXX: This arrangement matches that of a phone, not that + // of a PC numeric keypad + // I'm not sure which one is better https://www.google.com/search?q=pin+pad&tbm=isch shows that most popular PIN pads have the phone layout.
Review of attachment 219057 [details] [review]: Fine.
Comment on attachment 219057 [details] [review] ShellDBus: export screensaver interface gnome-session started reacting to this, forcing session on IDLE status when the screen is locked, and thus causing the lightbox to be present at all times.
Review of attachment 219056 [details] [review]: Sure. ::: js/ui/screenShield.js @@ +464,3 @@ + try { + Shell.util_kill(this._watchdogPid); + } catch(e) { } I don't see the point in reporting an error if we just ignore it.
(In reply to comment #131) > (From update of attachment 219057 [details] [review]) > gnome-session started reacting to this, forcing session on IDLE status when the > screen is locked, and thus causing the lightbox to be present at all times. Ok found a workaround that could be even considered correct: gnome-shell emits ActiveChanged for lock-status-changed, and gnome-session reacts to that, but gnome-shell does not show the lightbox when going to IDLE status and already locked. This means that session presence is forced to IDLE when the screen is locked, which is correct. OTOH it means that leaving the PC at the lock screen won't fade the screen to black after the configured idle time.
(In reply to comment #127) > Review of attachment 219054 [details] [review]: > > There was patch that provided a dummy fallback screen shield. Squash with this > one, please. There is one problem with that: without libgdm, gnome-shell won't even start (missing import in userMenu.js). With libgdm, we don't know if the running gdm-binary is old or new until we try.
(In reply to comment #126) > Review of attachment 219053 [details] [review]: > > ::: js/ui/panelMenu.js > @@ +147,3 @@ > > + setLockedState: function(locked) { > + // default behaviour is to hide completely > > bluetooth is the only button that has the default implementation (well, besides > extensions). Exactly: there are so many extensions in the top panel that the default must be as conservative as possible.
(In reply to comment #125) > Review of attachment 219051 [details] [review]: > > Interesting. Any reason you just can't split out the curtain behaviour and use > it in both places? I doubt we need everything the screen shield does. Gdm uses all of the ScreenShield but the notification handling. Not worth to split, IMHO. > ::: js/ui/screenShield.js > @@ +220,1 @@ > + _showUnlockDialog: function(animate) { > > Seems like this should be a separate patch? Not really. The animate parameter is only needed by the public showDialog method, and the rest is just moving around to cater for the SessionMode.createDialog() change.
Created attachment 219067 [details] [review] Allow the shell to run without the screenshield The screenshield requires gdm 3.5, which can be problematic in jhbuild configurations. Allow transparent fallback to gnome-screensaver in that case.
Created attachment 219068 [details] [review] Show the panel above the screenshield when locked Track locked status and use it to provide a reduced version of the panel in the locked screen. Accessibility, input sources and volume menus are preserved, without the link to the control center. Network, battery and user menu are reduced to pure indicators, with no menu. This is similar to the design but not exactly, because designers in IRC said that network needs more analysis before exposing, and because the design didn't account for a11y and IM (so the one menu metaphor is not really appropriate).
Created attachment 219069 [details] [review] Consolidate creation of login and unlock dialog in the screenshield The design calls for the curtain to appear in the gdm greeter too. Implement this by having the screenshield manage the login dialog (delegating its creation to SessionMode).
Created attachment 219071 [details] [review] Show the panel above the screenshield when locked Track locked status and use it to provide a reduced version of the panel in the locked screen. Accessibility, input sources and volume menus are preserved, without the link to the control center. Network, battery and user menu are reduced to pure indicators, with no menu. This is similar to the design but not exactly, because designers in IRC said that network needs more analysis before exposing, and because the design didn't account for a11y and IM (so the one menu metaphor is not really appropriate).
Review of attachment 219048 [details] [review]: ::: js/gdm/util.js @@ +37,3 @@ + if (!actor.visible || actor.opacity == 0) { + actor.opacity = 0; + actor.hide(); There's still a missing return; here.
Review of attachment 219071 [details] [review]: One very minor nit. I'd still like to adjust navigate_focus so that it doesn't try to focus unreactive actors. ::: js/ui/popupMenu.js @@ +905,3 @@ + + if (!this._settingsActions) + this._settingsActions = { }; Shouldn't this go in the constructor?
Review of attachment 219057 [details] [review]: Marked needs-work by accident.
Review of attachment 219056 [details] [review]: ::: js/ui/screenShield.js @@ +464,3 @@ + try { + Shell.util_kill(this._watchdogPid); + } catch(e) { } Also, bare catches are fairly evil since they hide *every* error. ::: src/shell-screenshield-watchdog.c @@ +43,3 @@ +on_sigterm (gpointer user_data) +{ + g_debug ("Got SIGTERM, quitting cleanly"); What's "cleanly"? What are you doing here that wouldn't be done by X on process exit?
Review of attachment 219049 [details] [review]: Now that I have a much better understanding of the PAM/GDM stack due to me working with it for a while, I can actually review this patch in a non-superficial manner! (How did you learn this stuff so fast? I'm jealous; I had to ask Ray like 50 questions.) ::: js/ui/screenShield.js @@ -91,3 @@ - _onButtonPressEvent: function(object, buttonPressEvent) { - log("in _onButtonPressEvent - lock is enabled: " + this._settings.get_boolean(LOCK_ENABLED_KEY)); still have these pesky logs around ::: js/ui/unlockDialog.js @@ +38,3 @@ + this._label = new St.Label({ style_class: 'unlock-dialog-user-name' }); + this.actor.add(this._label, + { expand: true, Awkward indenting still persists. @@ +121,3 @@ + this._promptLabel = new St.Label({ style_class: 'login-dialog-prompt-label' }); + this._promptLayout.add(this._promptLabel, + { expand: false, Bad indentation. @@ +124,3 @@ + x_fill: true, + y_fill: true, + x_align: St.Align.START }); There are a number of different styles in play here: A. const foo = func(bar, { a: 1, b: 2 }); B. const foo = func(bar, { a: 1, b: 2 }); C. const foo = func(bar, { a: 1, b: 2 }); D. const foo = func(bar, { a: 1, b: 2 }); We tend to use styles A and C in the shell code. I really don't care which one you use, as long as you're consistent. This patch is not. @@ +133,3 @@ + + this._promptLayout.add(this._promptEntry, + { expand: true, Bad indentation. @@ +156,3 @@ + x_fill: true }); + this._otherUserButton.connect('clicked', Lang.bind(this, this._otherUserClicked)); + this.contentLayout.add(this._otherUserButton, { x_align: St.Align.START, x_fill: false }); And one more... @@ +171,3 @@ + _updateOkButton: function(sensitive) { + this._okButton.button.reactive = sensitive; + this._okButton.button.can_focus = sensitive; By the way, did you ever investigate modifying navigate_focus so that we can't focus an unreactive actor @@ +175,3 @@ + this._okButton.button.remove_style_pseudo_class('disabled'); + else + this._okButton.button.add_style_pseudo_class('disabled'); ... or adding an automatic 'disabled' class to St based on reactivity? @@ +206,3 @@ + // as a cue to display our own message. + if (serviceName == GdmUtil.FINGERPRINT_SERVICE_NAME && + this._haveFingerprintReader) { Is there ever a case where we get the serviceName of FINGERPRINT_SERVICE_NAME and we don't have a service? @@ +213,3 @@ + if (serviceName != GdmUtil.PASSWORD_SERVICE_NAME) + return; + Main.notify(info); Seems strange to me that we notify in the message tray for all other info prompts, but show a label for fingerprints. also: { if (a) return; if (b) return; doAThing(); } seems awkward to me @@ +220,3 @@ + // users who haven't enrolled their fingerprint. + if (serviceName != GdmUtil.PASSWORD_SERVICE_NAME) + return; wait what? The comment doesn't match for me. @@ +221,3 @@ + if (serviceName != GdmUtil.PASSWORD_SERVICE_NAME) + return; + Main.notifyError(problem); Se above. @@ +230,3 @@ + + this._promptLabel.text = question; + this._promptEntry.text = ''; Whitespace above this line please. this._promptLabel.text = 'foo'; this._promptEntry.text = ''; on cursory inspection, it looks like it's an overwrite. @@ +240,3 @@ + _onSecretInfoQuery: function(client, serviceName, secretQuestion) { + // We only expect secret requests to come from the main auth service + if (serviceName != GdmUtil.PASSWORD_SERVICE_NAME) What about kerberos? @@ +244,3 @@ + + this._promptLabel.text = secretQuestion; + this._promptEntry.text = ''; See above.
Review of attachment 219069 [details] [review]: Not the biggest fan of the session mode stuff. Can't you just pass it into a parameter? ::: js/ui/main.js @@ +107,1 @@ loginDialog.connect('loaded', function() { Why does the loginDialog need to load, and the unlockDialog need not? @@ +113,3 @@ +function createSessionUnlockDialog(parentActor) { + let dialog = new UnlockDialog.UnlockDialog(parentActor); + if (!dialog.open()) { Woah now. create != open ::: js/ui/screenShield.js @@ +237,3 @@ + + if (!this._dialog) { + [this._dialog, this._keepDialog] = Main.sessionMode.createUnlockDialog(this._lockDialogGroup); This keepDialog stuff is iffy to me. I'd rather have the thing that sets up the screen shield in both cases give it a parameter on construction.
Review of attachment 219067 [details] [review]: Yeah, I'm not sure it's worth it at this point either.
Review of attachment 218816 [details] [review]: ::: js/ui/userMenu.js @@ +53,3 @@ + icon_type: St.IconType.SYMBOLIC, + icon_size: DIALOG_ICON_SIZE + }); if you use St.Icon instead of what was there before then the avatar won't sit behind (and get clipped by) its rounded frame anymore right?
(In reply to comment #149) > Review of attachment 218816 [details] [review]: > > ::: js/ui/userMenu.js > @@ +53,3 @@ > + icon_type: St.IconType.SYMBOLIC, > + icon_size: DIALOG_ICON_SIZE > + }); > > if you use St.Icon instead of what was there before then the avatar won't sit > behind (and get clipped by) its rounded frame anymore right? I said that, and he said it worked. I trust him.
Review of attachment 219056 [details] [review]: ::: js/ui/screenShield.js @@ +412,3 @@ + GLib.SpawnFlags.DO_NOT_REAP_CHILD, + null, null); + GLib.child_watch_add(GLib.PRIORITY_DEFAULT, pid, function () { }, null); should probably clear _watchdogPid here instead of when you kill it explicitly, so that state is right even if an eager sysadmin kills it. ::: src/shell-screenshield-watchdog.c @@ +194,3 @@ + case ConfigureNotify: + if (!(xevent->xany.window == gdk_x11_window_get_xid (gdk_window))) + gdk_window_raise (gdk_window); So I know gnome-screensaver does this, but another idea might to use the MIT-Screensaver window. It should be always on top. i did a little example program a few years ago here: http://cgit.freedesktop.org/~halfline/test-screensaver-extension/tree/test-screensaver-extension.c or you could potentially leverage the COW @@ +241,3 @@ + 0, 0, + gdk_screen_get_width (gdk_screen), + gdk_screen_get_height (gdk_screen)); I guess we don't support zaphod mode multi-screen setups right now at all with the shell, but it might make sense to quickly cover it here to futureproof the code. @@ +251,3 @@ + on_name_appeared, + on_name_vanished, + NULL, NULL); This is probably fine, but i wonder if DBus's little used queueing feature would help here (i.e. take the org.gnome.shell name but using the queue flag to make it so you don't get the name until the instant it becomes available.)
Review of attachment 219048 [details] [review]: If you drop the Batch.Hold stuff then animations that aren't supposed to start until other animations finish, will start prematurely and muck up the choreography.
Review of attachment 219049 [details] [review]: ::: js/ui/unlockDialog.js @@ +220,3 @@ + // users who haven't enrolled their fingerprint. + if (serviceName != GdmUtil.PASSWORD_SERVICE_NAME) + return; This comment is just saying "ignore messages from the fingerprint pam module because it tells us stuff we don't care about (like authentication failed when the users print isn't enrolled)" @@ +240,3 @@ + _onSecretInfoQuery: function(client, serviceName, secretQuestion) { + // We only expect secret requests to come from the main auth service + if (serviceName != GdmUtil.PASSWORD_SERVICE_NAME) that gets added inside the gdm-password service file (or rather the password-auth file included in it) so would work
(In reply to comment #149) > Review of attachment 218816 [details] [review]: > > ::: js/ui/userMenu.js > @@ +53,3 @@ > + icon_type: St.IconType.SYMBOLIC, > + icon_size: DIALOG_ICON_SIZE > + }); > > if you use St.Icon instead of what was there before then the avatar won't sit > behind (and get clipped by) its rounded frame anymore right? Ugh. It's not noticeable, but you're right, clipping is wrong. Back to background-image then.
I wonder if it would be possible to just make StIcon (or all StWidgets) cope with the clipping.
(In reply to comment #132) > Review of attachment 219056 [details] [review]: > > ::: js/ui/screenShield.js > @@ +464,3 @@ > + try { > + Shell.util_kill(this._watchdogPid); > + } catch(e) { } > > I don't see the point in reporting an error if we just ignore it. shell_util_kill (which really should be g_kill, or g_subprocess_terminate) is a library function, and it makes sense for it have a GError. (In reply to comment #146) > Review of attachment 219049 [details] [review]: > > ::: js/ui/screenShield.js > @@ -91,3 @@ > > - _onButtonPressEvent: function(object, buttonPressEvent) { > - log("in _onButtonPressEvent - lock is enabled: " + > this._settings.get_boolean(LOCK_ENABLED_KEY)); > > still have these pesky logs around They're part of the original "add initial functionality" patch, which was from Marina, and it don't want to modify it too much. It's removed anyway. (In reply to comment #147) > Review of attachment 219069 [details] [review]: > > Not the biggest fan of the session mode stuff. Can't you just pass it into a > parameter? What do you mean? Having a (enum? constructor obj?) parameter to ScreenShield.showDialog? > ::: js/ui/main.js > @@ +107,1 @@ > loginDialog.connect('loaded', function() { > > Why does the loginDialog need to load, and the unlockDialog need not? The login dialog needs to load the user list from AccountsService, the unlock dialog has that cached from userMenu. I'm ok with a 'loaded' signal at the end of UnlockDialog, though. > @@ +113,3 @@ > +function createSessionUnlockDialog(parentActor) { > + let dialog = new UnlockDialog.UnlockDialog(parentActor); > + if (!dialog.open()) { > > Woah now. create != open > > ::: js/ui/screenShield.js > @@ +237,3 @@ > + > + if (!this._dialog) { > + [this._dialog, this._keepDialog] = > Main.sessionMode.createUnlockDialog(this._lockDialogGroup); > > This keepDialog stuff is iffy to me. I'd rather have the thing that sets up the > screen shield in both cases give it a parameter on construction. Uhm... ScreenShield is created outside of SessionMode, so it would be a SessionMode option, but how would you call it? At least with this it's easy to track where it's used and how. (In reply to comment #151) > Review of attachment 219056 [details] [review]: > > ::: src/shell-screenshield-watchdog.c > @@ +194,3 @@ > + case ConfigureNotify: > + if (!(xevent->xany.window == gdk_x11_window_get_xid (gdk_window))) > + gdk_window_raise (gdk_window); > > So I know gnome-screensaver does this, but another idea might to use the > MIT-Screensaver window. It should be always on top. i did a little example > program a few years ago here: > http://cgit.freedesktop.org/~halfline/test-screensaver-extension/tree/test-screensaver-extension.c > or you could potentially leverage the COW MIT-Screensaver seems more complexity and libX11 than it's worth. > @@ +241,3 @@ > + 0, 0, > + gdk_screen_get_width (gdk_screen), > + gdk_screen_get_height (gdk_screen)); > > I guess we don't support zaphod mode multi-screen setups right now at all with > the shell, but it might make sense to quickly cover it here to futureproof the > code. Is that really the future? Multiseat is handled with multiple servers, and multihead is handled with xrandr (including multigpu) - there should be no need for multiple X Screens. (And then there is wayland, where there isn't even a Screen concept) > @@ +251,3 @@ > + on_name_appeared, > + on_name_vanished, > + NULL, NULL); > > This is probably fine, but i wonder if DBus's little used queueing feature > would help here (i.e. take the org.gnome.shell name but using the queue flag to > make it so you don't get the name until the instant it becomes available.) But gnome-shell doesn't pass REPLACE_EXISTING unless --replace is on the command line, so restarting would fail.
Created attachment 219104 [details] [review] UserMenu: split user avatar into its own widget This way, it can be reused in the lock screen without code duplication.
Created attachment 219110 [details] [review] St: add :disabled pseudo class when a button is not reactive The :reactive property is used on StButton to like the :sensitive property on GtkWidgets, that is, to indicate that the user is not (yet) expected to click the button, and therefore should affect styling too. This allows to remove some code at the JS layer.
Created attachment 219113 [details] [review] St: don't attempt to give focus to non reactive actors Non reactive actors don't expect to be interacted with, and thus should not get keyboard focus.
Created attachment 219114 [details] [review] Split some common code out of gdm/loginDialog This will be reused by session unlocking.
Created attachment 219115 [details] [review] Add UnlockDialog for unlocking the screen shield When the screenshield is deactivated, instead of going back to the session immediately, prompt the user for authentication. This essentially reinstates what used to be provided by gnome-screensaver.
Created attachment 219117 [details] [review] Consolidate creation of login and unlock dialog in the screenshield The design calls for the curtain to appear in the gdm greeter too. Implement this by having the screenshield manage the login dialog (delegating its creation to SessionMode).
Review of attachment 219110 [details] [review]: Sure.
Review of attachment 219113 [details] [review]: Sure.
Review of attachment 219104 [details] [review]: Looks fine.
Review of attachment 219114 [details] [review]: Looks fine.
Review of attachment 219115 [details] [review]: One minor nit. ::: js/ui/unlockDialog.js @@ +171,3 @@ + this._okButton.button.reactive = sensitive; + if (sensitive) + this._okButton.button.remove_style_pseudo_class('disabled'); This is still here.
So remember that part where I said we should split the PAM handling out? We're going to need to do that, as I'm going to need to use it in a third place, maybe.
(In reply to comment #156) > > ::: src/shell-screenshield-watchdog.c > > @@ +194,3 @@ > > + case ConfigureNotify: > > + if (!(xevent->xany.window == gdk_x11_window_get_xid (gdk_window))) > > + gdk_window_raise (gdk_window); > > > > So I know gnome-screensaver does this, but another idea might to use the > > MIT-Screensaver window. It should be always on top. i did a little example > > program a few years ago here: > > http://cgit.freedesktop.org/~halfline/test-screensaver-extension/tree/test-screensaver-extension.c > > or you could potentially leverage the COW > > MIT-Screensaver seems more complexity and libX11 than it's worth. If you don't use the screensaver window then you at least need to resize your window when the screen changes size. I actually think using the screensaver extension would be less code then trying to maintain "king of the hill" status (you might have been scared off in that example i posted because it has a bunch of code you wouldn't need that puts an animation window per monitor up) Another idea (that might not work) would be to have a window that's a sibling of the clutter stage (set up by gnome-shell at login time). iconify it so it doesn't get in the way, use XSetCloseDownMode to make it stick around after gnome-shell crashes, and then I think when the window manager goes away the X server will automatically map it. Since it's a sibling of the clutter stage, it's a direct child of the composite overlay window, so should be always on top.
Created attachment 219217 [details] [review] Split some common code out of gdm/loginDialog This will be reused by session unlocking. Includes a split of the PAM code into a common class, as discussed on IRC. Tested for unlock, not for login.
Created attachment 219218 [details] [review] Add UnlockDialog for unlocking the screen shield When the screenshield is deactivated, instead of going back to the session immediately, prompt the user for authentication. This essentially reinstates what used to be provided by gnome-screensaver.
Created attachment 219219 [details] [review] UnlockDialog: show a numeric keypad when using PIN authentication When using pam_pin.so to login with a numeric PIN, show a numeric keypad to make it easier to type, especially on touch devices.
Created attachment 219222 [details] [review] MessageTray: rework icon handling to split model and view To allow more than one summary icon actor for a source we split the model of the source icon (which is iconName, if the default implementation is used, or a GIcon otherwise) and replace createNotificationIcon() with a generic createIcon(size). Also, the actual source actor is split into a separate class, that handles the notification counter automatically.
Created attachment 219223 [details] [review] ScreenShield: show notifications in the locked screen Track screen lock status in the message tray, and filter banner notifications. The message tray is completely hidden when the screen is locked, but exceptions can be made for individual transient notifications, such as shell messages and the on screen keyboard. Non transient sources are shown in the middle of the lock screen. Resident notifications (such as those from Rhythmbox) are shown in full, while persistent ones are displayed as icon and message count.
Created attachment 219226 [details] [review] ModalDialog: add 'default' pseudo-class to default button Add 'default' parameter to setButtons, that controls the binding of Return (unless overridden) and applies the 'default' pseudo-class. Currently it has no effect, but it will start having after the login dialog redesign. Not attaching the obvious rebases to "Add UnlockDialog for unlocking the screen shield" and "Login Dialogs: update styling to match mockups"
Review of attachment 219217 [details] [review]: Nice. Thank you so much!
Review of attachment 219218 [details] [review]: And it makes this commit so much cleaner.
Review of attachment 219222 [details] [review]: I wonder if we can scrap the mainIcon stuff, so that it's truly pure. I don't really care that much, but this is a really nice cleanup. ACN if not possible. ::: js/ui/messageTray.js @@ +1030,3 @@ Signals.addSignalMethods(Notification.prototype); +const SourceCounterActor = new Lang.Class({ This is a bit of a bad name, as it contains both the icon and the counter. @@ +1168,3 @@ return new St.Icon({ icon_name: this.iconName, icon_type: this.iconType, + icon_size: size }); Ana was going to do this, but sure. :) @@ +1174,2 @@ // there is only one summary icon actor for a Source. getSummaryIcon: function() { Where is this used? @@ +1220,2 @@ //// Protected methods //// _setSummaryIcon: function(icon) { I wonder if this should emit a signal that source actors listen to.
Review of attachment 219219 [details] [review]: Awesome.
Review of attachment 219223 [details] [review]: Much better overall. Man, you are *fast*. ::: js/ui/screenShield.js @@ +165,3 @@ + + let countLabel = new St.Label({ text: this._makeNotificationCountText(source), + style_class: 'screen-shield-notification-count-text' }); Not sure what this is about -- the source counter actor already has a counter. Are we going to have two counters? Also, you never connect to count-updated here.
Review of attachment 219226 [details] [review]: Yes.
Review of attachment 219226 [details] [review]: Oh. Except the commit message is a lie, if I read this right. It has every effect in the world, as it binds "Return". You just haven't introduced the fancy style class yet :)
Review of attachment 219117 [details] [review]: Yep, this is much better.
(In reply to comment #180) > Review of attachment 219223 [details] [review]: > > Much better overall. Man, you are *fast*. > > ::: js/ui/screenShield.js > @@ +165,3 @@ > + > + let countLabel = new St.Label({ text: > this._makeNotificationCountText(source), > + style_class: > 'screen-shield-notification-count-text' }); > > Not sure what this is about -- the source counter actor already has a counter. > Are we going to have two counters? Yes, https://live.gnome.org/GnomeOS/Design/Whiteboards/ScreenLock shows two counter (one overlaid to the icon), and one part of the "%d new messages" label. > Also, you never connect to count-updated here. Because there wasn't one. Will fix.
(In reply to comment #184) > Yes, https://live.gnome.org/GnomeOS/Design/Whiteboards/ScreenLock shows two > counter (one overlaid to the icon), and one part of the "%d new messages" > label. Gotcha.
(In reply to comment #178) > Review of attachment 219222 [details] [review]: > > I wonder if we can scrap the mainIcon stuff, so that it's truly pure. I don't > really care that much, but this is a really nice cleanup. ACN if not possible. The problem is with legacy trayIcons. There can only be one Shell.EmbeddedWindow wrapping them (created by native code and passed through the StatusIconDispatcher), the others are Clutter.Clones. If it weren't for that, SummaryItem could just create a SourceCounterActor like ScreenShield does (and even better, SourceCounterActor could create the icon), and the summaryIcon getter/setter in Source could go away. It would be interesting to have more than one ShellEmbeddedWindow for one GtkWindow (like you planned to do with MetaWindowActor). PS: do you have a better name than SourceCounterActor?
(In reply to comment #186) > (In reply to comment #178) > > Review of attachment 219222 [details] [review] [details]: > > > > I wonder if we can scrap the mainIcon stuff, so that it's truly pure. I don't > > really care that much, but this is a really nice cleanup. ACN if not possible. > > The problem is with legacy trayIcons. There can only be one > Shell.EmbeddedWindow wrapping them (created by native code and passed through > the StatusIconDispatcher), the others are Clutter.Clones. > If it weren't for that, SummaryItem could just create a SourceCounterActor like > ScreenShield does (and even better, SourceCounterActor could create the icon), > and the summaryIcon getter/setter in Source could go away. Why does that require a mainIcon? Couldn't we just return a ClutterClone from createNotificationIcon for the icon in the first place? > It would be interesting to have more than one ShellEmbeddedWindow for one > GtkWindow (like you planned to do with MetaWindowActor). It would. I don't know if it actually binds a GLX texture, then. If it does, NVIDIA will go crazy if you have more than one GLX texture bound to the same window. > PS: do you have a better name than SourceCounterActor? SourceActor
Created attachment 219231 [details] [review] ScreenShield: implement curtain design This separates the screen shield into two main screens. One is the lock screen, and it is shown when coming back from idle status and when failing authentication. The other is the actual unlock dialog. Moving from the first to the second is possible by pressing Escape or by dragging an arrow on the bottom on the screen.
Created attachment 219232 [details] [review] ScreenShield: show notifications in the locked screen Track screen lock status in the message tray, and filter banner notifications. The message tray is completely hidden when the screen is locked, but exceptions can be made for individual transient notifications, such as shell messages and the on screen keyboard. Non transient sources are shown in the middle of the lock screen. Resident notifications (such as those from Rhythmbox) are shown in full, while persistent ones are displayed as icon and message count.
Just my 2¢ as I am forced to part from this bug report: github (or gitorious) is a fine place to push and review commits :)
Review of attachment 219231 [details] [review]: Sure.
Created attachment 219238 [details] [review] Add a clock to the lock screen Start implementing the lock screen design by adding a big white clock in the middle.
Review of attachment 219232 [details] [review]: Looks good.
Review of attachment 219238 [details] [review]: Yes. ::: js/ui/screenShield.js @@ +26,3 @@ + + CLOCK_FORMAT_KEY: 'clock-format', + CLOCK_SHOW_SECONDS_KEY: 'clock-show-seconds', these are unused
Review of attachment 219056 [details] [review]: (moving to needs-work since it needs to track screen size changes)
Created attachment 219316 [details] [review] ScreenShield: add a watchdog process in case the shell crashes If the shell crashes while the screenshield is active, the lock screen is teared down, and that's potentially a security issue. To mitigate that, add a process that keeps a full black screen on top of everything (except the shell itself) and grabs the mouse and keyboard if the shell crashes.
UXHackfest is two days, so to have something presentable, I'm merging the accepted (or acceptable) patches now.
Attachment 218268 [details] pushed as 4e6fa56 - screenShield: add initial functionality Attachment 218271 [details] pushed as f5e58c5 - Modal stack: fix handling of destroyed actors Attachment 218274 [details] pushed as 54dc0fd - ScreenShield: use LayoutManager for creating the actor Attachment 218282 [details] pushed as 01a1255 - ShellEntry: make isPassword param changeable at runtime Attachment 218288 [details] pushed as e0bb15e - Login Dialogs: update styling to match mockups Attachment 218638 [details] pushed as 5e865f5 - LayoutManager: reverse the visibleInFullscreen flag Attachment 219047 [details] pushed as d9c3b83 - Panel: fix finding leftmost and rightmost buttons Attachment 219050 [details] pushed as c22a00a - ScreenShield: improve locking/modal policy Attachment 219054 [details] pushed as 34cb92f - Don't track the status of the screensaver on DBus Attachment 219055 [details] pushed as 0e4171a - Screen Shield: animate manual locking Attachment 219057 [details] pushed as f7f2f50 - ShellDBus: export screensaver interface Attachment 219071 [details] pushed as c3afe1a - Show the panel above the screenshield when locked Attachment 219104 [details] pushed as 2c073fb - UserMenu: split user avatar into its own widget Attachment 219110 [details] pushed as a29507e - St: add :disabled pseudo class when a button is not reactive Attachment 219113 [details] pushed as 26d3b19 - St: don't attempt to give focus to non reactive actors Attachment 219117 [details] pushed as a28d639 - Consolidate creation of login and unlock dialog in the screenshield Attachment 219217 [details] pushed as 46db9ed - Split some common code out of gdm/loginDialog Attachment 219218 [details] pushed as 2c3ec78 - Add UnlockDialog for unlocking the screen shield Attachment 219222 [details] pushed as ac6c808 - MessageTray: rework icon handling to split model and view Attachment 219226 [details] pushed as 8970e17 - ModalDialog: add 'default' pseudo-class to default button Attachment 219231 [details] pushed as c0652bc - ScreenShield: implement curtain design Attachment 219232 [details] pushed as 22eea75 - ScreenShield: show notifications in the locked screen Attachment 219238 [details] pushed as 904ceba - Add a clock to the lock screen Not pushing PIN support since the necessary patches to accountsservice haven't landed yet.
retitling for whats left
The pin infrastructure bug is http://bugs.freedesktop.org/show_bug.cgi?id=51833
Some initial impressions from trying the new lock screen: - First of all, this is really awesome work. Of course, I'll have a long list of complaints below, but they are really minor, compared to how well this already works overall. - When raising the curtain, I can see the 'Password' label appear, which is a little irritating - shouldn't we have that label there from the start ? Also, the label is not centered wrt to the entry. - Using the Escape key to raise the curtain works - but only the first time. After entering a wrong password, the curtain comes down again... and Esc no longer works. - Changing keyboard layouts works, but input methods don't seem to work for the password entry. Should they ? - The 'Log in as another user' and the 'Unlock' button have no keyboard focus indication at all. - The timeout for bringing the curtain back down doesn't seem to take user activity into account ? I've had the curtain fall right after making changes in the menus, which feels wrong. - What also feels wrong is that I get a 'Authentication failure' notification when the timeout kicks in. - There is no way to dismiss notifications - is that intentional ? the notification bubble keeps growing and pushes the clock up and pulls the background down, until the gray starts showing through. I think we should limit the size of the notification bubble, and scroll. - When dragging the curtain up without letting go, the gray is empty, no sign of the login prompt etc. I think the 'dialog' should be there all the time. - The pointer should be hidden on the curtain until the mouse is moved
There is also the watchdog patch still in this bug, but anyway, let's keep this for the PIN work. I'll open a new bug with patches from the Screen Lock BOF.
Ping? Can I get the watchdog reviewed, before this bug falls into the 3.8 land?
Review of attachment 219316 [details] [review]: I don't understand the logic here. The shell crashes, a new shell appears, the watchdog window quits. What guarantees the new shell has the lock screen showing? I've been "crashing" the shell because I don't have a new GDM with Ctrl+C on a VT, and it bypasses the lock screen for me.
Good point, effectively nothing does, and the shell just happily restarts... I wonder how to implement that, without big changes in gnome-session (whose presence state machine is kind of broken, IMHO)...
Any reason the PIN didn't go through?
Lets pick this up again for 3.8
One more ping on PIN?
still blocked on http://bugs.freedesktop.org/show_bug.cgi?id=51833
Created attachment 236319 [details] [review] UnlockDialog: show a numeric keypad when using PIN authentication When using pam_pin.so to login with a numeric PIN, show a numeric keypad to make it easier to type, especially on touch devices. Rebased, because the referenced commits were garbage collected.
Created attachment 236371 [details] [review] Login/UnlockDialog: allow choosing explicitly the authentication method The design calls for an explicit choice between password and PIN, so we should start both conversation and then complete the one chosen by the user. If PIN is not available, the chooser is automatically hidden and we go back to password as usual. I rebased the patches on bug 691300, which had some code to deal with service-unavailable.
Created attachment 236372 [details] Screencast of the new chooser
Created attachment 236374 [details] [review] UnlockDialog: Improve the look of the pin pad Use bigger, square buttons, with larger text.
Created attachment 236375 [details] New look after the last patch It's closer to the mockups, and it looks more "touchy", but I don't know, it just doesn't feel right to me... Maybe it's because it pushes the entry up.
past feature freeze...didn't make it in...lets try again next time
I think this has existed long enough, and the code / design has strayed far enough that if we want to add PIN support, we'd need a new design, and some new code. Closing.
Filed bug 735782 for the follow-up.