After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 619955 - Add pin support to the lock screen
Add pin support to the lock screen
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
touch
: 660945 678284 (view as bug list)
Depends on: 676381 677412 677640 680256
Blocks:
 
 
Reported: 2010-05-28 15:43 UTC by William Jon McCann
Modified: 2014-09-01 04:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screenShield: add initial functionality (11.09 KB, patch)
2012-07-08 14:59 UTC, Giovanni Campagna
committed Details | Review
Add UnlockDialog for unlocking the screen shield (23.09 KB, patch)
2012-07-08 15:00 UTC, Giovanni Campagna
needs-work Details | Review
ScreenShield: improve locking/modal policy (4.09 KB, patch)
2012-07-08 15:03 UTC, Giovanni Campagna
reviewed Details | Review
Modal stack: fix handling of destroyed actors (1.01 KB, patch)
2012-07-08 15:05 UTC, Giovanni Campagna
committed Details | Review
ScreenShield: reset isLocked flag when unlocking (880 bytes, patch)
2012-07-08 15:05 UTC, Giovanni Campagna
needs-work Details | Review
LayoutManager: reverse the visibleInFullscreen flag (5.77 KB, patch)
2012-07-08 15:06 UTC, Giovanni Campagna
needs-work Details | Review
ScreenShield: use LayoutManager for creating the actor (4.51 KB, patch)
2012-07-08 15:06 UTC, Giovanni Campagna
committed Details | Review
LayoutManager: don't track the status of the screensaver on DBus (4.87 KB, patch)
2012-07-08 15:06 UTC, Giovanni Campagna
needs-work Details | Review
MessageTray: filter notifications in the screen lock (8.70 KB, patch)
2012-07-08 15:06 UTC, Giovanni Campagna
needs-work Details | Review
Show the panel above the screenshield when locked (9.80 KB, patch)
2012-07-08 15:06 UTC, Giovanni Campagna
rejected Details | Review
Remove usage of ScreenSaver DBus interface (8.28 KB, patch)
2012-07-08 15:07 UTC, Giovanni Campagna
rejected Details | Review
ScreenShield: implement curtain design (14.39 KB, patch)
2012-07-08 15:07 UTC, Giovanni Campagna
needs-work Details | Review
Do not use constraints for alignment (4.86 KB, patch)
2012-07-08 15:07 UTC, Giovanni Campagna
none Details | Review
Consolidate creation of login and unlock dialog in the screenshield (19.11 KB, patch)
2012-07-08 15:07 UTC, Giovanni Campagna
rejected Details | Review
ShellEntry: make isPassword param changeable at runtime (4.34 KB, patch)
2012-07-08 15:08 UTC, Giovanni Campagna
committed Details | Review
UnlockDialog: a few usability improvements (2.08 KB, patch)
2012-07-08 15:08 UTC, Giovanni Campagna
none Details | Review
Add a clock to the lock screen (6.57 KB, patch)
2012-07-08 15:08 UTC, Giovanni Campagna
needs-work Details | Review
ScreenShield: show notifications in the locked screen (18.07 KB, patch)
2012-07-08 15:08 UTC, Giovanni Campagna
none Details | Review
Screen Shield: animate manual locking (4.38 KB, patch)
2012-07-08 15:08 UTC, Giovanni Campagna
none Details | Review
Show more of the status area in the locked screen (2.84 KB, patch)
2012-07-08 15:08 UTC, Giovanni Campagna
none Details | Review
Login Dialogs: update styling to match mockups (6.49 KB, patch)
2012-07-08 15:10 UTC, Giovanni Campagna
committed Details | Review
UnlockDialog: add the "Login with other user" link (2.86 KB, patch)
2012-07-08 15:10 UTC, Giovanni Campagna
none Details | Review
ScreenShield: make the entire lock screen draggable (2.26 KB, patch)
2012-07-08 15:12 UTC, Giovanni Campagna
none Details | Review
ScreenShield: add a watchdog process in case the shell crashes (13.28 KB, patch)
2012-07-08 15:12 UTC, Giovanni Campagna
accepted-commit_now Details | Review
Layout: ignore fullscreen windows when the lock screen is active (2.10 KB, patch)
2012-07-08 15:12 UTC, Giovanni Campagna
accepted-commit_now Details | Review
User Menu: wait for the animation before suspending (2.50 KB, patch)
2012-07-08 15:12 UTC, Giovanni Campagna
reviewed Details | Review
ShellDBus: export screensaver interface (3.03 KB, patch)
2012-07-08 15:12 UTC, Giovanni Campagna
needs-work Details | Review
Fix notifications from PAM (3.85 KB, patch)
2012-07-08 15:25 UTC, Giovanni Campagna
none Details | Review
Allow the shell to run without the screenshield (4.31 KB, patch)
2012-07-08 15:44 UTC, Giovanni Campagna
accepted-commit_now Details | Review
UnlockDialog: show a numeric keypad when using PIN authentication (5.75 KB, patch)
2012-07-10 17:24 UTC, Giovanni Campagna
accepted-commit_now Details | Review
Add UnlockDialog for unlocking the screen shield (43.95 KB, patch)
2012-07-12 15:05 UTC, Giovanni Campagna
needs-work Details | Review
ScreenShield: improve locking/modal policy (3.67 KB, patch)
2012-07-12 15:06 UTC, Giovanni Campagna
none Details | Review
LayoutManager: reverse the visibleInFullscreen flag (5.74 KB, patch)
2012-07-12 15:06 UTC, Giovanni Campagna
committed Details | Review
ScreenShield: implement curtain design (13.49 KB, patch)
2012-07-12 15:32 UTC, Giovanni Campagna
needs-work Details | Review
ScreenShield: show notifications in the locked screen (20.06 KB, patch)
2012-07-12 15:33 UTC, Giovanni Campagna
needs-work Details | Review
UserMenu: split user avatar into its own widget (4.42 KB, patch)
2012-07-14 15:22 UTC, Giovanni Campagna
reviewed Details | Review
Add UnlockDialog for unlocking the screen shield (41.81 KB, patch)
2012-07-14 15:23 UTC, Giovanni Campagna
reviewed Details | Review
ScreenShield: improve locking/modal policy (3.49 KB, patch)
2012-07-14 15:24 UTC, Giovanni Campagna
reviewed Details | Review
Don't track the status of the screensaver on DBus (11.05 KB, patch)
2012-07-14 15:25 UTC, Giovanni Campagna
reviewed Details | Review
ScreenShield: implement curtain design (13.10 KB, patch)
2012-07-14 15:25 UTC, Giovanni Campagna
reviewed Details | Review
Show the panel above the screenshield when locked (10.16 KB, patch)
2012-07-14 15:26 UTC, Giovanni Campagna
needs-work Details | Review
ScreenShield: show notifications in the locked screen (18.43 KB, patch)
2012-07-14 15:27 UTC, Giovanni Campagna
reviewed Details | Review
Panel: fix finding leftmost and rightmost buttons (1.65 KB, patch)
2012-07-17 18:52 UTC, Giovanni Campagna
committed Details | Review
Split some common code out of gdm/loginDialog (16.99 KB, patch)
2012-07-17 18:57 UTC, Giovanni Campagna
needs-work Details | Review
Add UnlockDialog for unlocking the screen shield (24.64 KB, patch)
2012-07-17 18:58 UTC, Giovanni Campagna
needs-work Details | Review
ScreenShield: improve locking/modal policy (4.36 KB, patch)
2012-07-17 18:59 UTC, Giovanni Campagna
committed Details | Review
Consolidate creation of login and unlock dialog in the screenshield (10.50 KB, patch)
2012-07-17 19:00 UTC, Giovanni Campagna
reviewed Details | Review
Add a clock to the lock screen (5.94 KB, patch)
2012-07-17 19:00 UTC, Giovanni Campagna
reviewed Details | Review
Show the panel above the screenshield when locked (9.25 KB, patch)
2012-07-17 19:00 UTC, Giovanni Campagna
reviewed Details | Review
Don't track the status of the screensaver on DBus (10.45 KB, patch)
2012-07-17 19:01 UTC, Giovanni Campagna
committed Details | Review
Screen Shield: animate manual locking (5.01 KB, patch)
2012-07-17 19:02 UTC, Giovanni Campagna
committed Details | Review
ScreenShield: add a watchdog process in case the shell crashes (13.26 KB, patch)
2012-07-17 19:02 UTC, Giovanni Campagna
needs-work Details | Review
ShellDBus: export screensaver interface (3.89 KB, patch)
2012-07-17 19:07 UTC, Giovanni Campagna
committed Details | Review
UnlockDialog: show a numeric keypad when using PIN authentication (6.07 KB, patch)
2012-07-17 19:07 UTC, Giovanni Campagna
accepted-commit_now Details | Review
Allow the shell to run without the screenshield (4.42 KB, patch)
2012-07-17 20:58 UTC, Giovanni Campagna
rejected Details | Review
Show the panel above the screenshield when locked (9.27 KB, patch)
2012-07-17 20:58 UTC, Giovanni Campagna
none Details | Review
Consolidate creation of login and unlock dialog in the screenshield (10.43 KB, patch)
2012-07-17 20:59 UTC, Giovanni Campagna
needs-work Details | Review
Show the panel above the screenshield when locked (9.60 KB, patch)
2012-07-17 21:08 UTC, Giovanni Campagna
committed Details | Review
UserMenu: split user avatar into its own widget (4.89 KB, patch)
2012-07-18 12:58 UTC, Giovanni Campagna
committed Details | Review
St: add :disabled pseudo class when a button is not reactive (2.02 KB, patch)
2012-07-18 12:59 UTC, Giovanni Campagna
committed Details | Review
St: don't attempt to give focus to non reactive actors (4.15 KB, patch)
2012-07-18 12:59 UTC, Giovanni Campagna
committed Details | Review
Split some common code out of gdm/loginDialog (17.25 KB, patch)
2012-07-18 12:59 UTC, Giovanni Campagna
accepted-commit_now Details | Review
Add UnlockDialog for unlocking the screen shield (24.52 KB, patch)
2012-07-18 13:00 UTC, Giovanni Campagna
accepted-commit_now Details | Review
Consolidate creation of login and unlock dialog in the screenshield (11.32 KB, patch)
2012-07-18 13:12 UTC, Giovanni Campagna
committed Details | Review
Split some common code out of gdm/loginDialog (30.02 KB, patch)
2012-07-19 12:16 UTC, Giovanni Campagna
committed Details | Review
Add UnlockDialog for unlocking the screen shield (20.93 KB, patch)
2012-07-19 12:17 UTC, Giovanni Campagna
committed Details | Review
UnlockDialog: show a numeric keypad when using PIN authentication (6.64 KB, patch)
2012-07-19 12:17 UTC, Giovanni Campagna
accepted-commit_now Details | Review
MessageTray: rework icon handling to split model and view (17.80 KB, patch)
2012-07-19 13:21 UTC, Giovanni Campagna
committed Details | Review
ScreenShield: show notifications in the locked screen (18.28 KB, patch)
2012-07-19 13:22 UTC, Giovanni Campagna
reviewed Details | Review
ModalDialog: add 'default' pseudo-class to default button (5.53 KB, patch)
2012-07-19 13:38 UTC, Giovanni Campagna
committed Details | Review
ScreenShield: implement curtain design (12.93 KB, patch)
2012-07-19 14:32 UTC, Giovanni Campagna
committed Details | Review
ScreenShield: show notifications in the locked screen (18.31 KB, patch)
2012-07-19 14:34 UTC, Giovanni Campagna
committed Details | Review
Add a clock to the lock screen (4.79 KB, patch)
2012-07-19 15:05 UTC, Giovanni Campagna
committed Details | Review
ScreenShield: add a watchdog process in case the shell crashes (13.77 KB, patch)
2012-07-20 11:45 UTC, Giovanni Campagna
reviewed Details | Review
UnlockDialog: show a numeric keypad when using PIN authentication (6.46 KB, patch)
2013-02-15 23:04 UTC, Giovanni Campagna
none Details | Review
Login/UnlockDialog: allow choosing explicitly the authentication method (21.26 KB, patch)
2013-02-16 15:58 UTC, Giovanni Campagna
none Details | Review
Screencast of the new chooser (604.57 KB, video/webm)
2013-02-16 16:00 UTC, Giovanni Campagna
  Details
