I'm using Ubuntu Karmic, latest Alpha with gdm 2.27.90
I managed to switch off the user list. Now when I start the machine or logout, I see a greeter with three buttons: Restart, Shut Down and Log In. I think this Log In button makes not much sense here. I would prefer the user name field to show up immediately. I suppose in 99% of cases I would click Log In. 

The Ubuntu version I am looking at is probably work in progress, but so far I prefer the interaction that gdm 2.20 offered.

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

Created attachment 151319 [details] [review]
No need to click Login button when user list is disabled

Created attachment 151452 [details] [review]
Better patch to fix this issue

Simulating the mouse click in Halton's patch sort of worked, but has a number of issues:

- The entry field doesn't have focus by default, so the user needs to click on 
  the entry field to enter their username.
- Removing the Cancel button is bad.  Users should have a way to get back to
  the username entry field if they realize that they mistyped their username.
- Emiting a signal on a button from within the GDM code is bad practice, and 
  not safe.

This patch takes more care to treat the face browser disabled case more elegantly and addresses the above issues.  It has some code to make the Cancel button not appear on the first PAM entry, and only starts showing it on the 2nd PAM entry.
It doesn't make sense to show the Cancel button on the first entry.

Review of attachment 151452 [details] [review]:

::: gdm-2.29.5/gui/simple-greeter/Makefile.am-orig
@@ +475,3 @@
+                } else {
+                        show_widget (login_window, "cancel-button", TRUE);
+                }

I think we should have an in_cancellable_state boolean or needs_cancel_button boolean that gets set to FALSE until an cancellable operation is initiated and then gets set to TRUE.  Then we'd call show_widget based on the boolean.

@@ +599,3 @@
+        return FALSE;
+}
+

This function needs a better name. Also it seems to overlap with on_login_button_clicked_start_other which should probably be removed at the same time this patch goes in (along with the START_OTHER button)

@@ +611,3 @@
+                g_idle_add ((GSourceFunc)gdm_no_user_list, login_window);
+        }
+

if you did this at the end of the function instead of the beginning can you get away from doing it on idle?

@@ +800,3 @@
+        }
+        login_window->priv->num_info_query++;
+

we don't care how many queries are happening.  We only care if we need to show a cancel button.  Drop the num_info_query stuff and replace it with the needs_cancel_button boolean mentioned above.

@@ +841,3 @@
+                show_widget (login_window, "cancel-button", TRUE);
+        }
+        login_window->priv->num_info_query++;

it's a little ugly to have this code duplicated (including duplicate comments).  This should be factored into it's own function if it's needed more than once.

@@ +1548,3 @@
+        error = NULL;
+        login_window->priv->user_list_disabled = gconf_client_get_bool (client,
+                KEY_DISABLE_USER_LIST, &error);

Checking this gconf key more than once in various parts of the code is wrong.  We should really check it once and bubble that state around internally.

@@ +819,3 @@
+        client = gconf_client_get_default ();
+        error = NULL;
+        result = gconf_client_get_bool (client, KEY_DISABLE_USER_LIST, &error);

This is a no-no.  We absolutely can't check user-list specific things in the generic chooser code.

@@ +827,3 @@
+        if (! result) {
+                _grab_focus (GTK_WIDGET (widget));
+        }

So I assume what's going on is you're doing something new that needs focus and this code is stealing it from you.

Instead, you should make sure the new thing you're doing happens after the grow is finished.

> I think we should have an in_cancellable_state boolean or needs_cancel_button
> boolean that gets set to FALSE until an cancellable operation is initiated and
> then gets set to TRUE.  Then we'd call show_widget based on the boolean.

The problem with using a boolean is that we really only need to call show_widget once.  On the 2nd PAM prompt.  If we use a boolean, then we would call show_widget for all subsequent PAM prompts needlessly.  The advantage of using a counter is it makes it easier to just call show_widget one time on the 2nd PAM prompt.

> This function needs a better name. Also it seems to overlap with
> on_login_button_clicked_start_other which should probably be removed at the
> same time this patch goes in (along with the START_OTHER button)

I don't understand.  We still want the "Other" button to be available when the Face Browser is being used don't we?  Do you have a suggestion for a better name than gdm_no_user_list()?

> +                KEY_DISABLE_USER_LIST, &error);
>
> Checking this gconf key more than once in various parts of the code is wrong. 
> We should really check it once and bubble that state around internally.

