> Should use the system level theme for the background - same as in the message tray (with the texture)

I did this in the wip/message-tray branch

http://git.gnome.org/browse/gnome-shell/commit/?h=wip/message-tray&id=47f347b92eb4afec74de8db54b8d4a2f3b686a8e

requires patches from https://bugzilla.gnome.org/show_bug.cgi?id=680801

Created attachment 220256 [details] [review]
ScreenShield: lower the autodrag threshold to 10%

The threshold is there just to ensure that the user is opening
the curtain on purpose. Requiring 40% is too much for that.

Created attachment 220266 [details] [review]
ScreenShield: blur and desaturate the screenshield background

The background is the same as the normal desktop, so we blur and
desaturate it to clearly show that it's not the normal system state.

Includes a noticeable slowdown due to GLSL shading and FBO redirection.

Review of attachment 220266 [details] [review]:

Sure.

Created attachment 220269 [details] [review]
theme: Use the noise texture for the background of the screen shield

Review of attachment 220269 [details] [review]:

And clearly so!

Comment on attachment 220269 [details] [review]
theme: Use the noise texture for the background of the screen shield

Attachment 220269 [details] pushed as ed991f7 - theme: Use the noise texture for the background of the screen shield

Created attachment 220273 [details] [review]
UnlockDialog: allow going back to the lock screen by pressing escape

Review of attachment 220273 [details] [review]:

Sure.

Created attachment 220274 [details] [review]
ScreenShield: when explicitly locking, tween the shield from the top

The curtain should appear to be an overlay on top of the system,
and since it is lifted by dragging up, it makes sense to slide it
down on lock.

Comment on attachment 220266 [details] [review]
ScreenShield: blur and desaturate the screenshield background

Togheter with the new animation, this lags a lot here. (Intel GM45, with Core 2 Duo CPU).

Needs a better implementation.

Comment on attachment 220273 [details] [review]
UnlockDialog: allow going back to the lock screen by pressing escape

Attachment 220273 [details] pushed as 413ace2 - UnlockDialog: allow going back to the lock screen by pressing escape

Created attachment 220287 [details] [review]
ScreenShield: handle Escape on the stage, not on the lock screen

This allows to press esc to unlock even if the focus is not on
the lock screen (for example after dismissing the auth failure
notification).

The focus shouldn't be on the stage in that case. When the message tray pops up, it stores the current focus, and then tries to go back to it once done.

Created attachment 220290 [details] [review]
ScreenShield: bump the lock screen slightly when pressing a key

When pressing a key different from escape (one thus that has no
effect), bump the screen up, to indicate that it eats keyboard
input and it must be lifted up.

Review of attachment 220290 [details] [review]:

