Created attachment 249840 [details] [review]
util: move common user verifier code into AuthPrompt

There's quite a bit of duplicated code between the login dialog
and the unlock dialog dealing with the various signals from the
ShellUserVerifier.

This commit moves that duplicated code into the AuthPrompt.

Created attachment 249984 [details] [review]
GdmUtil: separate AuthPrompt out into its own file

It's cleaner to have it in its own file than to cram it into
util.js, so this commit moves it.

Created attachment 249985 [details] [review]
AuthPrompt: move unlock and login user verifier code here

There's quite a bit of duplicated code between the login dialog
and the unlock dialog dealing with the various signals from the
ShellUserVerifier.

This commit moves that duplicated code into the AuthPrompt.

Review of attachment 249984 [details] [review]:

::: js/gdm/loginDialog.js
@@ +33,3 @@
 const St = imports.gi.St;
 
+const AuthPrompt = imports.gdm.authPrompt;

You only seem to import it. Did you mean to change a "GdmUtil." to "AuthPrompt." further down?

Review of attachment 249985 [details] [review]:

::: js/gdm/authPrompt.js
@@ +18,3 @@
 const DEFAULT_BUTTON_WELL_ANIMATION_TIME = 0.3;
 
+const AuthPromptMode = {

Yeah, this was the part I was concerned with. In my set of patches, I solved it with subclassing, but this is fine too.

@@ +412,3 @@
+        let hold = params.hold;
+        if (!hold)
+            hold = new Batch.Hold();

And here I was hoping we could get rid of "hold" entirely :(

Can you replace it with a signal instead?

::: js/gdm/loginDialog.js
@@ +450,3 @@
                                      y_fill: true });
 
+        this._authPrompt = new AuthPrompt.AuthPrompt(gdmClient, AuthPrompt.AuthPromptMode.UNLOCK_OR_LOGIN);

Ah, this is where you fix it :)

::: js/ui/unlockDialog.js
@@ +53,3 @@
         this._authPrompt.setUser(this._user);
         this._authPrompt.setPasswordChar('\u25cf');
         this._authPrompt.nextButton.label = _("Unlock");

I don't see where you replace the 'cancel' signal. Is it 'reset'? But that emits 'failed', rather than calling _escape, which does some extra stuff.

@@ +80,3 @@
         Main.ctrlAltTabManager.addGroup(this.actor, _("Unlock Window"), 'dialog-password-symbolic');
 
         this._idleMonitor = new GnomeDesktop.IdleMonitor();

Hm, I wonder if the idle monitor code should go to the auth prompt as well?

@@ +113,2 @@
         this.actor.destroy();
+        this._isModal = false;

Are you sure about this?

> @@ +412,3 @@
> +        let hold = params.hold;
> +        if (!hold)
> +            hold = new Batch.Hold();
> 
> And here I was hoping we could get rid of "hold" entirely :(
> 
> Can you replace it with a signal instead?
I actually started to replace it with a onBeginComplete callback earlier today, but it's a bit of a can of worms because the login dialog needs a hold on one side and ShellUserVerifier needs a hold on the other side so we'd have to fix both those, too.  Also, the shell user verifier usage of the hold is a bit nuanced since it relies on the hold's ability to have multiple refs inhibiting release (so we'd need a counter).  I decided since it's not straightforward and it's not strictly about deduplicating code it's a cleanup for another day (perhaps when we finally ditch batch.js entirely).

> ::: js/gdm/loginDialog.js
> @@ +450,3 @@
>                                       y_fill: true });
> 
> +        this._authPrompt = new AuthPrompt.AuthPrompt(gdmClient,
> AuthPrompt.AuthPromptMode.UNLOCK_OR_LOGIN);
> 
> Ah, this is where you fix it :)
right, i went in and fixed that earlier, but I guess i picked the wrong commit to fix it in.

> I don't see where you replace the 'cancel' signal. Is it 'reset'? But that
> emits 'failed', rather than calling _escape, which does some extra stuff.
in authPrompt.js all occurances of this.emit('cancel') get changed to this.cancel(); which does:
   
        this._userVerifier.cancel();
        this.reset(); which does some stuff and then emits 'reset' which causes this handler to get called:
        _onReset: function() {
              this.emit('failed');
         }

this._escape did this before:
         this._userVerifier.cancel();
         this.emit('failed');

So the only functional difference is what happens in the reset() function before 'reset' is emitted.  that code is just clearing the auth prompt back to a clean slate (which is more "right" but not strictly necessary since the screenShield will just destroy the unlockDialog on failure anyway)
> 
> @@ +80,3 @@
>          Main.ctrlAltTabManager.addGroup(this.actor, _("Unlock Window"),
> 'dialog-password-symbolic');
> 
>          this._idleMonitor = new GnomeDesktop.IdleMonitor();
> 
> Hm, I wonder if the idle monitor code should go to the auth prompt as well?
Well that would be a behavior change (screen shield would start showing up a lot more frequently at the login screen which currently never emits 'failed' to the screenShield).  not sure if we want that or not. if we do we should probably do it a separate commit and not just sneak it in here.

> 
> @@ +113,2 @@
>          this.actor.destroy();
> +        this._isModal = false;
> 
> Are you sure about this?
no, should be this.popModal()

Created attachment 249987 [details] [review]
GdmUtil: separate AuthPrompt out into its own file

It's cleaner to have it in its own file than to cram it into
util.js, so this commit moves it.

Created attachment 249988 [details] [review]
AuthPrompt: move unlock and login user verifier code here

There's quite a bit of duplicated code between the login dialog
and the unlock dialog dealing with the various signals from the
ShellUserVerifier.

This commit moves that duplicated code into the AuthPrompt.

Review of attachment 249987 [details] [review]:

OK.

Review of attachment 249988 [details] [review]:

OK.