Created attachment 271246 [details] [review]
loginDialog: Add missing return value

_loadUserList() may be used as idle handler, so we should explicitly
return a boolean.

Created attachment 271247 [details] [review]
loginDialog: Defer loading user list until needed

Loading the user list can be expensive, for instance when there is
a large number of users and/or their avatars have to be fetched over
the network. In case the user list is disabled anyway, there is no
point in doing that work just to hide it, so stop doing that.

foo

Created attachment 271248 [details] [review]
loginDialog: Defer loading user list until needed

Whoops, fix commit message

Review of attachment 271246 [details] [review]:

OK.

Review of attachment 271248 [details] [review]:

::: js/gdm/loginDialog.js
@@ +470,3 @@
         this._authPrompt.addActorToDefaultButtonWell(this._sessionMenuButton.actor);
 
+        this._disableUserList = undefined;

uh, what's the point of this?

@@ +477,3 @@
+        // focus later
+        Main.layoutManager.connect('startup-complete',
+                                   Lang.bind(this, this._updateDisableUserList));

I dislike that updateDisableUserList is the thing that "starts the user list". updateDisableUserList should simply update disableUserList. Although currently it also triggers the state machine again.

I've really never liked how the state machine here acts, held together with chicken wire and chewing gum, so sure, we can add a few more pieces of duct tape to it.

@@ +505,3 @@
+
+        if (!this._disableUserList)
+            this._queueLoadUserList();

Nothing will unqueue the load if we go from true-to-false while it's in-load, but perhaps that's OK.

Created attachment 271280 [details] [review]
loginDialog: Defer loading user list until needed

(In reply to comment #5)
> +        this._disableUserList = undefined;
> 
> uh, what's the point of this?

The initial value must not match the setting or the state machine is never kicked off correctly. I'm sure we agree that this is extremely ugly, but it's simply how the code works currently - all the patch does is making it explicit.


> I dislike that updateDisableUserList is the thing that "starts the user list".
> updateDisableUserList should simply update disableUserList.

Except that something obviously needs to trigger a UI update to adapt to the setting :-)

But yeah, loading the list can be done elsewhere, moved.


> I've really never liked how the state machine here acts, held together with
> chicken wire and chewing gum, so sure, we can add a few more pieces of duct
> tape to it.

I don't think the patch makes anything worse, and this is clearly not the time for refactoring, so *shrug*


> @@ +505,3 @@
> +        if (!this._disableUserList)
> +            this._queueLoadUserList();
> 
> Nothing will unqueue the load if we go from true-to-false while it's in-load,
> but perhaps that's OK.

You mean false-to-true, but yeah, I don't see much of a problem there. It'd leave us with the same situation we have now, e.g. the list is loaded despite being hidden. A bit inefficient, but then I wouldn't expect the setting to change at all at runtime ...

Review of attachment 271280 [details] [review]:

OK.

Attachment 271246 [details] pushed as ef8123e - loginDialog: Add missing return value
Attachment 271280 [details] pushed as 0ba05b2 - loginDialog: Defer loading user list until needed

( I actually started working on a large-scale gdm refactor today for the Wayland work, but I won't push it for 3.12: https://git.gnome.org/browse/gnome-shell/log/?h=wip/wayland-gdm-cleanup )