UnlockDialog: Improve the look of the pin pad (1.96 KB, patch)
2013-02-16 16:12 UTC, Giovanni Campagna
none Details | Review
New look after the last patch (444.54 KB, image/png)
2013-02-16 16:14 UTC, Giovanni Campagna
  Details

Description William Jon McCann 2010-05-28 15:43:28 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
Comment 1 William Jon McCann 2010-05-28 15:51:06 UTC
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
Comment 2 Jean-François Fortin Tam 2011-09-22 16:27:26 UTC
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 :)
Comment 3 William Jon McCann 2011-09-22 16:29:23 UTC
I suppose things may change once ConsoleKit is replaced by systemd. Not sure.
Comment 4 Florian Müllner 2011-10-05 01:24:58 UTC
*** Bug 660945 has been marked as a duplicate of this bug. ***
Comment 5 Lionel Dricot 2011-11-06 09:02:59 UTC
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?
Comment 6 Matthias Clasen 2011-11-06 16:02:13 UTC
This is being worked on: https://live.gnome.org/ThreePointThree/Features/ScreenLock
Comment 7 Giovanni Campagna 2012-06-07 17:34:14 UTC
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.
Comment 8 Bastien Nocera 2012-06-18 18:21:26 UTC
*** Bug 678284 has been marked as a duplicate of this bug. ***
Comment 9 Giovanni Campagna 2012-07-08 14:59:35 UTC
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.
Comment 10 Giovanni Campagna 2012-07-08 15:00:03 UTC
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.
Comment 11 Giovanni Campagna 2012-07-08 15:03:54 UTC
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.
Comment 12 Giovanni Campagna 2012-07-08 15:05:02 UTC
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().
Comment 13 Giovanni Campagna 2012-07-08 15:05:39 UTC
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).
Comment 14 Giovanni Campagna 2012-07-08 15:06:06 UTC
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.
Comment 15 Giovanni Campagna 2012-07-08 15:06:29 UTC
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)
Comment 16 Giovanni Campagna 2012-07-08 15:06:39 UTC
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.
Comment 17 Giovanni Campagna 2012-07-08 15:06:50 UTC
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.
Comment 18 Giovanni Campagna 2012-07-08 15:06:59 UTC
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)
Comment 19 Giovanni Campagna 2012-07-08 15:07:09 UTC
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.
Comment 20 Giovanni Campagna 2012-07-08 15:07:34 UTC
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.
Comment 21 Giovanni Campagna 2012-07-08 15:07:44 UTC
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.
Comment 22 Giovanni Campagna 2012-07-08 15:07:54 UTC
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).
Comment 23 Giovanni Campagna 2012-07-08 15:08:04 UTC
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.
Comment 24 Giovanni Campagna 2012-07-08 15:08:13 UTC
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.
Comment 25 Giovanni Campagna 2012-07-08 15:08:23 UTC
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.
Comment 26 Giovanni Campagna 2012-07-08 15:08:32 UTC
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.
Comment 27 Giovanni Campagna 2012-07-08 15:08:41 UTC
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.
Comment 28 Giovanni Campagna 2012-07-08 15:08:50 UTC
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.
Comment 29 Giovanni Campagna 2012-07-08 15:10:40 UTC
Created attachment 218288 [details] [review]
Login Dialogs: update styling to match mockups

