This still happens on current gdm 2.26.1. IMHO it shouldn't set GDM_KEYBOARD_LAYOUT at all. It contradicts to the language you have set in gdm, and right now there isn't even an (obvious?) keyboard layout selector in gdm at all.

If you don't set anything, the GNOME session should just use the very same default (from hal) than gdm itself.

Created attachment 137802 [details] [review]
Ubuntu patch

This is the patch we applied to Ubuntu to fix this. It might not be what you have in mind, but it fixes the immediate "OMG my keyboard is broken!" flood of bugs. :-)

Thanks, Martin

Created attachment 137918 [details] [review]
fixed patch

This fixes the previous patch to not return NULL, but an empty string, and to check this before setting $GDM_KEYBOARD_LAYOUT. Various assertion checks trip over this and cause gdm to crash.

The patch makes some sense to me; it is worth pointing out that we have a patch to make gdm fall back to hal device properties for finding the system layout. Here: 

http://cvs.fedoraproject.org/viewvc//devel/gdm/gdm-system-keyboard.patch?view=markup

GDM does have a keyboard layout selector.  It appears down in the panel when a user is selected next to language and session.

I don't think this patch is quite right because it means we're going to be calling SetLayoutName "" instead of SetLayoutName "us" on the greeter, which will have weird side effects.

Maybe instead of this patch we should just wrap the setenv call in this guard:

if (strcmp (get_layout_name (), get_default_layout_name ()) != 0) {
}

What do you think?

Right, sorry, we initially built gdm without; the selector is there now. One patch that helped a lot is "Correctly handle invalid xkb layout from ~/.dmrc" (966a59bf068 in git), I indeed had "Layout=" in .dmrc before.

However, I still think above patch would be a sensible safeguard.

Your proposed change sounds good to me. Thanks!

Should be set as of 4b177ed3c1998d9cd2f203f5c635b3f73ca4aecd.

Please note that your patch and my original one do different things: your's should essentially be a no-op (i. e. don't set $GDM_KEYBOARD_LAYOUT to the default), while mine prevented "us" to _override_ the default (which cuold be e. g. German). 

However, I think that the original problem was fixed in a better way by http://git.gnome.org/cgit/gdm/commit/?id=966a59bf0680152fe92f8f08d4db9b3df7a8eeec and proper libxklavier enabling, so I hope that the original issue won't happen any more.

Thank you!

I'm afraid this still isn't fixed. The original patch that I attached was a workaround, but it doesn't solve the keyboard layout picker displaying the wrong value, and it still sets the wrong keyboard layout by default.

The real fix is to have gdm actually know about the current layout itself, and thus default to that both for users without a layout setting in ~/.dmrc, as well as if you log in as "other".

Fedora has carried a patch for this for a while, which reads the current system default from hal:
http://cvs.fedoraproject.org/viewvc//devel/gdm/gdm-system-keyboard.patch?view=markup

This works very well, and finally restores keyboard layout sanity. Would you consider applying this upstream as well?

Created attachment 138799 [details] [review]
detect keyboard layout from hal

This is the Fedora patch, updated to apply to the current version.

(Also tested and applied to current Ubuntu Karmic)

Argh, and even that isn't enough :(, since it just swallows variants and options, such as "nodeadkeys" for "de". So it seems that either gdm needs to read/set in GDM_KEYBOARD_LAYOUT variants and options as well, or (probably easier) stop setting $GDM_KEYBOARD_LAYOUT if the user didn't change it in gdm.

The problem with variants and options is that we can't really add a fullblown keyboard layout capplet to the login screen. The idea with GDM_KEYBOARD_LAYOUT was always that it will be used to pick out the best-matching layout from the session configuration, while keeping variants options and other decorations that might be configured in the session.

Actually, the gdm keyboard layout selector does know about variants, and exports them in $GDM_KEYBOARD_LAYOUT (e. g. "de nodeadkeys"). There are two problems here:

 - gdm does not read variant from hal to select the default layout
 - gnome-settings-daemon does not take the variant from $GDM_KEYBOARD_LAYOUT into account.

I'll try and get this fixed properly. If it becomes too complex, a mitigation might be to not set $GDM_KEYBOARD_LAYOUT at all if the user didn't change the default, so that the system layout from hal is used instead of the (wrong) $GDM_KEYBOARD_LAYOUT.

See also bug 596897 for the g-s-d counterpart.

BTW, it seems that gdm now actually behaves according to the original request in bug 585290 ("try harder to use the keyboard layout passed by gdm")? It always writes a .dmrc and thus passes the layout to g-s-d, even if you never actually picked a different one in gdm.

> BTW, it seems that gdm now actually behaves according to the original request
in bug 585290

Please ignore that, that was just confusing.

Created attachment 144496 [details] [review]
detect keyboard layout and variant from hal

This improved patch also correctly detects the variant from hal (ugh, finding that it needs to be tab-separated took me a while..). With this patch and the corresponding one in g-s-d (bug 596897) variants now work well.

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

Created attachment 145935 [details] [review]
Patch for gdm-session-direct.c, gdm-session-settings.c

I'd like to revert the integration because the following regression is caused.

I think which layout is configure, is gnome-settings-daemon's task instead of GDM's one and I think it's better to always send $GDM_KEYBOARD_LAYOUT .
I think your problem is fixed with bug 585290.


The following scenario is the bug reproducing steps:

