ah, right.  We definitely should add that in.

But in gsettings, not gconf, right ?

Yes :-)

Occasionally, the gdm login is reverting to the gnome shell 3.0 login screen using the old graphic where you enter your name and password. I haven't been able to determine what causes this to happen.

Created attachment 199385 [details] [review]
data: add disable-user-list key

This commit adds a disable-user-list key that the greeter can read
to know to disable the user list.

Note, neither the shell greeter or the fallback greeter support
reading the key. That will happen in follow up commits.

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

As a temporary workaround, try the following:

su gdm -s /bin/sh -c "gconftool-2 --set --type boolean \
       /apps/gdm/simple-greeter/disable_user_list true" 

rm /etc/dconf/db/gdm.d/99-disable-user-list
cat <<EOF > /etc/dconf/db/gdm.d/99-disable-user-list
[org/gnome/login-screen]
disable-user-list=true

[org/gnome/desktop/session]
session-name='gdm-fallback'
EOF
dconf update

/etc/dconf/db/gdm.d/00-upstream-settings: [org/gnome/desktop/session]: session-name: ignoring duplicate definition of key /org/gnome/desktop/session/session-name

does that mean the higher setting (which is 99-) is taken?

Note, we don't support disable_user_list yet.  I just added the key on the gdm side so we would have the prep work in place to add it on the gnome-shell side.

That error message is interesting. Let me ask Ryan about it.

(In reply to comment #8)
> does that mean the higher setting (which is 99-) is taken?
It did for me :-)

(In reply to comment #9)
> Note, we don't support disable_user_list yet.  I just added the key on the gdm
> side so we would have the prep work in place to add it on the gnome-shell side.
I know, thats the reason for gdm-fallback, which brings in the old gdm-login which supports disable_user_list. Ugly, but it works for me.

> That error message is interesting. Let me ask Ryan about it.
The solution is a tweak, so don't bother, it saves my system from scanning 100+ NFS directories.

I know that. It is just a question about if keys that are already set in a db directory can be overwritten by keys in files with higher starting number.

So I talked to Ryan about this today.

He wants me to file a bug about the error messages, which I'll do shortly and point out here.

Another point that came up was that there's also now the start of movement to make systems functional with empty /etc, so we may need to change the way we handle certain aspects of configuration and lockdown in gdm.  Right now if you delete 00-upstream-defaults from /etc/dconf/db/gdm.d you'll end up with a somewhat broken system and that conflicts with the goal he has of empty /etc being safe and functional.

Okay I've filed Bug 664288 to cover the message noise.

Now that I think of it, the lines 

[org/gnome/login-screen]
disable-user-list=true

has nothing to do with the kludge working, since the fallback gdm still uses gconf, but my error definately had a nice side-effect :-)

Hi Linuxers, I note with when the video don't have support for gnome-shell, the command to disable user list on GDM works perfect, but, when the video have support, other GDM is used and don't works, show user list normally.

Created attachment 202366 [details] [review]
Very hackish javascript attack at disabling user list

Somebody that understands what goes on should take a look :-)

Review of attachment 202366 [details] [review]:

Hey thank you very much for the patch.  I appreciate that you're taking an interest in this bug and helping out.

I think the right approach is to first think of what kind of transition we want the user to see.  So if we're just initializing we want to avoid loading the user list at all if it's disabled and go straight to Username:.  If we're already loaded, we probably want to do some sort of fade out or shrink animation, and then crossfade to the Username: prompt, or maybe make the Not Listed? button become front and center, then blink and then jump to the Username: prompt.

Going the other direction is interesting because if it's sitting at Username: and the key gets changed, we probably want to jump back tot he user list, but if the user is in the middle of typing we probably don't.

Right now, we have PAM do the asking for Username: in the Not Listed? case.  We may want to change things up so the greeter does that question on its own before handing off to PAM.

::: /usr/share/gnome-shell/js/gdm/loginDialog.js.orig
@@ +286,3 @@
 UserList.prototype = {
+    _init: function(disable_user_list) {
+	this.disable_user_list = disable_user_list;

I know it's a little confusing, but javascript member variables should be camelCase.  The only things that are underscored are things coming from introspection (gobject properties, C functions, etc).

@@ +797,2 @@
         this._settings = new Gio.Settings({ schema: _LOGIN_SCREEN_SCHEMA });
+        this._disable_user_list = this._settings.get_boolean(_DISABLE_USER_LIST);

Should probably try to handle change notification, so if the value changes while the user list is up it disappears.

@@ +819,3 @@
                                  x_fill: true,
                                  y_fill: false });