Remove backgrounds, change button styling, reduce font sizes.
Comment 30 Giovanni Campagna 2012-07-08 15:10:50 UTC
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.
Comment 31 Giovanni Campagna 2012-07-08 15:12:15 UTC
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.
Comment 32 Giovanni Campagna 2012-07-08 15:12:24 UTC
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.
Comment 33 Giovanni Campagna 2012-07-08 15:12:33 UTC
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.
Comment 34 Giovanni Campagna 2012-07-08 15:12:42 UTC
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.
Comment 35 Giovanni Campagna 2012-07-08 15:12:56 UTC
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.
Comment 36 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:14:48 UTC
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.
Comment 37 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:18:36 UTC
Review of attachment 218268 [details] [review]:

Sure.
Comment 38 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:24:00 UTC
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.
Comment 39 Giovanni Campagna 2012-07-08 15:25:50 UTC
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.
Comment 40 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:25:57 UTC
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.
Comment 41 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:26:44 UTC
Review of attachment 218271 [details] [review]:

I'm not sure I like the implicit destroy approach. Modal is something we should take very carefully.
Comment 42 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:27:12 UTC
Review of attachment 218272 [details] [review]:

Squash this with whatever patch you had before.
Comment 43 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:28:54 UTC
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.
Comment 44 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:32:06 UTC
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
Comment 45 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:36:36 UTC
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;

