*** Bug 682543 has been marked as a duplicate of this bug. ***

Created attachment 222475 [details] [review]
ScreenShield: move lock status handling to a three state enumeration

Have distinct states for the lock screen and the unlock dialog, and rework
the logic in ScreenShield for state changes, to minimize unnecessary
signal emissions.
Adapt the various components to the new API, and in particular show a11y
and keyboard in the unlock dialog but not in the lock screen.

Review of attachment 222475 [details] [review]:

We're getting into session mode territory, here. Can we do something with that?

(In reply to comment #3)
> Review of attachment 222475 [details] [review]:
> 
> We're getting into session mode territory, here. Can we do something with that?

I think ScreenShield state is somehow orthogonal to session mode: State.FULL_SESSION is defined to be "whatever the session mode says", and all mode need some way to go to State.LOCK_SCREEN, although only normal mode ever goes to State.UNLOCK_DIALOG.

Created attachment 222479 [details] [review]
ScreenShield: move lock status handling to a three state enumeration

Have distinct states for the lock screen and the unlock dialog, and rework
the logic in ScreenShield for state changes, to minimize unnecessary
signal emissions.
Adapt the various components to the new API, and in particular show a11y
and keyboard in the unlock dialog but not in the lock screen.

Rebased on top of bug 682540.

Created attachment 223459 [details] [review]
ScreenShield: move lock status handling to use session mode

Have distinct session modes for the lock screen and the unlock dialog,
and rework the logic in ScreenShield for mode changes, to ensure we can
compute the current mode and the new mode at each transition.

Rebased on top of bug 683156. It's a much smaller patch now.

Note this is needed to avoid regressing the gdm greeter, as current code calls lock() in showDialog(), and thus pushes the 'lock-screen' mode (which is more restricted than gdm).
This patch has the right state machine.

Created attachment 223658 [details] [review]
ScreenShield: move lock status handling to use session mode

Have distinct session modes for the lock screen and the unlock dialog,
and rework the logic in ScreenShield for mode changes, to ensure we can
compute the current mode and the new mode at each transition.

Small fix required for bug 683060 to work.

Review of attachment 223658 [details] [review]:

::: js/ui/screenShield.js
@@ -651,3 @@
-        this._lockDialogGroup.scale_x = 1;
-        this._lockDialogGroup.scale_y = 1;
-

bad rebase?

@@ +703,3 @@
+            Main.sessionMode.pushMode('lock-screen');
+        else
+        // logind and doesn't know about gnome-shell internal state)

why not just put lock-screen on top of unlock-dialog?

::: js/ui/sessionMode.js
@@ +57,3 @@
+    'unlock-dialog': {
+        isLocked: true,
+        isGreeter: undefined,

true? false?

Boolean values don't have a third state.

@@ +58,3 @@
+        isLocked: true,
+        isGreeter: undefined,
+        unlockDialog: undefined,

This looks really silly and weird.

@@ +124,3 @@
     },
 
+    switchMode: function(to) {

Sort of the point of having a stack like this is to prevent shenanigans like this. Any reason you can't do a pop and push?

::: js/ui/shellDBus.js
@@ +344,3 @@
         this.parent();
 
+        Main.screenShield.connect('lock-status-changed', Lang.bind(this, function(shield, state) {

You don't use "state"?

To me, both the lock screen (without a11y) and the login screen (with a11y) look as expected in .91 - what is still missing here ?

That the unlock dialog should behave similar to the login screen, and have a11y and input source.

(In reply to comment #9)
> Review of attachment 223658 [details] [review]:
> 
> @@ +703,3 @@
> +            Main.sessionMode.pushMode('lock-screen');
> +        else
> +        // logind and doesn't know about gnome-shell internal state)
> 
> why not just put lock-screen on top of unlock-dialog?

Ok, let me try that.

> ::: js/ui/sessionMode.js
> @@ +57,3 @@
> +    'unlock-dialog': {
> +        isLocked: true,
> +        isGreeter: undefined,
> 
> true? false?
> 
> Boolean values don't have a third state.

Yes they have. All values have an extra state, which is "don't update the current value", although I may live without it.

> @@ +58,3 @@
> +        isLocked: true,
> +        isGreeter: undefined,
> +        unlockDialog: undefined,
> 
> This looks really silly and weird.

I could say that the session mode was not really designed to handle these cases, but I would be repeating myself...

> @@ +124,3 @@
>      },
> 
> +    switchMode: function(to) {
> 
> Sort of the point of having a stack like this is to prevent shenanigans like
> this. Any reason you can't do a pop and push?

I don't want to do useless work to enable and disable components back and forth (especially when extensions are involved, as enabling them is usually very expensive).

> ::: js/ui/shellDBus.js
> @@ +344,3 @@
>          this.parent();
> 
> +        Main.screenShield.connect('lock-status-changed', Lang.bind(this,
> function(shield, state) {
> 
> You don't use "state"?

Yeah, old stuff.

Created attachment 223889 [details] [review]
ScreenShield: move lock status handling to use session mode

Have distinct session modes for the lock screen and the unlock dialog,
and rework the logic in ScreenShield to have the lock-screen mode stack
onto the unlock-dialog mode (where applicable)

Review of attachment 223889 [details] [review]:

If you want to clean up the 'undefined' stuff, put this at the top of the file:

    const inherit = undefined;

That should make it a bit clearer. Do that in a separate commit, though.

Looks fine otherwise.

I don't think we need inherit, there is documentation for what undefined does. And I hope
we're not changing that sessionMode code soon.
Attachment 223889 [details] pushed as 8cf9baa - ScreenShield: move lock status handling to use session mode