::: js/ui/screenShield.js
@@ +442,3 @@
+                           transition: 'easeOutQuad',
+                           onComplete: function() {
+                               Tweener.addTween(this,

Ugh. Have you considered writing a custom curve?

(In reply to comment #16)
> Review of attachment 220290 [details] [review]:
> 
> ::: js/ui/screenShield.js
> @@ +442,3 @@
> +                           transition: 'easeOutQuad',
> +                           onComplete: function() {
> +                               Tweener.addTween(this,
> 
> Ugh. Have you considered writing a custom curve?

Yes. But apparently tweener will set the property to the final value at the end of the transition "just in case", and so the curtain stays up.

Created attachment 220296 [details] [review]
ScreenShield: constrain vertical movement of the screen lock

Don't allow dragging the screen lock above and below the screen,
as in particular dragging below breaks the illusion of the curtain.

Created attachment 220298 [details] [review]
ScreenShield: update the displayed title when the source changes

Notifications in the lock screen need to watch signals on source
and react accordingly.
At the same time, don't allow the appname provided by the notifications
protocol to override the app name obtained via the app system (if
one is present).
This fixes the "XChat-GNOME OSD" problem.

Review of attachment 220298 [details] [review]:

This should be two separate patches.

Created attachment 220453 [details] [review]
NotificationDaemon: don't override the title for app notifications

If a source is associated with an app, ignore the app name provided
by libnotify, as that is often garbage.
This fixes the "XChat-GNOME OSD" problem.

Created attachment 220454 [details] [review]
ScreenShield: update the displayed title when the source changes

Notifications in the lock screen need to watch signals on source
and react accordingly.

Created attachment 220461 [details] [review]
Only show new notifications in the screen shield

Rework the count system in Source, to distinguish between the
normal notification count and the count of unseen/unacknowledged
notification. (A notification is considered unacknowledged until
shown, as a banner or inside the summary box pointer).
Includes some code cleanups and a test for multiple notifications
in the same source.

This is on top of bug 680426 but not wip/message-tray. Rebasing is
however trivial.

Created attachment 220466 [details] [review]
Only show new notifications in the screen shield

Rework the count system in Source, to distinguish between the
normal notification count and the count of unseen/unacknowledged
notification. (A notification is considered unacknowledged until
shown, as a banner or inside the summary box pointer).
Includes some code cleanups and a test for multiple notifications
in the same source.

Created attachment 220476 [details] [review]
ScreenSheild: make notification view scrollable

Place a maximum height on the notification view, and show scrollbars
if the list of persistent sources would overflow.

Created attachment 220485 [details] [review]
ScreenShield: fix spacing of notifications and sources

Reduce padding around persistent sources, and ensure that spacing
around resident notifications is only applied once.
Also, add some padding to the clock.

Review of attachment 220453 [details] [review]:

Yes.

Review of attachment 220454 [details] [review]:

OK.

Review of attachment 220256 [details] [review]:

Sure.

Review of attachment 220476 [details] [review]:

::: data/theme/gnome-shell.css
@@ +2232,3 @@
     background-color: rgba(0.0, 0.0, 0.0, 0.9);
     border: 2px solid #868686;
+    max-height: 500px;

...

At some point I wish we had something like CSS does, like:

   "top: 20px; bottom: 20px"

and it constraints its height to that of the parent container.

(Don't take this as a cue to start implementing this, though, this is fine.)

::: js/ui/screenShield.js
@@ +82,3 @@
         this.actor = new St.BoxLayout({ vertical: true,
+                                       name: 'screenShieldNotifications',
+                                       style_class: 'screen-shield-notifications-box' });

Indentation issues?

Review of attachment 220485 [details] [review]:

Sure, looks fine.

::: js/ui/messageTray.js
@@ -1308,3 @@
         this.actor.child = this._sourceBox;
 
-        this.notificationStackView = new St.ScrollView({ name: source.isChat ? '' : 'summary-notification-stack-scrollview',

This is really weird for me. We need to re-do this at some point.

Review of attachment 220274 [details] [review]:

Better than what we had, but the animation still doesn't feel right - sliding the shield from the top is good, but the lock "background" just magically appears; we probably want a fade there (though we currently use the shield's parent, needs design input I guess)

::: js/ui/screenShield.js
@@ +32,3 @@
 // SHORT_FADE_TIME is used when requesting lock explicitly from the user menu
 const STANDARD_FADE_TIME = 10;
+const SHORT_FADE_TIME = 0.8;

Kinda unrelated, but OK

Review of attachment 220287 [details] [review]:

Minor comment, otherwise looks good

::: js/ui/screenShield.js
@@ +495,3 @@
         this._arrow.show();
 
+        if (!this._stageKeyHandler)

I'd prefer an explicit
  this._stageKeyHandler = 0;
in _init()

Review of attachment 220476 [details] [review]:

Minor nit: ScreenSheild in the commit message

Review of attachment 220287 [details] [review]:

As I said before, this is wrong. The message tray should restore focus to what was there before. We need to investigate that.

Attachment 220256 [details] pushed as a29ceaa - ScreenShield: lower the autodrag threshold to 10%
Attachment 220453 [details] pushed as 1e37969 - NotificationDaemon: don't override the title for app notifications
Attachment 220454 [details] pushed as 59dfb58 - ScreenShield: update the displayed title when the source changes
Attachment 220476 [details] pushed as 59cd9b4 - ScreenSheild: make notification view scrollable
Attachment 220485 [details] pushed as 8d7fa54 - ScreenShield: fix spacing of notifications and sources

Review of attachment 220296 [details] [review]:

Looks good, but depends on pending clutter changes - in the meanwhile, https://bugzilla.gnome.org/show_bug.cgi?id=681168#c6 is probably a good idea

(In reply to comment #37)
> Review of attachment 220296 [details] [review]:
> 
> Looks good, but depends on pending clutter changes - in the meanwhile,
> https://bugzilla.gnome.org/show_bug.cgi?id=681168#c6 is probably a good idea

Do you want it for 3.5.5?
Otherwise, I can fix the clutter patch before 3.5.90 / API freeze.

Review of attachment 220466 [details] [review]:

::: js/ui/messageTray.js
@@ +950,3 @@
 
+    acknowledge: function() {
+        this.acknowledged = true;

Please set acknowledged explicitly to false in _init()

@@ +951,3 @@
+    acknowledge: function() {
+        this.acknowledged = true;
+        this.emit('acknowledged');

Unused signal

@@ +1165,3 @@
+
+    countUpdated: function() {
+        this.emit('count-changed');

Nit: mismatch of function and signal name is a bit odd

@@ +1224,2 @@
     notify: function(notification) {
+        notification.acknowledged = false;

Mmmh,
  notification.acknowledged = false;
vs.
  notification.acknowledge();

Pick one

@@ +1843,3 @@
                 this._reNotifyAfterHideNotification = notification;
+            } else {
+                // The summary box pointer is showing or shown.

... or hidden?

@@ +2547,3 @@
+                    let sameSource = this._summaryBoxPointerItem.source == notification.source;
+                    if (sameSource)
+                        notification.acknowledge();

Ugh - not a fan of filter functions with side effects

::: js/ui/screenShield.js
@@ +128,3 @@
     },
 
+    _makeNotificationCountText: function(source, count) {

Not clear why you add the count parameter instead of doing
  let count = source.unseenCount;

If you want the additional parameter,
  function(count, isChat)
makes more sense to me

::: js/ui/telepathyClient.js
@@ +640,1 @@
     _updateCount: function() {

I suspect some subtle bugs here, as all previous calls to _updateCount() in messageTray.js are now effectively ignored. Can't you just do

get count() {
    return this._pendingMessages.length;
},

get unseenCount() {
    return this.count;
}?

::: tests/Makefile.am
@@ +16,3 @@
 	interactive/icons.js			\
 	interactive/inline-style.js		\
+	interactive/notify-ack.js		\

libnotify has a rather extensive test suite - if it's missing something important, better to add the test there imo

(In reply to comment #38)
> > Looks good, but depends on pending clutter changes - in the meanwhile,
> > https://bugzilla.gnome.org/show_bug.cgi?id=681168#c6 is probably a good idea
> 
> Do you want it for 3.5.5?

I guess we can do without (can't say I care deeply about unstable tarballs ;-)

(In reply to comment #32)
> Review of attachment 220274 [details] [review]:
> 
> Better than what we had, but the animation still doesn't feel right - sliding
> the shield from the top is good, but the lock "background" just magically
> appears; we probably want a fade there (though we currently use the shield's
> parent, needs design input I guess)
> 
> ::: js/ui/screenShield.js
> @@ +32,3 @@
>  // SHORT_FADE_TIME is used when requesting lock explicitly from the user menu
>  const STANDARD_FADE_TIME = 10;
> +const SHORT_FADE_TIME = 0.8;
> 
> Kinda unrelated, but OK

Initially, I had the whole lock screen tween from the top (including the background). The problem with that is the watchdog, which is spawned before locking and draws a full black window.

(In reply to comment #39)
> Review of attachment 220466 [details] [review]:
> 
> @@ +1165,3 @@
> +
> +    countUpdated: function() {
> +        this.emit('count-changed');
> 
> Nit: mismatch of function and signal name is a bit odd

It is intentionally similar to icon-changed/iconUpdated in bug 680426

> @@ +1224,2 @@
>      notify: function(notification) {
> +        notification.acknowledged = false;
> 
> Mmmh,
>   notification.acknowledged = false;
> vs.
>   notification.acknowledge();
> 
> Pick one
> 
> @@ +1843,3 @@
>                  this._reNotifyAfterHideNotification = notification;
> +            } else {
> +                // The summary box pointer is showing or shown.
> 
> ... or hidden?

If hidden, this._summaryBoxPointerItem is cleared.

> 
> ::: js/ui/screenShield.js
> @@ +128,3 @@
>      },
> 
> +    _makeNotificationCountText: function(source, count) {
> 
> Not clear why you add the count parameter instead of doing
>   let count = source.unseenCount;

In the non telepathy case, source.unseenCount is potentially slow (O(N)), so this is an optimization.

(In reply to comment #41)
> Initially, I had the whole lock screen tween from the top (including the
> background). 

That doesn't sound right either - IMHO the "correct" animation would fade in the background while sliding in the shield from the top.


> The problem with that is the watchdog, which is spawned before
> locking and draws a full black window.

Right, we would need to not show the watchdog window (in the compositor that is, we obviously need to open/map the window on the server). In any case, all those are perfectly fine as future improvements ...


(In reply to comment #42)
> > Nit: mismatch of function and signal name is a bit odd
> 
> It is intentionally similar to icon-changed/iconUpdated in bug 680426


You mean the last patch? That one uses icon-updated/iconUpdated, which is perfectly fine. (If you are referring to another patch, chances are I'd consider that odd as well ;-)


> > ... or hidden?
> 
> If hidden, this._summaryBoxPointerItem is cleared.

... which is not immediately obvious, so a comment would be appropriate IMO



> In the non telepathy case, source.unseenCount is potentially slow (O(N)), so
> this is an optimization.

You mean because you use it as well to update actor.visible? Frankly, I suspect that a large number of notifications becomes a UI problem long before affecting performance (iterating over 100 notifications shouldn't be too bad, having a scrolled list of 100 notifications crammed together in a boxpointer otoh ...)

Still, as mentioned before, I'm fine with the alternative of replacing the source parameter with source.isChat

(In reply to comment #43)
> (In reply to comment #42)
> > > Nit: mismatch of function and signal name is a bit odd
> > 
> > It is intentionally similar to icon-changed/iconUpdated in bug 680426
> 
> 
> You mean the last patch? That one uses icon-updated/iconUpdated, which is
> perfectly fine. (If you are referring to another patch, chances are I'd
> consider that odd as well ;-)

It was originally icon-changed, to match count-changed. I changed it because I thought the function consistency was better than the signal consistency. We should probably fix it to be count-updated as well.

Created attachment 220549 [details] [review]
MessageTray: fix restoring focus when the notification is destroyed

- Emit Notification::destroy before actually destroying the actor, so
  that MessageTray (through FocusGrabber) has a chance to restore
  focus before Clutter assigns it to the stage
- Don't reenter a transient animation state if the final state is
  already reached, otherwise the state machine gets confused and
  destroyed notifications are not cleaned up properly.

This is a second attempt to solve the esc problem. I'm a lot less confident
in this, since the message tray state machine has a lot of edge cases.

Note that I thought we already had some mechanism for unseen vs. seen notifications that was used in some tracking somewhere.

Created attachment 220555 [details] [review]
ScreenShield: when explicitly locking, tween the shield from the top

The curtain should appear to be an overlay on top of the system,
and since it is lifted by dragging up, it makes sense to slide it
down on lock.

Created attachment 220556 [details] [review]
Only show new notifications in the screen shield

Rework the count system in Source, to distinguish between the
normal notification count and the count of unseen/unacknowledged
notification. (A notification is considered unacknowledged until
shown, as a banner or inside the summary box pointer).
Includes some code cleanups and a test for multiple notifications
in the same source.

Created attachment 220572 [details] [review]
Login/UnlockDialog: don't reset immediately if auth fails

Instead of showing a notification, add a small message immediately
below the entry, and give the user two more attempts to login,
before going back to the welcome or lock screen.

Created attachment 220573 [details] [review]
UnlockDialog: Move "Login as another user" at the bottom

The Unlock button is more logically related to the entry than
this link, and having the latter in the middle breaks the workflow.

Created attachment 220575 [details] [review]
Unlock: place the prompt above the entry, rather than on the side

As indicated in the mockups

Review of attachment 220556 [details] [review]:

I still think the notification test belongs into libnotify (or just be kept private); I can see how the timeouts are useful for testing unseen notifications on the screen shield, but a month or so from now on the test will just appear completely arbitrary. Other than that looks good!

Created attachment 220576 [details] [review]
ScreenShield: prepare the dialog when the user starts dragging

This way if the user drags long enough it will see the dialog
below the curtain, and it will appear as it was always there.

Comment on attachment 220556 [details] [review]
Only show new notifications in the screen shield

Attachment 220556 [details] pushed as aa733b5 - Only show new notifications in the screen shield
Pushed without the test. I still have it locally, but I don't think it belongs in
libnotify (it's not testing the protocol, just the very special policies of gnome-shell)

Review of attachment 220555 [details] [review]:

::: js/ui/screenShield.js
@@ +305,3 @@
         this._lockScreenGroup.add_action(action);
 
+        this._lockDialogGroup = new Shell.GenericContainer({ name: 'lockDialogGroup' });

So are we still not confident enough to inherit from StWidget? Not to mention that there *must* be a nicer way to enforce the lock dialog's size/position ...

Review of attachment 220576 [details] [review]:

Looks reasonable except for one assumed typo (disclaimer: I didn't actually test the patch)

::: js/ui/screenShield.js
@@ +482,3 @@
                                time: time,
                                transition: 'linear',
+                               onComplete: function() { this.hide(); }

this._lockScreenGroup.hide() I assume?

@@ -524,3 @@
     _resetLockScreen: function(animate) {
         this._lockScreenGroup.show();
-        this._arrow.show();

_arrow.show()/hide() not necessary?

(In reply to comment #56)
> Review of attachment 220576 [details] [review]:
> 
> Looks reasonable except for one assumed typo (disclaimer: I didn't actually
> test the patch)
> 
> ::: js/ui/screenShield.js
> @@ +482,3 @@
>                                 time: time,
>                                 transition: 'linear',
> +                               onComplete: function() { this.hide(); }
> 
> this._lockScreenGroup.hide() I assume?

No, this.hide: absent onCompleteScope, the function is invoked with the tweened actor as this.

> @@ -524,3 @@
>      _resetLockScreen: function(animate) {
>          this._lockScreenGroup.show();
> -        this._arrow.show();
> 
> _arrow.show()/hide() not necessary?

No, we're hiding its container anyway. It was probably a left over from when arrow was a sibling of the two groups.

Review of attachment 220573 [details] [review]:

I'd prefer a separate patch for making _dialogLayout public, otherwise good

Review of attachment 220575 [details] [review]:

Sure

Review of attachment 220572 [details] [review]:

Looks reasonable at first sight, but needs testing - currently the patch assumes the PIN changes

(In reply to comment #55)
> Review of attachment 220555 [details] [review]:
> 
> ::: js/ui/screenShield.js
> @@ +305,3 @@
>          this._lockScreenGroup.add_action(action);
> 
> +        this._lockDialogGroup = new Shell.GenericContainer({ name:
> 'lockDialogGroup' });
> 
> So are we still not confident enough to inherit from StWidget? Not to mention
> that there *must* be a nicer way to enforce the lock dialog's size/position ...

I don't think there is, short of making my own layout manager that acts like BinLayout but respects fixed position.

Attachment 220573 [details] pushed as 03db0d6 - UnlockDialog: Move "Login as another user" at the bottom

Created attachment 220590 [details] [review]
ScreenShield: when explicitly locking, tween the shield from the top

The curtain should appear to be an overlay on top of the system,
and since it is lifted by dragging up, it makes sense to slide it
down on lock.

Using StWidget now.

Created attachment 220591 [details] [review]
Login/UnlockDialog: don't reset immediately if auth fails

Instead of showing a notification, add a small message immediately
below the entry, and give the user two more attempts to login,
before going back to the welcome or lock screen.

Rebased to avoid the PIN dependency. Tested locally, both the login
and unlock paths seem to work, although the login part could use some
testing in a real system, rather than faking it inside the session.

Review of attachment 220590 [details] [review]:

Yeah, I like that better (still time to switch back to GenericContainer if necessary)

Review of attachment 220591 [details] [review]:

I generally like how this works, but let's keep this out of the release for now:

::: js/gdm/loginDialog.js
@@ +696,3 @@
         this._userVerifier = new GdmUtil.ShellUserVerifier(this._greeterClient);
         this._userVerifier.connect('ask-question', Lang.bind(this, this._askQuestion));
+        this._userVerifier.connect('show-message', Lang.bind(this, this._showMessage));

Looks like we need to disconnect this at some point:

(lt-gnome-shell-real:26753): Clutter-CRITICAL **: clutter_text_get_text: assertion `CLUTTER_IS_TEXT (self)' failed

(lt-gnome-shell-real:26753): St-ERROR **: st_widget_get_theme_node called on the widget [0x5982820 StLabel.login-dialog-message-warning ("(null)")] which is not in the stage.
Shell killed with signal 5

Comment on attachment 220590 [details] [review]
ScreenShield: when explicitly locking, tween the shield from the top

Attachment 220590 [details] pushed as d7f5421 - ScreenShield: when explicitly locking, tween the shield from the top

Comment on attachment 220576 [details] [review]
ScreenShield: prepare the dialog when the user starts dragging

Comments were addressed, I believe.
Attachment 220576 [details] pushed as 2c627ca - ScreenShield: prepare the dialog when the user starts dragging

Created attachment 221032 [details] [review]
Login/UnlockDialog: don't reset immediately if auth fails

Instead of showing a notification, add a small message immediately
below the entry, and give the user two more attempts to login,
before going back to the welcome or lock screen.

Created attachment 221033 [details] [review]
ScreenShield: bump the lock screen slightly when pressing a key

When pressing a key different from escape (one thus that has no
effect), bump the screen up, to indicate that it eats keyboard
input and it must be lifted up.

Created attachment 221066 [details] [review]
ScreenShield: constrain vertical movement of the screen lock

Don't allow dragging the screen lock above and below the screen,
as in particular dragging below breaks the illusion of the curtain.

Rebased to account for comments on the clutter patch.

Created attachment 221067 [details] [review]
ScreenShield: animate going from the unlock dialog to the lock screen

If the user presses esc in the dialog, tween the lock screen from
the top instead of showing it immediately.

Created attachment 221127 [details] [review]
UserMenu: show a padlock icon in the screenshield

When the screen is locked, the IM presence is forced to idle (away), so
it's not helpful. Show a padlock instead, to indicate that password
or other authentication method is required.

(There is no way currently to know if a password is really required,
or pam would succeed right away, so the padlock is just always shown)

Review of attachment 221127 [details] [review]:

Not sure why you care about password in the commit message. I think a padlock indicates "locked", not "requires password". I don't know any padlocks that take a password.

::: js/ui/userMenu.js
@@ +677,3 @@
+        if (this._isLocked) {
+            // ignore presence changes when locked
+            this._iconBox.child = this._lockedIcon;

Why do you need to re-set the icon?

(In reply to comment #74)
> Review of attachment 221127 [details] [review]:
> 
> Not sure why you care about password in the commit message. I think a padlock
> indicates "locked", not "requires password". I don't know any padlocks that
> take a password.

From the wiki:
Show the lock icon when a password or other authentication method is in effect
And effectively, if there is no password, the screen is not really locked.

So what you're saying is that "in obscure conditions where some guy has pam_true has his auth stack, we'll show a padlock"?

And in the not so obscure conditions where some guy has chosen to log in without a password in the control center.

Comment on attachment 221127 [details] [review]
UserMenu: show a padlock icon in the screenshield

This is not what we want

Created attachment 221425 [details] [review]
Hide user menu and a11y menu in the screen lock

This is a temporary patch. The user menu button needs to move to
the left, and the a11y menu in some cases needs to be hidden outside
of the screen lock too.

Created attachment 221465 [details] [review]
UnlockDialog: remove hover effect from the avatar widget

It is not clickable, so it should not have any hover.

Review of attachment 221465 [details] [review]:

Sure.

Review of attachment 221425 [details] [review]:

This can work for now.

Created attachment 221468 [details] [review]
ScreenShield: Fix updating visibility of the notification box

If there were no persistent or resident notification sources,
nothing ever would call _updateVisibility, and the box would stay
there empty

Attachment 221425 [details] pushed as cbb56b6 - Hide user menu and a11y menu in the screen lock
Attachment 221465 [details] pushed as 80fde70 - UnlockDialog: remove hover effect from the avatar widget

Review of attachment 221468 [details] [review]:

::: js/ui/screenShield.js
@@ +124,3 @@
 
+        let children = this._persistentNotificationBox.get_children();
+        let yes = children.some(function(a) { return a.visible; });

um

@@ +125,3 @@
+        let children = this._persistentNotificationBox.get_children();
+        let yes = children.some(function(a) { return a.visible; });
+        log('_persistentNotificationBox.children.some.etc...: ' + yes);

No.

@@ +127,3 @@
+        log('_persistentNotificationBox.children.some.etc...: ' + yes);
+
+        this.actor.visible = yes;

Yes.

Created attachment 221473 [details] [review]
UnlockDialog: add an explicit cancel button

Pressing escape to cancelling the unlock procedure is not discoverable
enough.
At the same time, fix styling to avoid bluish hover effects.

Created attachment 221474 [details] [review]
ScreenShield: Fix updating visibility of the notification box

If there were no persistent or resident notification sources,
nothing ever would call _updateVisibility, and the box would stay
there empty

Review of attachment 221474 [details] [review]:

Sure.

Review of attachment 221473 [details] [review]:

Sure.

Review of attachment 221033 [details] [review]:

Sure.

Review of attachment 221066 [details] [review]:

Sure.

Created attachment 221475 [details] [review]
ScreenShield: hide Removable devices in the lock screen

Showing the removable devices is potentially a security risk (as
they include network shares). Also, a nautilus launched from there
can't be used, so it's just a way to overload the system.

Attachment 221033 [details] pushed as d0a9476 - ScreenShield: bump the lock screen slightly when pressing a key
Attachment 221066 [details] pushed as b53d18f - ScreenShield: constrain vertical movement of the screen lock
Attachment 221468 [details] pushed as f8b1a84 - ScreenShield: Fix updating visibility of the notification box
Attachment 221473 [details] pushed as fd04c59 - UnlockDialog: add an explicit cancel button
Attachment 221474 [details] pushed as f8b1a84 - ScreenShield: Fix updating visibility of the notification box

Created attachment 221524 [details] [review]
ScreenShield: reduce curtain sliding time to 0.5 seconds

Current time is too long, 0.5 was proved to be more effective
in user testing.

Review of attachment 221524 [details] [review]:

Easy enough.

Comment on attachment 221524 [details] [review]
ScreenShield: reduce curtain sliding time to 0.5 seconds

Attachment 221524 [details] pushed as a5f2d28 - ScreenShield: reduce curtain sliding time to 0.5 seconds

Created attachment 222070 [details] [review]
Login/UnlockDialog: don't reset immediately if auth fails

Instead of showing a notification, add a small message immediately
below the entry, and give the user two more attempts to login,
before going back to the welcome or lock screen.

Rebased to current master (after the gdm login hint).
Needs fast review, because it has UI impact.

Comment on attachment 220549 [details] [review]
MessageTray: fix restoring focus when the notification is destroyed

Apparently, the original bug fixed itself in the message tray rework, and this might be the cause of bug 682533

Let's try and wrap this bug up and file separate reports for the outstanding issues. I've added most of the uncommitted patches to bug 682536, bug 682544 and bug 682545.

After that, the only patches that remain are:

 * ScreenShield: handle Escape on the stage, not on the lock screen

 * MessageTray: fix restoring focus when the notification is destroyed

Comment on attachment 221475 [details] [review]
ScreenShield: hide Removable devices in the lock screen

Attachment 221475 [details] pushed as bec4849 - ScreenShield: hide Removable devices in the lock screen

Some of the remaining patches are tracked in other bugs (bug 682536 for blur and desaturate, bug 682545 for escape animation, bug 682544 for failed login attempts), while the message tray stuff is unnecessary.
What about FIXED? :)

(In reply to comment #101)
> What about FIXED? :)

The screen lock has been IMPROVED. :)

Comment on attachment 221067 [details] [review]
ScreenShield: animate going from the unlock dialog to the lock screen

Attachment 221067 [details] pushed as 107f5de - ScreenShield: animate going from the unlock dialog to the lock screen