...
Comment 46 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:40:26 UTC
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.
Comment 47 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:42:37 UTC
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();

???
Comment 48 Giovanni Campagna 2012-07-08 15:44:35 UTC
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.
Comment 49 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:45:04 UTC
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
Comment 50 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:53:33 UTC
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.
Comment 51 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:53:55 UTC
Review of attachment 218278 [details] [review]:

.
Comment 52 Jasper St. Pierre (not reading bugmail) 2012-07-08 15:54:29 UTC
I'm done reviewing for the day. Somebody else can pick up the pieces if they really want to.
Comment 53 Giovanni Campagna 2012-07-08 16:13:16 UTC
(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.
Comment 54 Giovanni Campagna 2012-07-08 16:18:28 UTC
(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...
Comment 55 Jasper St. Pierre (not reading bugmail) 2012-07-08 17:05:51 UTC
(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.
Comment 56 Jasper St. Pierre (not reading bugmail) 2012-07-08 17:15:55 UTC
(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.
Comment 57 Giovanni Campagna 2012-07-10 17:24:20 UTC
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.
Comment 58 Giovanni Campagna 2012-07-12 14:10:57 UTC
(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)
Comment 59 Giovanni Campagna 2012-07-12 14:13:24 UTC
(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.
Comment 60 Giovanni Campagna 2012-07-12 14:17:30 UTC
(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.
Comment 61 Jasper St. Pierre (not reading bugmail) 2012-07-12 14:22:26 UTC
(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.
Comment 62 Giovanni Campagna 2012-07-12 14:25:56 UTC
(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...
Comment 63 Giovanni Campagna 2012-07-12 15:05:40 UTC
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
Comment 64 Giovanni Campagna 2012-07-12 15:06:04 UTC
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.
Comment 65 Giovanni Campagna 2012-07-12 15:06:27 UTC
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.
Comment 66 Giovanni Campagna 2012-07-12 15:09:47 UTC
I'm only uploading patches for which I made substantial changes. The complete stack (including rebases) is in the git branch, as usual.
Comment 67 Giovanni Campagna 2012-07-12 15:32:06 UTC
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.
Comment 68 Giovanni Campagna 2012-07-12 15:33:26 UTC
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.
Comment 69 Jasper St. Pierre (not reading bugmail) 2012-07-13 01:38:43 UTC
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);
Comment 70 Jasper St. Pierre (not reading bugmail) 2012-07-13 01:38:53 UTC
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);
Comment 71 Jasper St. Pierre (not reading bugmail) 2012-07-13 01:39:38 UTC
Review of attachment 218638 [details] [review]:

Sure.
Comment 72 Jasper St. Pierre (not reading bugmail) 2012-07-13 01:40:40 UTC
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?
Comment 73 Jasper St. Pierre (not reading bugmail) 2012-07-13 01:41:30 UTC
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.
Comment 74 Jasper St. Pierre (not reading bugmail) 2012-07-13 01:45:11 UTC
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.
Comment 75 Jasper St. Pierre (not reading bugmail) 2012-07-13 03:38:10 UTC
(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.)
Comment 76 Giovanni Campagna 2012-07-14 13:53:03 UTC
(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).
Comment 77 Giovanni Campagna 2012-07-14 13:57:44 UTC
(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.
Comment 78 Giovanni Campagna 2012-07-14 14:59:19 UTC
(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.
Comment 79 Giovanni Campagna 2012-07-14 15:09:06 UTC
(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.
Comment 80 Giovanni Campagna 2012-07-14 15:22:44 UTC
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.
Comment 81 Giovanni Campagna 2012-07-14 15:23:24 UTC
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
Comment 82 Giovanni Campagna 2012-07-14 15:24:17 UTC
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.
Comment 83 Giovanni Campagna 2012-07-14 15:25:04 UTC
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.
Comment 84 Giovanni Campagna 2012-07-14 15:25:44 UTC
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.
Comment 85 Giovanni Campagna 2012-07-14 15:26:18 UTC
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).
Comment 86 Giovanni Campagna 2012-07-14 15:27:06 UTC
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.
Comment 87 Jasper St. Pierre (not reading bugmail) 2012-07-14 19:06:09 UTC
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.
Comment 88 Jasper St. Pierre (not reading bugmail) 2012-07-14 19:10:49 UTC
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 :)
Comment 89 Jasper St. Pierre (not reading bugmail) 2012-07-14 20:44:17 UTC
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.
Comment 90 Jasper St. Pierre (not reading bugmail) 2012-07-14 20:47:19 UTC
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?
Comment 91 Jasper St. Pierre (not reading bugmail) 2012-07-14 20:49:31 UTC
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.
Comment 92 Jasper St. Pierre (not reading bugmail) 2012-07-14 21:09:01 UTC
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.
Comment 93 Jasper St. Pierre (not reading bugmail) 2012-07-14 21:11:14 UTC
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 :)
Comment 94 Jasper St. Pierre (not reading bugmail) 2012-07-14 21:12:54 UTC
Review of attachment 218282 [details] [review]:

Sure.
Comment 95 Jasper St. Pierre (not reading bugmail) 2012-07-14 21:16:42 UTC
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;
}
Comment 96 Jasper St. Pierre (not reading bugmail) 2012-07-14 21:19:06 UTC
Review of attachment 218281 [details] [review]:

I think this has been obsoleted.
Comment 97 Jasper St. Pierre (not reading bugmail) 2012-07-14 21:23:52 UTC
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.
Comment 98 Jasper St. Pierre (not reading bugmail) 2012-07-14 21:25:46 UTC
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?
Comment 99 Jasper St. Pierre (not reading bugmail) 2012-07-14 21:35:24 UTC
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.
Comment 100 Jasper St. Pierre (not reading bugmail) 2012-07-14 21:35:59 UTC
Review of attachment 218292 [details] [review]:

There was a patch before where you removed this for some reason. This should be squashed with that.
Comment 101 Jasper St. Pierre (not reading bugmail) 2012-07-14 21:37:26 UTC
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.
Comment 102 Jasper St. Pierre (not reading bugmail) 2012-07-14 21:38:14 UTC
Review of attachment 218293 [details] [review]:

Called it. Squash.
Comment 103 Jasper St. Pierre (not reading bugmail) 2012-07-14 21:39:43 UTC
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 104 Giovanni Campagna 2012-07-17 13:29:43 UTC
Comment on attachment 218297 [details] [review]
Allow the shell to run without the screenshield

Not needed (see bug 677118)
Comment 105 Giovanni Campagna 2012-07-17 17:47:59 UTC
(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)
Comment 106 Giovanni Campagna 2012-07-17 17:54:40 UTC
(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)
Comment 107 Giovanni Campagna 2012-07-17 18:52:30 UTC
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.
Comment 108 Giovanni Campagna 2012-07-17 18:57:44 UTC
Created attachment 219048 [details] [review]
Split some common code out of gdm/loginDialog

This will be reused by session unlocking.
Comment 109 Giovanni Campagna 2012-07-17 18:58:31 UTC
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.
Comment 110 Giovanni Campagna 2012-07-17 18:59:40 UTC
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.
Comment 111 Giovanni Campagna 2012-07-17 19:00:04 UTC
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).
Comment 112 Giovanni Campagna 2012-07-17 19:00:21 UTC
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.
Comment 113 Giovanni Campagna 2012-07-17 19:00:51 UTC
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).
Comment 114 Giovanni Campagna 2012-07-17 19:01:53 UTC
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.
Comment 115 Giovanni Campagna 2012-07-17 19:02:17 UTC
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.
Comment 116 Giovanni Campagna 2012-07-17 19:02:44 UTC
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.
Comment 117 Giovanni Campagna 2012-07-17 19:07:01 UTC
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.
Comment 118 Giovanni Campagna 2012-07-17 19:07:16 UTC
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.
Comment 119 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:20:44 UTC
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.
Comment 120 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:21:27 UTC
Review of attachment 219048 [details] [review]:

Sure.
Comment 121 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:23:17 UTC
(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?
Comment 122 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:24:32 UTC
Review of attachment 219049 [details] [review]:

Cool.
Comment 123 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:25:30 UTC
Review of attachment 219050 [details] [review]:

Sure.
Comment 124 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:26:56 UTC
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.
Comment 125 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:29:48 UTC
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?
Comment 126 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:33:27 UTC
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).
Comment 127 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:34:14 UTC
Review of attachment 219054 [details] [review]:

There was patch that provided a dummy fallback screen shield. Squash with this one, please.
Comment 128 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:36:17 UTC
Review of attachment 219055 [details] [review]:

Looks fine.
Comment 129 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:39:37 UTC
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.
Comment 130 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:40:11 UTC
Review of attachment 219057 [details] [review]:

Fine.
Comment 131 Giovanni Campagna 2012-07-17 19:40:49 UTC
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.
Comment 132 Jasper St. Pierre (not reading bugmail) 2012-07-17 19:41:21 UTC
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.
Comment 133 Giovanni Campagna 2012-07-17 20:03:36 UTC
(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.
Comment 134 Giovanni Campagna 2012-07-17 20:40:20 UTC
(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.
Comment 135 Giovanni Campagna 2012-07-17 20:47:45 UTC
(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.
Comment 136 Giovanni Campagna 2012-07-17 20:52:15 UTC
(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.
Comment 137 Giovanni Campagna 2012-07-17 20:58:21 UTC
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.
Comment 138 Giovanni Campagna 2012-07-17 20:58:58 UTC
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).
Comment 139 Giovanni Campagna 2012-07-17 20:59:41 UTC
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).
Comment 140 Giovanni Campagna 2012-07-17 21:08:27 UTC
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).
Comment 141 Jasper St. Pierre (not reading bugmail) 2012-07-18 02:33:06 UTC
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.
Comment 142 Jasper St. Pierre (not reading bugmail) 2012-07-18 02:33:23 UTC
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.
Comment 143 Jasper St. Pierre (not reading bugmail) 2012-07-18 02:34:03 UTC
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?
Comment 144 Jasper St. Pierre (not reading bugmail) 2012-07-18 02:35:29 UTC
Review of attachment 219057 [details] [review]:

Marked needs-work by accident.
Comment 145 Colin Walters 2012-07-18 02:45:57 UTC
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?
Comment 146 Jasper St. Pierre (not reading bugmail) 2012-07-18 02:53:49 UTC
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.
Comment 147 Jasper St. Pierre (not reading bugmail) 2012-07-18 02:58:53 UTC
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.
Comment 148 Jasper St. Pierre (not reading bugmail) 2012-07-18 02:59:40 UTC
Review of attachment 219067 [details] [review]:

Yeah, I'm not sure it's worth it at this point either.
Comment 149 Ray Strode [halfline] 2012-07-18 05:32:53 UTC
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?
Comment 150 Jasper St. Pierre (not reading bugmail) 2012-07-18 05:54:54 UTC
(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.
Comment 151 Ray Strode [halfline] 2012-07-18 06:01:08 UTC
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.)
Comment 152 Ray Strode [halfline] 2012-07-18 06:05:02 UTC
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.
Comment 153 Ray Strode [halfline] 2012-07-18 06:17:17 UTC
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
Comment 154 Giovanni Campagna 2012-07-18 09:56:35 UTC
(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.
Comment 155 Jasper St. Pierre (not reading bugmail) 2012-07-18 09:59:18 UTC
I wonder if it would be possible to just make StIcon (or all StWidgets) cope with the clipping.
Comment 156 Giovanni Campagna 2012-07-18 12:39:00 UTC
(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.
Comment 157 Giovanni Campagna 2012-07-18 12:58:45 UTC
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.
Comment 158 Giovanni Campagna 2012-07-18 12:59:08 UTC
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.
Comment 159 Giovanni Campagna 2012-07-18 12:59:26 UTC
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.
Comment 160 Giovanni Campagna 2012-07-18 12:59:49 UTC
Created attachment 219114 [details] [review]
Split some common code out of gdm/loginDialog

This will be reused by session unlocking.
Comment 161 Giovanni Campagna 2012-07-18 13:00:06 UTC
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.
Comment 162 Giovanni Campagna 2012-07-18 13:12:23 UTC
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).
Comment 163 Jasper St. Pierre (not reading bugmail) 2012-07-18 16:31:29 UTC
Review of attachment 219110 [details] [review]:

Sure.
Comment 164 Jasper St. Pierre (not reading bugmail) 2012-07-18 16:31:58 UTC
Review of attachment 219113 [details] [review]:

Sure.
Comment 165 Jasper St. Pierre (not reading bugmail) 2012-07-18 16:33:07 UTC
Review of attachment 219104 [details] [review]:

Looks fine.
Comment 166 Jasper St. Pierre (not reading bugmail) 2012-07-18 16:33:55 UTC
Review of attachment 219114 [details] [review]:

Looks fine.
Comment 167 Jasper St. Pierre (not reading bugmail) 2012-07-18 16:34:21 UTC
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.
Comment 168 Jasper St. Pierre (not reading bugmail) 2012-07-18 17:39:25 UTC
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.
Comment 169 Ray Strode [halfline] 2012-07-18 20:31:45 UTC
(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.
Comment 170 Giovanni Campagna 2012-07-19 12:16:39 UTC
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.
Comment 171 Giovanni Campagna 2012-07-19 12:17:14 UTC
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.
Comment 172 Giovanni Campagna 2012-07-19 12:17:47 UTC
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.
Comment 173 Giovanni Campagna 2012-07-19 13:21:13 UTC
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.
Comment 174 Giovanni Campagna 2012-07-19 13:22:03 UTC
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.
Comment 175 Giovanni Campagna 2012-07-19 13:38:48 UTC
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"
Comment 176 Jasper St. Pierre (not reading bugmail) 2012-07-19 13:43:04 UTC
Review of attachment 219217 [details] [review]:

Nice. Thank you so much!
Comment 177 Jasper St. Pierre (not reading bugmail) 2012-07-19 13:44:34 UTC
Review of attachment 219218 [details] [review]:

And it makes this commit so much cleaner.
Comment 178 Jasper St. Pierre (not reading bugmail) 2012-07-19 13:48:26 UTC
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.
Comment 179 Jasper St. Pierre (not reading bugmail) 2012-07-19 13:48:55 UTC
Review of attachment 219219 [details] [review]:

Awesome.
Comment 180 Jasper St. Pierre (not reading bugmail) 2012-07-19 13:53:35 UTC
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.
Comment 181 Jasper St. Pierre (not reading bugmail) 2012-07-19 13:54:44 UTC
Review of attachment 219226 [details] [review]:

Yes.
Comment 182 Jasper St. Pierre (not reading bugmail) 2012-07-19 13:55:43 UTC
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 :)
Comment 183 Jasper St. Pierre (not reading bugmail) 2012-07-19 13:57:09 UTC
Review of attachment 219117 [details] [review]:

Yep, this is much better.
Comment 184 Giovanni Campagna 2012-07-19 14:11:51 UTC
(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.
Comment 185 Jasper St. Pierre (not reading bugmail) 2012-07-19 14:17:16 UTC
(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.
Comment 186 Giovanni Campagna 2012-07-19 14:26:53 UTC
(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?
Comment 187 Jasper St. Pierre (not reading bugmail) 2012-07-19 14:32:46 UTC
(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
Comment 188 Giovanni Campagna 2012-07-19 14:32:51 UTC
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.
Comment 189 Giovanni Campagna 2012-07-19 14:34:29 UTC
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.
Comment 190 Jean-François Fortin Tam 2012-07-19 14:35:52 UTC
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 :)
Comment 191 Jasper St. Pierre (not reading bugmail) 2012-07-19 15:05:48 UTC
Review of attachment 219231 [details] [review]:

Sure.
Comment 192 Giovanni Campagna 2012-07-19 15:05:55 UTC
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.
Comment 193 Jasper St. Pierre (not reading bugmail) 2012-07-19 15:06:49 UTC
Review of attachment 219232 [details] [review]:

Looks good.
Comment 194 Jasper St. Pierre (not reading bugmail) 2012-07-19 15:22:06 UTC
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
Comment 195 Ray Strode [halfline] 2012-07-19 21:39:41 UTC
Review of attachment 219056 [details] [review]:

(moving to needs-work since it needs to track screen size changes)
Comment 196 Giovanni Campagna 2012-07-20 11:45:11 UTC
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.
Comment 197 Giovanni Campagna 2012-07-21 13:38:30 UTC
UXHackfest is two days, so to have something presentable, I'm merging the accepted (or acceptable) patches now.
Comment 198 Giovanni Campagna 2012-07-21 13:42:18 UTC
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.
Comment 199 Matthias Clasen 2012-07-21 14:58:06 UTC
retitling for whats left
Comment 200 Matthias Clasen 2012-07-21 15:01:17 UTC
The pin infrastructure bug is http://bugs.freedesktop.org/show_bug.cgi?id=51833
Comment 201 Matthias Clasen 2012-07-21 19:25:38 UTC
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
Comment 202 Giovanni Campagna 2012-08-03 14:51:01 UTC
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.
Comment 203 Giovanni Campagna 2012-08-14 11:31:04 UTC
Ping?
Can I get the watchdog reviewed, before this bug falls into the 3.8 land?
Comment 204 Jasper St. Pierre (not reading bugmail) 2012-08-14 15:57:05 UTC
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.
Comment 205 Giovanni Campagna 2012-08-14 16:00:01 UTC
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)...
Comment 206 Jasper St. Pierre (not reading bugmail) 2012-09-22 22:52:48 UTC
Any reason the PIN didn't go through?
Comment 207 Matthias Clasen 2013-01-02 13:06:40 UTC
Lets pick this up again for 3.8
Comment 208 Jasper St. Pierre (not reading bugmail) 2013-01-23 18:34:19 UTC
One more ping on PIN?
Comment 209 Matthias Clasen 2013-01-23 19:11:11 UTC
still blocked on http://bugs.freedesktop.org/show_bug.cgi?id=51833
Comment 210 Giovanni Campagna 2013-02-15 23:04:09 UTC
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.
Comment 211 Giovanni Campagna 2013-02-16 15:58:19 UTC
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.
Comment 212 Giovanni Campagna 2013-02-16 16:00:30 UTC
Created attachment 236372 [details]
Screencast of the new chooser
Comment 213 Giovanni Campagna 2013-02-16 16:12:42 UTC
Created attachment 236374 [details] [review]
UnlockDialog: Improve the look of the pin pad

Use bigger, square buttons, with larger text.
Comment 214 Giovanni Campagna 2013-02-16 16:14:45 UTC
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.
Comment 215 Matthias Clasen 2013-03-01 03:22:02 UTC
past feature freeze...didn't make it in...lets try again next time
Comment 216 Jasper St. Pierre (not reading bugmail) 2013-08-15 01:58:40 UTC
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.
Comment 217 Bastien Nocera 2014-09-01 04:24:39 UTC
Filed bug 735782 for the follow-up.