1. default system keyboard layout is jp from /etc/sysconfig/keyboard
2. GDM layout option Widget shows jp layout by default.
3. Users can add us layout from layout dialog.
4. Log in to GNOME session with us layout.
5. GDM worker sends GDM_KEYBOARD_LAYOUT="us"
6. GDM worker saves Layout=us in $HOME/.dmrc
7. gnome-settings-daemon gets "us" from GDM_KEYBOARD_LAYOUT and save it in
gconf value /desktop/gnome/peripherals/keyboard/kbd/layouts = [us]
   And update XKB layout with xkl_engine_lock_group().

8. Log out the GNOME session.
9. GDM layout option Widget shows us layout.
10. Log in to GNOME session with jp layout.

Then GDM worker does *not* send GDM_KEYBOARD_LAYOUT="jp"
Because get_layout_name (session) == "jp" from layout Widget and
        get_system_default_layout (session) == "jp" from
/etc/sysconfig/keyboard
and setup_session_environment() doesn't set GDM_KEYBOARD_LAYOUT without this patch.

11. GDM worker saves Layout=jp in $HOME/.dmrc
12. gnome-settings-daemon doesn't get $GDM_KEYBOARD_LAYOUT. It sets XKB layout
"us" from the gconf value.

So user choose "jp" layout but actual session assigns "us" layout.


To fix this problem, I think removing the if condition is good.
get_layout_name() returns the layout from GUI.
get_system_default_layout() returns the system layout.
get_default_layout_name() returns saved layout in .dmrc.

The default layout is retrieved from user's gconf value so the either get*()
doens't return the correct value.
I also think checking user's gconf is not good before gnome-settings-daemon is
launched.

The fix of gdm_session_settings_set_layout_name() means to call "layout-name"
notification in reading .dmrc only.
gdm_session_settings_set_layout_name() is also called when users choose the
layout from GUI.
If "layout-name" notification is called with users choice, the notification
calls a DBUS method and gdm-simple-slave updates
GdmSessionDirectPrivate.saved_layout as the result.


This fix GDM always can send $GDM_KEYBOARD_LAYOUT.

(In reply to comment #18)
> Created an attachment (id=145935) [details] [review]

The latest Fedora fixes my problem. Please ignore the previous comment.
Thanks.

Created attachment 147209 [details] [review]
detect default layout with xklavier

Instead of using HAL, this patch uses xklavier for detecting the system default layout. The code could be shared with gdm-layouts if GDK_DISPLAY() is the same as session->priv->display_id.

Created attachment 148577 [details] [review]
detect default layout with xklavier

Thanks Luca!

I fixed your patch to pass the correct argument to XDisplayOpen() (it needs a display name, not a gdm display ID), and also added the missing variant handling.

Working great now!

I don't think that is the right approach.

Matthias,

do you have some details why not? Something wrong with the concept/using xklavier/handling variants/etc?

Thanks,

Martin

Because it feel it is quite icky to open and close an X connection for this purpose and rely on the vagarities of how X initializes some root window properties. Imo, we should get the system setting directly from where it is stored (ie hal now, and udev in the near future), instead of indirectly via X. 

Also, I don't see what guarantees that the value stored on the root window is still the system default and not something else.

Of course you have to decide at some point what the definitive default is. E. g. on Ubuntu this is originally configured in /etc/default/console-setup, Debian calls it /etc/default/keyboard, Fedora has a similar file. This is then passed through hal and udev, where X.org reads it from. But X.org also has other sources, you could have it in xorg.conf, and upstream are discussing rewriting the X.org keyboard configuration now to get more independent from udev/hal (for compatibility to non-Linux). 

I agree that it's quite a mess right now, but to me the most robust solution seems to be to ask X and don't have to care about where (udev/hal/xorg.conf/whatever) the setting came from. Also, GNOME (including gdm) already uses libxklavier, so why using two different systems within gdm itself?

Where would you take the setting from now from an upstream's POV?

If you prefer, we can keep this as distro patches for the time being. However, right now the  upstream gdm has a guaranteed-broken implementation (which _always_ uses US layout), so accepting _some_ patch upstream seems appropriate?

Some further problems with the libxklavier approach, pointed out by Peter Hutterer:

There is only one root window property, but there can be multiple keyboards. It is somewhat nondeterministic which device 'wins' at server initialization. So you may well end up with the property value being 'us' because the 'keyboard' that won was actually a power button or similar :-(

Right, I'm aware of that. The hal patch had the problem as well, since in the end you _can_ have multiple keyboards with different layouts, and want to present just one default in gdm.

The solution/workaround for this with the hal FDI rules, and now with the proposed udev rules is to set the system keyboard layout to all input devices which have keys, not just "real" keyboards.

I'm going to commit this now for the .5 release.  I think we need a better solution long-term, but then again, I think the whole "choose your keyboard layout from GDM" thing needs to be more carefully thought about in general.  It's been the source of a lot of issues and its behavior has been torqued and twisted to work around those issues.

Eventually, we'll need to take a step back, think about the problems we want solved and go from from there I think.

Comment on attachment 148577 [details] [review]
detect default layout with xklavier

committed as 978596dcd5cbca22f6dc94669219b23f1626cf4f

Thanks Martin and Luca.

I'm sorry, this introduced a crash when you mistyped the password, or press Cancel, i. e. whenever you switch to the "enter password" mask a second time.

http://launchpadlibrarian.net/37788065/Stacktrace.txt shows why: xkl_engine_get_instance() is a singleton, so if you call get_system_default_layout() more than once, the XklEngine returned on the second time contains an invalid pointer to the Display that was closed on the first call (with XCloseDisplay). (Thanks to Chris Coulson for spotting this).

I committed a fix for this to trunk:
http://git.gnome.org/browse/gdm/commit/?id=51669cb03613b36b0b1798b1f8d2bba85b3e2a49

(Hope you don't mind committing directly; please harass me if you rather prefer reviewing such kind of patches in bz)