+        this._userList = new UserList(this._disable_user_list);

It's a little strange to have a class called UserList that takes an argument called "disable_user_list".  I think we probably just want to hide the user list actor entirely if it's disabled.

@@ +904,3 @@
+        if (this._disable_user_list) {
+            this._onReset();
+        };

I'm not sure calling onReset directly is a good idea.  That means multiple conversations of the same type are going to get started in the daemon, etc. It kind of drives the state machine in the wrong way.

(In reply to comment #17)
> Review of attachment 202366 [details] [review]:
> 
> Hey thank you very much for the patch.  I appreciate that you're taking an
> interest in this bug and helping out.
> 
> I think the right approach is to first think of what kind of transition we want
> the user to see.  
Chicken and egg problem, I want to deploy on as many machines as possible to weed out 
other bugs, but in principle you are right.

> So if we're just initializing we want to avoid loading the
> user list at all if it's disabled and go straight to Username:.  If we're
> already loaded, we probably want to do some sort of fade out or shrink
> animation, and then crossfade to the Username: prompt, or maybe make the Not
> Listed? button become front and center, then blink and then jump to the
> Username: prompt.
> 
> Going the other direction is interesting because if it's sitting at Username:
> and the key gets changed, we probably want to jump back tot he user list, but
> if the user is in the middle of typing we probably don't.
> 
> Right now, we have PAM do the asking for Username: in the Not Listed? case.  We
> may want to change things up so the greeter does that question on its own
> before handing off to PAM.
OK, here is my wishlist (which probably affects accountmanager as well):

* Possibility to turn off reading peoples .face files (don't want to wait
  for NFS timeouts when net is disconnected).
* Possibility to turn of most (hiding all as done in my is OK but not optimal) 
  accounts in userlist (this can be done with hide/show attributes or some 
  filtering JavaScript function). 
* Possibility to have the 'Username' field below the user list (avoids mode change
  problem above, but might give some users 'Urrgh, need to use keyboard' reflexes) 

My use case is 5000+ account system with some public lab/test accounts that would be 
nice to have in the user list.

> I know it's a little confusing, but javascript member variables should be
> camelCase.  The only things that are underscored are things coming from
> introspection (gobject properties, C functions, etc).
Noted, will even try to remember.


> I'm not sure calling onReset directly is a good idea.  That means multiple
> conversations of the same type are going to get started in the daemon, etc. It
> kind of drives the state machine in the wrong way.
I said hackish :-)

BTW: Is there a simple way to test the login screen javascript in a separate window, testing on the live login screen is a pain.

not really.  User-switching or two computers with ssh are the best ways to work on gdm.

Saw https://fedoraproject.org/wiki/Features/Gnome_shell_software_rendering, what chances would you give that and Xnest/Xvnc to get a working test environment?

once that's fully available, you'd probably be able to enable XDMCP then do Xephyr -query localhost for testing

OK, will try to keep up to date on that.

Comments on my wish list?

> * Possibility to turn off reading peoples .face files (don't want to wait
   for NFS timeouts when net is disconnected).
Accounts service primarily reads face from /var now.  We could probably make it do that only or so.

> * Possibility to turn of most (hiding all as done in my is OK but not optimal) 
  accounts in userlist (this can be done with hide/show attributes or some 
  filtering JavaScript function). 
There's a bug to support Exclude/Include, though note, accountsservice only shows users in /etc/passwd and users who have logged in before.  So as long as you're using nis/ldap/etc and not copying passwd files around it should scale fine.

> * Possibility to have the 'Username' field below the user list (avoids mode
change
  problem above, but might give some users 'Urrgh, need to use keyboard'
reflexes)
there's a bug about adding type-ahead.  I think that's better than showing an entry always.