I notice that load_theme calls gdm_user_chooser_widget_new().  Would it make sense to pass it in as an argument to the constructor, or add a new setter function to call after construction or something else?

> This is a no-no.  We absolutely can't check user-list specific things in the
> generic chooser 
>
> Instead, you should make sure the new thing you're doing happens after the 
> grow is finished.code.

Isn't the "grow" and "animation" code in the chooser widget really only used by 
the GdmUserChooserWidget?  Are there any other widgets that use this code?

The problem is that, as far as I can tell, there is no code that gets called in gdm-greeter-login-window.c after the call to _grab_focus in on_grow_animation_complete().  On IRC you suggested using the on_users_loaded() function in gdm-greeter-login-window.c, but in my testing this happens before 
the on_grow_animation_complete() function is called, so that does not work.

So, I wonder how to do this.  Perhaps the loaded signal should be moved so that it happens after the grow completes, or a new signal is needed to indicate that the grow is done?

When the face browser is not being used, there is obviously no need to grow or animate the widget at all.  I think we basically need some flag to pass into gdm-user-chooser-widget to tell it not to bother even doing this work at all.  In my proposed patch, I am just making it avoid doing the problematic grab, but perhaps it makes more sense to deal with this earlier.  A better fix may be
to tell gdm-user-chooser-widget at an earlier point to not even start trying to do any animations or growing at all.  Do you have any suggestions on how to do this?

For example, perhaps the gdm_user_chooser_widget_constructor could take
an argument to tell it to avoid calling the load_users idle function at all.  Thus avoiding the work to load the widget since we aren't going to display it anyway.

Thoughts?

Created attachment 156026 [details]
debug output

debug log

Created attachment 156027 [details] [review]
updated patch


Here is an updated patch that resolves many of the issues that Ray highlighted and which Ray also highlighted in further discussion I had with him on IRC.

- Now a boolean is used for controlling when the cancel button becomes active.
- The code for on_login_button_clicked_start_other and the code relating to
  LOGIN_BUTTON_START_OTHER is removed since it is not needed after making this 
  change.
- I added a function gdm_greeter_show_cancel_button which is called by 
  gdm_greeter_login_window_info_query and 
  gdm_greeter_login_window_secret_info_query rather than duplicating the code
  in both functions.
- The gdm_greeter_login_window_ready function no longer uses an idle function
  since in testing I found it was not really necessary.

However, I still need help figuring out how to make the code not call _grab_focus in on_animation_complete in gui/simple-greeter/gdm-chooser-widget.c
when the face browser is disabled.  Ray suggests above that having user-chooser specific code in the general widget is a bad idea.

This means we should either figure out how to inform gdm-greeter-login-window when the animation is complete so the code to select the Other user can be moved so it happens afterwards, or modify the code to avoid doing the animation entirely when the face browser is disabled - since the user list widget is not shown anyway when the face browser is disabled we probably don't need to be doing animations anyway.  

Or we could just decide the current patch is okay, perhaps with the caveat that it might be better to modify it to pass the GConf setting into the widget rather than checking GConf again.  

You seem to suggest it is a problem for the gdm-chooser-widget to have code specific to the user-chooser widget.  However, this seems a bit odd to me since isn't the animation code in general specific to the user chooser widget?  If it is okay to have user-chooser animation specific code in the general widget, then why is it a problem for it to have extra code to check if the user list is disabled or not and avoid doing grabs when the widget won't be shown anyway?

I am guessing the code needs to be refactored to move the animation code into the user-chooser widget, so for now we could just allow a bit more user-chooser specific code to leak into the general widget and perhaps we can agree to fix the refactoring problem when there is time.

Thoughts?

So we talked about the animation issue before on irc:

   [15:57:56]  <halfline> yippi_: so instead of calling gdm_no_user_list
   [15:58:18]  <halfline> can't you just call
   gdm_user_chooser_widget_set_chosen_user_name (user_chooser,
   GDM_USER_CHOOSER_USER_OTHER) ?
   [15:59:10]  <yippi_> do you think that will avoid the grab problems?
   [15:59:14]  <halfline> that should make on_user_chosen get invoked after
   the animation is complete
   [15:59:29]  <halfline> and on_user_chosen will emit the BEGIN_VERIFICATION
   signal and do the switch_mode call
   [15:59:52]  <halfline> try it and tell me if it works
   [16:00:01]  <yippi_> okay, i'll try it.

Did you try that?  It would be good to only emit BEGIN_VERIFICATION in one code path.

