After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 572765 - always overrides keyboard layout variants
always overrides keyboard layout variants
Status: RESOLVED FIXED
Product: gdm
Classification: Core
Component: general
2.29.x
Other Linux
: Normal normal
: ---
Assigned To: GDM maintainers
GDM maintainers
: 599032 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2009-02-22 18:02 UTC by Id2ndR
Modified: 2011-02-03 14:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Ubuntu patch (683 bytes, patch)
2009-07-03 17:23 UTC, Martin Pitt
none Details | Review
fixed patch (1.74 KB, patch)
2009-07-06 13:30 UTC, Martin Pitt
none Details | Review
detect keyboard layout from hal (2.97 KB, patch)
2009-07-20 11:01 UTC, Martin Pitt
none Details | Review
detect keyboard layout and variant from hal (3.90 KB, patch)
2009-10-01 12:56 UTC, Martin Pitt
none Details | Review
Patch for gdm-session-direct.c, gdm-session-settings.c (2.16 KB, patch)
2009-10-21 09:29 UTC, Takao Fujiwara
none Details | Review
detect default layout with xklavier (2.33 KB, patch)
2009-11-08 11:15 UTC, Luca Bruno
none Details | Review
detect default layout with xklavier (2.72 KB, patch)
2009-11-27 08:28 UTC, Martin Pitt
committed Details | Review

Description Id2ndR 2009-02-22 18:02:07 UTC
1) Go to login,
2) Select a user,
3) At bottom in keyboard layout dropdown menu there is NO chose : it's empty
4) Login
5) The environnement variable GDM_KEYBOARD_LAYOUT is set to "us"
6) Run gnome-keyboard-properties : there is an USA keyboard.
7) Click on "reset to default" in gnome-keyboard-properties to get back the correct layout (french in my case).

System : Ubuntu jaunty alpha 4. gdm-new - 2.25.2-0ubuntu0.1 package installed from https://launchpad.net/~ubuntu-desktop/+archive/ppa
Comment 1 Martin Pitt 2009-07-03 16:43:41 UTC
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.
Comment 2 Martin Pitt 2009-07-03 17:23:03 UTC
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
Comment 3 Martin Pitt 2009-07-06 13:30:20 UTC
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.
Comment 4 Matthias Clasen 2009-07-10 13:10:13 UTC
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
Comment 5 Ray Strode [halfline] 2009-07-15 18:17:30 UTC
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?
Comment 6 Martin Pitt 2009-07-16 06:35:26 UTC
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!
Comment 7 Ray Strode [halfline] 2009-07-17 02:18:52 UTC
Should be set as of 4b177ed3c1998d9cd2f203f5c635b3f73ca4aecd.
Comment 8 Martin Pitt 2009-07-17 07:32:17 UTC
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!
Comment 9 Martin Pitt 2009-07-20 11:00:47 UTC
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?
Comment 10 Martin Pitt 2009-07-20 11:01:49 UTC
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)
Comment 11 Martin Pitt 2009-07-20 13:30:22 UTC
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.
Comment 12 Matthias Clasen 2009-07-20 13:42:00 UTC
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.
Comment 13 Martin Pitt 2009-09-30 16:13:59 UTC
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.
Comment 14 Martin Pitt 2009-09-30 17:15:52 UTC
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.
Comment 15 Martin Pitt 2009-10-01 08:37:39 UTC
> BTW, it seems that gdm now actually behaves according to the original request
in bug 585290

Please ignore that, that was just confusing.
Comment 16 Martin Pitt 2009-10-01 12:56:05 UTC
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.
Comment 17 Ray Strode [halfline] 2009-10-20 18:08:57 UTC
*** Bug 599032 has been marked as a duplicate of this bug. ***
Comment 18 Takao Fujiwara 2009-10-21 09:29:12 UTC
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.
Comment 19 Takao Fujiwara 2009-10-29 05:36:28 UTC
(In reply to comment #18)
> Created an attachment (id=145935) [details] [review]

The latest Fedora fixes my problem. Please ignore the previous comment.
Thanks.
Comment 20 Luca Bruno 2009-11-08 11:15:49 UTC
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.
Comment 21 Martin Pitt 2009-11-27 08:28:19 UTC
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!
Comment 22 Matthias Clasen 2009-11-27 19:13:04 UTC
I don't think that is the right approach.
Comment 23 Martin Pitt 2009-11-27 21:13:49 UTC
Matthias,

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

Thanks,

Martin
Comment 24 Matthias Clasen 2009-11-27 21:55:25 UTC
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.
Comment 25 Martin Pitt 2009-11-27 22:38:54 UTC
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?
Comment 26 Matthias Clasen 2009-12-01 16:51:28 UTC
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 :-(
Comment 27 Martin Pitt 2009-12-01 17:52:31 UTC
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.
Comment 28 Ray Strode [halfline] 2010-01-12 20:31:22 UTC
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 29 Ray Strode [halfline] 2010-01-12 20:34:54 UTC
Comment on attachment 148577 [details] [review]
detect default layout with xklavier

committed as 978596dcd5cbca22f6dc94669219b23f1626cf4f
Comment 30 Ray Strode [halfline] 2010-01-12 20:35:23 UTC
Thanks Martin and Luca.
Comment 31 Martin Pitt 2010-01-18 15:14:59 UTC
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)