(In reply to comment #24)
> > * Possibility to turn off reading peoples .face files (don't want to wait
>    for NFS timeouts when net is disconnected).
> Accounts service primarily reads face from /var now.  We could probably make it
> do that only or so.
Or add a flag when calling it? Generally most new GUI things behaves very badly with 
NFS (especially when logged in on multiple machines), so perhaps I need to write some 
clever fuse-fs to filter/cache them on a machine to machine basis. Still need to prevent
reading 100+ .face files from NFS.

> > * Possibility to turn of most (hiding all as done in my is OK but not optimal) 
>   accounts in userlist (this can be done with hide/show attributes or some 
>   filtering JavaScript function). 
> There's a bug to support Exclude/Include, though note, accountsservice only
> shows users in /etc/passwd and users who have logged in before.  So as long as
> you're using nis/ldap/etc and not copying passwd files around it should scale
> fine.
Just abandoning NIS for our staff due to it's bad scalability (it eats reserved TCP ports), 
LDAP is used for student accounts, where I have had to make the LDAP lookup conditional depending 
on network status (https://bugs.freedesktop.org/show_bug.cgi?id=42631). Probably should find somwhere
to send that module to :-)
But main point is that the list of people that has logged in before tends to get fairly long 
(3 new users each day during lab weeks).
 
> > * Possibility to have the 'Username' field below the user list (avoids mode
> change
>   problem above, but might give some users 'Urrgh, need to use keyboard'
> reflexes)
> there's a bug about adding type-ahead.  I think that's better than showing an
> entry always.
Would type-ahead lighting it up (a'la gnome-shell type-to-search) be OK?

BTW: should new dconf keys reside in accountmanager, gdm or gnome-shell? The distinction between 
these parts is not very obvious :-(

Has there been any progress on this?  I noticed that the bug is still "UNCONFIRMED".

(In reply to comment #26)
> Has there been any progress on this?  I noticed that the bug is still
> "UNCONFIRMED".

We don't confirm bugs, so a bug status of UNCONFIRMED really doesn't mean anything.

Created attachment 209056 [details] [review]
Patch 1/3

A series of patches to enable org.gnome.login-screen:disable-user-list.

Created attachment 209057 [details] [review]
Patch 2/3

Created attachment 209058 [details] [review]
Patch 3/3

Review of attachment 209056 [details] [review]:

Hey thanks for looking into this!

::: js/gdm/loginDialog.js
@@ +305,3 @@
     },
 
+    _loadUserList: function() {

Probably should just call this "_load" since it's now a method on the UserList object (the words UserList are redundant)

@@ +342,3 @@
+        let batch = new Batch.ConsecutiveBatch(this, tasks);
+        return batch.run();
+    },

I like how you pumoved show/shrink/hide into the userlist.  it makes the object feel more opaque and functional.  It feels maybe that sould be a separate, earlier commit from the user manager changes, though.

@@ +344,3 @@
+    },
+
+    shrinkTo: function(userName) {

should be called shrinkToUser

@@ +388,3 @@
+    },
+
+    is_logged_in: function(userName) {

should be called userIsLoggedIn

@@ +967,3 @@
 
+        Mainloop.idle_add(Lang.bind(this, function() {
+            this.emit('loaded');

you're unconditionally emitting loaded here but should only emit loaded after the user list (user manager) is loaded

Review of attachment 209057 [details] [review]:

Looks reasonable.

Review of attachment 209058 [details] [review]:

::: js/gdm/loginDialog.js
@@ +849,2 @@
         this._greeterClient.call_start_conversation(_PASSWORD_SERVICE_NAME);
+        this._greeterClientReady = new Batch.Hold();

Should probably be called _passwordServiceHold or something like that

@@ +880,3 @@
                                Lang.bind(this, this._updateLogo));
+        this._settings.connect('changed::' + _DISABLE_USER_LIST_KEY,
+                               Lang.bind(this, this._updateDisabelUserList));

Disable is spelled wrong

@@ +884,1 @@
 

and here

@@ +925,1 @@
+            let tasks = [this._greeterClientReady,

So one idea... instead of putting the hold here directly, why not defer calling this._greeterClient.call_start_conversation(_PASSWORD_SERVICE_NAME) until here?.  Then the batch would be something like:

start_conversation("gdm-password"),
wait_for_conversation_ready("gdm-password"),
begin_verification("gdm-password")

And it's more clear what the linear progression is.

@@ +928,3 @@
+                             this._greeterClient.call_begin_verification(_PASSWORD_SERVICE_NAME);
+                             Mainloop.idle_add(Lang.bind(this, function() {
+                                 this.emit('loaded');

If the idea is to hold off opening the dialog until the password conversation is going, you should probably emit loaded after the first response from the conversation comes in instead of right when you start the conversation.  You don't need idle_add here. That's only used directly in the constructor to prevent loaded from getting emitted before the caller has an opportunity to connect to the signal. This code is in the middle of a Batch, so you know the mainloop has had a chance to run.

@@ +940,3 @@
+    
+            Mainloop.idle_add(Lang.bind(this, function() {
+                this.emit('loaded');

again, can't emit loaded until the user list is loaded.

@@ +1088,3 @@
+
+    _onReady: function(client, serviceName) {
+        this._greeterClientReady.release();

should only call release if serviceName is _PASSWORD_SERVICE_NAME

@@ +1247,2 @@
                      function() {
+                         this._sessionList.close();

separate fix, that should be a separate commit, right?

Review of attachment 209058 [details] [review]:

::: js/gdm/loginDialog.js
@@ +1084,3 @@
 
+    _updateDisabelUserList: function() {
+        this._disableUserList = this._settings.get_boolean(_DISABLE_USER_LIST_KEY);

We should do something more here. If the machine is sitting idle at the user list and the admin deploys a puppet config change to /etc/dconf/db/gdm.d/01-site-overrides to force the user list off, then when that file hits disk, we should shrink down, hide the user list and fade in the password prompt.

Could we please include the above patches without the additional watch functionality and perhaps add that later?

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

is there a package update that includes those patchs ?
are those patchs working !? beacause it doesn't seems to me .

I applied patches from comment 28, 29 , 30 :
[root@d012-06 gnome-shell]# patch -p1 < /root/0001-Moved-UserManager-handling-into-UserList.patch 
[root@d012-06 gnome-shell]# patch -p1 < /root/0002-Added-_userListBox-around-userlist.patch 
[root@d012-06 gnome-shell]# patch -p1 < /root/0003-Add-GdmGreeterClient-ready-callback-to-keep-track-if.patch 

then configured either in gconf.xml.mandatory :

[root@d012-06 gnome-shell]# gconftool-2 --direct --config-source xml:readwrite:/etc/gconf/gconf.xml.mandatory --type bool --set "/apps/gdm/simple-greeter/disable_user_list" "true"


or in gconf.xml.defaults:

[root@d012-06 gconf.xml.defaults]# gconftool-2 --direct --config-source xml:readwrite:/etc/gconf/gconf.xml.defaults --type bool --set /apps/gdm/simple-greeter/disable_user_list true

No way, user list still appears :-( 

in our school, where hundreds of users connect via ldap auth + nfs it is very annoying to have that long list a previous logged-in users .

Did I applied the patch correctly ?, did I missed something in applying the gconf variable ? 
after killall gdm-binary or even reboot, the list of users still appears .

Thanks for your help.

PS: I run with gdm-3.4.1-3.fc17.i686

GNOME 3.4 uses gsettings.

I would love to use gsettings , but I don't know what is the name of the gdm key associated with disable-user-list, I supose it's something like:

gsettings  set org.xxx.xxx.... disable-user-list true

How can I find the key name (xxx) ?

And do I have to apply the patches, all of them sequentially ? or does my gdm-3.4.1-3.fc17.i686 is ready to go with gsetting as is (whithout patching) ?

Thanks .

(In reply to comment #39)
> I would love to use gsettings , but I don't know what is the name of the gdm
> key associated with disable-user-list, I supose it's something like:
> 
> gsettings  set org.xxx.xxx.... disable-user-list true

$ gsettings org.gnome.login-screen disable-user-list true

> How can I find the key name (xxx) ?

I'm quite sure we have lockdown docs somewhere, but I just checked in the source:

http://git.gnome.org/browse/gdm/tree/data/org.gnome.login-screen.gschema.xml.in

> And do I have to apply the patches, all of them sequentially ? or does my
> gdm-3.4.1-3.fc17.i686 is ready to go with gsetting as is (whithout patching) ?

GDM is ready to go.(In reply to comment #39)

Wait, no it's not. You'll still need to apply the patches here, I think.

Grrr, I cannot set the key , a warning prevent me to set it apparently:

first read it:
[root@d012-06 ~]# gsettings get org.gnome.login-screen disable-user-list
false

Set it:
[root@d012-06 ~]# gsettings set org.gnome.login-screen disable-user-list true

** (process:2044): WARNING **: Command line `dbus-launch --autolaunch=aeb21b81943f83000967014f00000009 --binary-syntax --close-stderr' exited with non-zero exit status 1: Autolaunch error: X11 initialization failed.\n

** (process:2044): WARNING **: Command line `dbus-launch --autolaunch=aeb21b81943f83000967014f00000009 --binary-syntax --close-stderr' exited with non-zero exit status 1: Autolaunch error: X11 initialization failed.\n

it is still set to false !? 
[root@d012-06 ~]# gsettings get org.gnome.login-screen disable-user-list
false

Any idea of what is wrong in that procedure ?

Nope, this is what I have used with good result (after applying patches)

#!/bin/sh
cat <<EOF > /etc/dconf/db/gdm.d/99-disable-user-list
[org/gnome/login-screen]
disable-user-list=true
EOF
dconf update

without the patches I tried:

$ yum install gsettings-desktop-schemas-devel.i686

$ dbus-launch gsettings set org.gnome.login-screen disable-user-list true

$ gsettings get org.gnome.login-screen disable-user-list
true

$ reboot

-----

The user list is still here :(

(In reply to comment #44)
> without the patches I tried:
> 
> $ yum install gsettings-desktop-schemas-devel.i686
> 
> $ dbus-launch gsettings set org.gnome.login-screen disable-user-list true
> 
> $ gsettings get org.gnome.login-screen disable-user-list
> true

Yes, you are changing the setting for your user account, which is not the user gdm runs as (probably 'gdm'). See comment #43 for a proper way to change the setting, but please note that bugzilla is not a support forum - if you continue to have problems, better take it to a mailing list (gnome-shell-list is not really appropriate, but still better than bugzilla) or drop by on IRC (#gnome would be a good channel there).

Marius's Patch does no longer work with GDM 3.5.91

we should try to get these in to 3.6.1, if possible, i think.

(In reply to comment #47)
> we should try to get these in to 3.6.1, if possible, i think.

Creating a list of 3.6.1 candidates is on my list :-)

Ray, were you going to look at this for 3.6.1 ?

yea

I missed the boat on this one, will have to get it in master and 3.6.2 (if there is a 3.6.2) I guess.

(In reply to comment #51)
> I missed the boat on this one, will have to get it in master and 3.6.2 (if
> there is a 3.6.2) I guess.

Any updates here? I'm indeed planning to do a 3.6.2 release in the not-too-distant future ...

will look at it either today or tomorrow.

Thanks! That's far less distant in the future than any release plans :-)

make that monday !

(In reply to comment #55)
> make that monday !

Of which week?

(Joking of course, have a nice weekend!)

@Ray is there a patchset available for testing? We are in need of this functionality in our environment.

no. i worked on it today, but haven't finished, so there's nothing testable yet.

(progress is here but it's not ready yet http://git.gnome.org/browse/gnome-shell/log/?h=wip/disable-user-list )

Created attachment 227757 [details] [review]
loginDialog: drop spurious parameter

_onNotListed had an unusued, incorrect parameter.

This commit drops it.

Created attachment 227758 [details] [review]
loginDialog: don't rely on PAM to ask for a username

For the "Not Listed?" case we'll ultimately want to
identify when the user has entered their username.

Right now, we pass "null" in for an initial username,
and let the PAM conversation machinery ask the user.

This commit changes the "Not Listed?" code to ask the
user their username up front, before starting the PAM
conversation (in much the same way we do if the user
picks a user from the user list).

Created attachment 227759 [details] [review]
loginDialog: hide session list until username is entered

Right now when a user clicks "Not Listed?" they end up
seeing a session list that gets reset after they enter their
username.

This commit hides the session list until the username has
been entered.

Created attachment 227760 [details] [review]
loginDialog: support disable-user-list key

In some deployments showing a user list at the login
screen is undesirable.

GDM's fallback login screen has a configuration key:

org.gnome.login-screen disable-user-list false

that causes the user-list to get hidden.

This commit adds similar functionality to the normal,
shell-based login screen.

Based on a series of patches by Marius Rieder.

Review of attachment 227757 [details] [review]:

Uh, a bit overkill for a patch, but sure.

Review of attachment 227758 [details] [review]:

More of a "Why?" in the commit message would be appreciated. What's the benefit of starting the PAM conversation later, and how / why does it relate to the lack of user list?

Code unreviewed, will comment when you answer.

Review of attachment 227759 [details] [review]:

Is this a workaround for the session list being reset, or something greater? Could we just fix the session list being reset?

Review of attachment 227759 [details] [review]:

Is this a workaround for the session list being reset, or something greater? Could we just fix the session list being reset?

Review of attachment 227760 [details] [review]:

::: js/gdm/loginDialog.js
@@ +710,3 @@
                               { y_fill: false,
                                 y_align: St.Align.START });
+        this._titleLabel.hide();

visible: false in the construction props, please.

@@ +805,3 @@
+        // If this is the first time around, set initial focus
+        if (this._disableUserList == undefined && disableUserList)
+            this.setInitialKeyFocus(this._promptEntry);

This will always trigger in _loadUserList as far as I'm aware, so I'd prefer the code there.

@@ +807,3 @@
+            this.setInitialKeyFocus(this._promptEntry);
+
+        if (disableUserList != this._disableUserList) {

GSettings already does this check as far as I'm aware, so it's not necessary -- you can simply call _reset globally, and stop storing the variable here.

@@ +811,3 @@
+
+            if (!this._verifyingUser)
+              this._reset();

Indentation is wrong.

::: js/gdm/util.js
@@ +25,2 @@
 const LOGO_KEY = 'logo';
+const DISABLE_USER_LIST_KEY = 'disable-user-list';

I mentioned this in another bug, but I'd appreciate if we stop doing this. This is clearly a style holdover from C, where we're even doing "changed::" + DISBALE_USER_LIST_KEY. I'd prefer of we stopped this nonsense.

(In reply to comment #67)
> Review of attachment 227759 [details] [review]:
> 
> Is this a workaround for the session list being reset, or something greater?
> Could we just fix the session list being reset?

We only want to ask for the session once. And since we are asking for it with the password when coming from the user list, we want to do the same in the case where the username is manually entered.

(In reply to comment #65)
> Review of attachment 227758 [details] [review]:
> 
> More of a "Why?" in the commit message would be appreciated. What's the benefit
> of starting the PAM conversation later, and how / why does it relate to the
> lack of user list?
> 
> Code unreviewed, will comment when you answer.

how about

loginDialog: don't rely on PAM to ask for a username

For the "Not Listed?" case we'll ultimately want to
identify when the user has entered their username.

One reason for needing to know this, is so we don't
show the session list too early, before the user can
reliably pick a session.

It will also be important for implementing a
disable-user-list configuration key (so we know whether
or not we can jump back to the user list immediately if the
config key gets toggled off at runtime)

Right now, we pass null in for an initial username,
and let the PAM conversation machinery ask the user,
which means we have no good way of knowing when the
username is entered.

This commit changes the "Not Listed?" code to ask the
user their username up front, before starting the PAM
conversation (in much the same way we do if the user
picks a user from the user list).

(In reply to comment #68)
> Review of attachment 227760 [details] [review]:
> 
> ::: js/gdm/loginDialog.js
> @@ +710,3 @@
>                                { y_fill: false,
>                                  y_align: St.Align.START });
> +        this._titleLabel.hide();
> 
> visible: false in the construction props, please.
Sure, makes sense.

> 
> @@ +805,3 @@
> +        // If this is the first time around, set initial focus
> +        if (this._disableUserList == undefined && disableUserList)
> +            this.setInitialKeyFocus(this._promptEntry);
> 
> This will always trigger in _loadUserList as far as I'm aware, so I'd prefer
> the code there.
Well, we can't really put it there unless we call  this._settings.get_boolean(GdmUtil.DISABLE_USER_LIST_KEY) a second time, which we could do, but I was explicitly avoiding.  I don't feel strongly about this, so if you do let me know, and I'll change it.

> @@ +807,3 @@
> +            this.setInitialKeyFocus(this._promptEntry);
> +
> +        if (disableUserList != this._disableUserList) {
> 
> GSettings already does this check as far as I'm aware, so it's not necessary 

--
> you can simply call _reset globally, and stop storing the variable here.
Oh, so you're suggesting everywhere we do 

if (this._disableUserList)

we should just be doing

if (this._settings.get_boolean(...))

instead?

> 
> @@ +811,3 @@
> +
> +            if (!this._verifyingUser)
> +              this._reset();
> 
> Indentation is wrong.
ah thanks.

> 
> ::: js/gdm/util.js
> @@ +25,2 @@
>  const LOGO_KEY = 'logo';
> +const DISABLE_USER_LIST_KEY = 'disable-user-list';
> 
> I mentioned this in another bug, but I'd appreciate if we stop doing this. This
> is clearly a style holdover from C, where we're even doing "changed::" +
> DISBALE_USER_LIST_KEY. I'd prefer of we stopped this nonsense.
concretely, what are you suggesting instead?

If we're going to change conventions, we should change it across the board first, and then jump to the new convention instead of having a mix-mash of styles, I think.

(In reply to comment #67)
> Review of attachment 227759 [details] [review]:
> 
> Is this a workaround for the session list being reset, or something greater?
> Could we just fix the session list being reset?

I don't think it really makes sense to show the session list before a username is picked, since we'd have to update it to the users last session after a username is picked, but avoid changing it if the user modified this time around (or something).  It's messy and we should avoid doing it, I think. I don't think there's really any advantage to having the session list ahead time, just a bug.

Comment on attachment 227757 [details] [review]
loginDialog: drop spurious parameter

Attachment 227757 [details] pushed as aef9b73 - loginDialog: drop spurious parameter

Can we wrap this up ?

(In reply to comment #70)
> (In reply to comment #65)
> > Review of attachment 227758 [details] [review] [details]:
> > 
> > More of a "Why?" in the commit message would be appreciated. What's the benefit
> > of starting the PAM conversation later, and how / why does it relate to the
> > lack of user list?
> > 
> > Code unreviewed, will comment when you answer.
> 
> how about
> 
> loginDialog: don't rely on PAM to ask for a username
> 
> For the "Not Listed?" case we'll ultimately want to
> identify when the user has entered their username.
> 
> One reason for needing to know this, is so we don't
> show the session list too early, before the user can
> reliably pick a session.
> 
> It will also be important for implementing a
> disable-user-list configuration key (so we know whether
> or not we can jump back to the user list immediately if the
> config key gets toggled off at runtime)
> 
> Right now, we pass null in for an initial username,
> and let the PAM conversation machinery ask the user,
> which means we have no good way of knowing when the
> username is entered.
> 
> This commit changes the "Not Listed?" code to ask the
> user their username up front, before starting the PAM
> conversation (in much the same way we do if the user
> picks a user from the user list).

Yes, that's a much better message, thanks. ACN with that.

(In reply to comment #72)
> I don't think it really makes sense to show the session list before a username
> is picked, since we'd have to update it to the users last session after a
> username is picked, but avoid changing it if the user modified this time around
> (or something).  It's messy and we should avoid doing it, I think. I don't
> think there's really any advantage to having the session list ahead time, just
> a bug.

ACN, then.

(In reply to comment #71)
> > you can simply call _reset globally, and stop storing the variable here.
> Oh, so you're suggesting everywhere we do 
> 
> if (this._disableUserList)
> 
> we should just be doing
> 
> if (this._settings.get_boolean(...))

Yes. We can always change it back if it comes on our profiles or something, but as far as I'm aware GSettings is already plenty optimized.

alright, i'll keep it the way it is for now, and we can have a flag day one day and flip to the new style.

Attachment 227758 [details] pushed as 1ae0fad - loginDialog: don't rely on PAM to ask for a username
Attachment 227759 [details] pushed as cac9d12 - loginDialog: hide session list until username is entered
Attachment 227760 [details] pushed as 87e8770 - loginDialog: support disable-user-list key

Thanks for getting this into 3.6.2.

Even if:

[org/gnome/login-screen]
disable-user-list=true

I have now to click on 'Other' in fallback mode, before I am able to enter a username.