Sorry, I tried using gdm_user_chooser_widget_set_chosen_user_name.  However, this did not work well.  I thought I added a comment about this, but I clearly did not.  The debug log in #7 is in relation to testing I did for this.  I added the debug log but neglected to explain what it is about.  :) 

Note that gdm_user_chooser_widget_set_chosen_user_name() calls gdm_chooser_widget_set_active_item() in gdm-chooser-widget.c   This calls activate_from_item_id().  However, the activate_from_item_id() function only works if the model is actually created, and it is not created when the user chooser is not constructed.  So because path == NULL it just returns.

To test further, I hacked the update_chooser_visibility fucntion in gdm-chooser-widget.c so that it always shows the widget.  This got me past the above issue
and got me to the gtk_tree_view_row_activated() call.  However this also added the User List with the "Other" choice above the PAM entry field, which is undesirable, but it allowed me to test this approach a bit further.

So, with this hack, I got past the above issue and got into the gtk_tree_view_row_activated() call.  This generates the "row_activated" signal which causes the on_row_activated() function to be called, which then calls gdm_chooser_widget_activate_selected_item().

So, the debug log in #7 shows how this works.

In the debug log you will notice that the "setting active item 'other'" message happens before "calling _grab in on_grow_animation_complete".  So, this approach does not cause the setting of the value to "Other" to happen before the final grab that we need to avoid to keep the focus on the entry field.

In short, the patch could probably be modified to work this way.  Though we would need to hack the code somehow to deal with the issue I describe above where activate_from_item_id() does not work if the user chooser is not visible on the screen.  I am not sure if it worth the bother to fix that bug since this approach still does not help to avoid the grab in on_grow_animation_complete().  As I say in Comment #8, we need to rework the code further if we want to avoid this, or decide the latest patch is okay.

Created attachment 157235 [details] [review]
patch fixing problem.


Here is a patch which I think is finally ready to go upstream.  It has only two
minor changes from the previous patch:

1) The code now checks if TimedLogin is enabled or not.  If the user list is
   disabled, but timed login is enabled, then it still shows the user list. 
   This is needed since TimedLogin uses the user list for its interface.  In
   my testing this works well and shows just the "Timed Login" user and the
   "Other" user.  If the user_list_disabled is set to true, then no other users
   are shown.  If user_list_disabled is set to false, then local users are shown.

2) The addition of this hunk to the load_theme function:

-        gtk_widget_show (login_window->priv->user_chooser);
+        if (!login_window->priv->user_list_disabled) {
+                gtk_widget_show (login_window->priv->user_chooser);
+        }

   This fixes the grab problem more nicely.  With this change, the _grab_focus
   function in gdm-chooser-widget.c just exist because the widget is not 
   realized.  It also makes the GUI not resize unnecessarily when the user list
   is not being shown, and avoids needless code since it isn't necessary to show
   the widget when it is not being used.

This works well in my testing when disable_user_list is true or false, and whether
timed login is true or false.  I tested all 4 combinations.

Also, just to be clear, this new patch does not modify gdm-chooser-widget.c at all since item #2 in the last comment fixes the grab issue more nicely.

Created attachment 159359 [details] [review]
updated patch

After another round of review with Ray Strode on IRC, I have updated the patch again and have made it cleaner.  This patch has the following differences.

1) It never enters SELECTION_MODE when the user list is disabled.
2) The show_cancel_button is initialized to FALSE and it gets set to true if
   SELECTION_MODE is entered or after the first time either in 
   gdm_greeter_login_window_info_query or 
   gdm_greeter_login_window_secret_info_query
   Overall this is much more simple than the way this flag was being handled 
   before.

So I think this is now ready for upstream.  Ray?

Created attachment 159367 [details] [review]
updated patch

Updated patch which adds a new initial mode MODE_UNDEFINED and sets the mode initially to this state.  Also, some comment cleanups and move some common code to the _show_cancel_button static function.

Created attachment 159376 [details] [review]
Updated patch

Patch with some minor fixes, and that applies to head.  The patch I had been sharing before was based on code that had other patches applied first, so the previous patches didn't apply cleanly.  This one does.  Cleaned up some formatting.

Fixed in master.

For the record, with 2.30.2 I still see "Log In", and not "Switch to" for users who already have a session, both in the face browser and in the "enter user name" cases.

Hey,

Mind filing that as a separate issue?  It's totally a valid bug, but this bug is already closed and fixed a different problem.

Sure, done in bug 617194. Thanks!

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