GNOME Bugzilla – Bug 600914
Add back "Last" to session/language/layout selection
Last modified: 2010-06-21 18:26:12 UTC
Created attachment 147071 [details] [review] patch adding this feature The old GDM provided a "Last" selection choice in the Session and Language selection dialogs. This design solved two problems: 1) There are problems caused if the display manager accesses the user's $HOME directory before pam_setcred is called. For example, this can cause problems when automounting a user's $HOME directory if encryption is being used. Touching the user's $HOME directory before pam_setcred can cause the automount to fail to be set up properly. 2) In environments where the user's $HOME directory is shared across multiple machines, the "Last" choice ensures that the user's default settings are always used regardless of which machine they are using. With the new GDM, issue #1 has been more elegantly solved by saving the user's configuration settings to a cache in /var/cache/gdm after pam_setcred. Then, after the first login the user's settings are maintained in the cache. In the common use case where GDM is running on a single machine, this solution works well. However, issue #2 is not well solved by this solution. If the user has a shared $HOME directory that is used across multiple machines, the default values stored in each machine's cache can be different. In an environment where users tend to switch computers and also tend to switch the session or language choice with some frequency, this can cause GDM to seem like it randomly picks different default settings when the user switches machines. This can be a more serious problem with some thin client solutions. For example, with Sun Ray, you can set up a pool of servers that will manage a cluster of thin clients. When the thin client connects to the server, it is routed to the server via a load balancing algorithm. So, the user can end up connecting to a different server managing their display even if they log out and log back into the same terminal. The cache becoming inconsistent in such an environment is obviously even more confusing to end users. It can seem like GDM randomly picks different session and language choices even when using the same terminal. Since the user is not supposed to be aware that they are really connecting to different servers with different caches, users would likely not understand the odd behavior. A sysadmin could perhaps work around this sort of problem by cross-mounting /var/cache/gdm across the various server machines, but this is an ugly hack to expect sysadmins to figure out and deal with. So, I propose that we add back a configuration option in the custom.conf file called "ShowLast" (similar to the ShowLastSession configuration option in the old GDM except it applies to not just the session, but also the language and layout selection areas). The default value is "false" by default since the use-case for this option is clearly a corner case. Most users would not want this behavior. However, as I explain above, there are some real environments where the "Last" behavior provided by the old GDM is truly more desirable, and this is typically in the more sophisticated network environments you might find in large corporate or academic environments where using shared $HOME directories across multiple machines or using thin clients is more common. This patch modifies the code so that if ShowLast is true that it avoids actually loading the user's preferences on username entry and instead sets the session/language/layout to a special "Last" key. After pam_setcred, the gdm_session_settings_load function is called again with "handle_last" set to TRUE, causing the user's real values to be read (unless the user has manually selected an choice, in which case the user's selection is honored). This works similarly to the way the "Last" option worked in the old GDM. I think it would be good for this patch to go upstream. The code changes to support this are not very intrusive, and it does make GDM more useful in some use cases. As I say above, I think the default value should be "false" since it is obviously nicer to show the user's settings in the GUI directly in the more typical environments where this works well (e.g. in environments where the user accounts are only accessed on a single machine). I would also appreciate any code review comments, or any suggestions on how to improve this patch in any way.
Created attachment 147276 [details] [review] Update to apply to current master
Review of attachment 147276 [details] [review]: ::: common/gdm-settings-keys.h @@ +33,3 @@ #define GDM_KEY_TIMED_LOGIN_USER "daemon/TimedLogin" #define GDM_KEY_TIMED_LOGIN_DELAY "daemon/TimedLoginDelay" +#define GDM_KEY_SHOW_LAST "greeter/ShowLast" I wonder if this should be in gconf instead. We don't have a greeter section in the config file anymore, and all other greeter configuration is via gconf, right? ::: daemon/gdm-session-direct.c @@ +1037,3 @@ dbus_message_unref (reply); + /* If setting to GDM_LAST_LANGUAGE, then just set default */ Maybe /* If the user picked "Last" in the language selector process the change, but don't update the saved default language, since it's not a real language */ @@ +1043,3 @@ + goto out; + } + instead of having a goto out here, might be better to make the next if an "else if", that way we can more easily see that the chunks of code are switching on the language name. @@ +1046,3 @@ if (strcmp (language_name, get_default_language_name (session)) != 0) { g_free (session->priv->saved_language); and then add above here, /* Update the saved default language, so that subsequent requests for it will reflect the user's choice */ @@ +1083,3 @@ + goto out; + } + Same comments here as above for language @@ +1123,3 @@ + goto out; + } + and session ::: daemon/gdm-session-worker.c @@ +604,3 @@ + + gdm_settings_direct_get_boolean (GDM_KEY_SHOW_LAST, &show_last); + if (show_last == TRUE && handle_last == FALSE) { Is it ever possible for handle_last to be TRUE and show_last is FALSE?
Created attachment 148093 [details] [review] updated patch This patch updates gdm-session-direct as you suggest, adding comments and getting rid of the usage of "goto out". In our discussions on IRC, we agreed that it does make sense for this to be configured in the custom.conf file rather than the "gdm" user's GConf configuration since this is really a configuration option that the daemon would want any greeter to manage. The need to manage how the dmrc file is handled in terms of PAM is really a matter of how the daemon needs to be configured. Regarding your comment: > Is it ever possible for handle_last to be TRUE and show_last is FALSE? handle_last can only be TRUE if show_last is TRUE. It is necessary to test for both because when this function is called the first time, it needs to set the values to the GDM_LAST_SESSION/GDM_LAST_LANGUAGE/GDM_LAST_LAYOUT values. After pam_setcred, if the values still contain these keys, then the function gets called again with handle_last set to TRUE so it actually does the load from the dmrc file. Any further comments?
Comment on attachment 148093 [details] [review] updated patch This isn't something we want. The problem is a real one but this isn't the right solution. Ideally we'd have a sort of directory service that can provide information like this.