Created attachment 125259 [details] [review]
Bugfix for metacity 2.24.0

Created attachment 125260 [details] [review]
Bugfix for metacity 2.25.55

Same patch as for 2.24.0, with different line numbers

Is this why I every morning when I get to work and plugin my external keyboard have to do:

1. Open "System -> Preferences -> Keyboard"
2. Select "Layouts" tab.
2.1 Notice that "Sweden Dvorak" is chosen, but external keyboard is qwerty
3. Open "Layout Options"
3.1 Notice that "Ctrl key position" is "Swap Ctrl and CapsLock" but  Ctrl and CapsLock isn't swapped.
4. Set "Ctrl key position" to "Default" and click "Close"
5. Open "Layout Options"
6. Set "Ctrl key position" to "Swap Ctrl and CapsLock" and click "Close"
7. Close "Keyboard Preferences"
8. Open up a terminal and write "setxkbmap se dvorak"
9. Finally, my external keyboard matches the keyboard on my laptop.

Possibly worth mentioning is that my external keyboard is found by ev input and not the oldskool way:


(II) config/hal: Adding input device Microsoft Natural? Ergonomic Keyboard 4000
(**) Microsoft Natural? Ergonomic Keyboard 4000: always reports core events
(**) Microsoft Natural? Ergonomic Keyboard 4000: Device: "/dev/input/event2"
(II) Microsoft Natural? Ergonomic Keyboard 4000: Found 1 mouse buttons
(II) Microsoft Natural? Ergonomic Keyboard 4000: Found keys
(II) Microsoft Natural? Ergonomic Keyboard 4000: Configuring as keyboard
(**) Microsoft Natural? Ergonomic Keyboard 4000: YAxisMapping: buttons 4 and 5
(**) Microsoft Natural? Ergonomic Keyboard 4000: EmulateWheelButton: 4, EmulateWheelInertia: 10, EmulateWheelTimeout: 200
(II) XINPUT: Adding extended input device "Microsoft Natural? Ergonomic Keyboard 4000" (type: KEYBOARD)
(**) Option "xkb_rules" "evdev"
(**) Microsoft Natural? Ergonomic Keyboard 4000: xkb_rules: "evdev"
(**) Option "xkb_model" "evdev"
(**) Microsoft Natural? Ergonomic Keyboard 4000: xkb_model: "evdev"
(**) Option "xkb_layout" "us"
(**) Microsoft Natural? Ergonomic Keyboard 4000: xkb_layout: "us"


or is this a separate bug?

This is not a metacity bug - not even sure it is a bug at all.

It surely can't be the expected behavior?

Sorry you can probably ignore comment 4, I guess I didn't read the description carefully enough.

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

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

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

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

Review of attachment 125260 [details] [review]:

This basically looks like a good fix to me. The key ingredient in why it is necessary (which needs to be described in a comment somewhere) is that *GTK+* selects for Xkb keyboard and mapping notify events when gdk_display_open() is called and delivery of MappingNotify events is disabled when a client has selected for Xkb events. E.g., comment in xserver/xkb/xkbEvents.c is:

 * This function [XkbSendLegacyMapNotify] sends out  two kinds of notification
 *   - Core mapping notify events sent to clients for whom kbd is the
 *     current core ('picked') keyboard _and_ have not explicitly
 *     selected for XKB mapping notify events;

(Presumably also in the docs somewhere)

Some detailed comments about the patch below; the patch also needs to be updated to current Metacity, since the code it is changing has been moved around and changed a bit.

There's a separate and much harder issue here that the grabbing code in Metacity is unable to handle the complexities of the XKB model of keyboards. (And in fact, XGrabKey really isn't up to it either.) Problems include:

 - Multiple installed layouts with the same keys in different positions (bug 103331)
 - Key symbols that are duplicated multiple places on the keyboard (https://bugzilla.redhat.com/show_bug.cgi?id=449908)

But that shouldn't block a fix like this one going in.

::: metacity-2.25.55/src/core/display.c.xkb
@@ +515,3 @@
+      
+      rebuild_window_binding_table (display);
+      rebuild_screen_binding_table (display);

I think these calls are not really right. For comparison, they don't occur in the MappingNotify versions of this code. I think you needed to add them because of a bug in reload_keycodes() - it reads:

          if (display->key_bindings[i].keycode == 0)
              display->key_bindings[i].keycode = XKeysymToKeycode (
                      display->xdisplay, display->key_bindings[i].keysym);

Which isn't going to work when keycode has already been initialized. Instead it should read:

          if (display->key_bindings[i].keysym != 0)

(That is, the binding is on a keysym not a keycode)

It probably makes sense to unify all three branches here with some boolean variables 'modifiers_changed' 'keycodes_changed' or something like that; otherwise only the XKB branch will ever be debugged.

@@ +614,3 @@
+  /* meta_display_init_keys() should have already called XkbQueryExtension() */
+  if (-1 == display->xkb_base_event_type ||
+      False == XkbSelectEvents (display->xdisplay, XkbUseCoreKbd,

The reversed conditionals here aren't in accordance to the Metacity style (GCC will catch an assignment used as a conditional anyways, so the trick isn't needed to deal with that.)

Also, I don't think it's worth checking the return valur for XkbSelectEvents - it's not a round trip to the server and can't fail for any interesting reason. The return value is documented as:

       True           The XkbSelectEvents function returns True if the Xkb extension has been initilialized.
       False          The XkbSelectEvents function returns False if the Xkb extension has not been initilialized.

@@ +618,3 @@
+                                XkbNewKeyboardNotifyMask | XkbMapNotifyMask))
+      meta_topic (META_DEBUG_KEYBINDINGS,
+                  "Unable to respond to XKB keyboard mapping changes");

If we don't have XKB, then the MappingNotify branches will work, so this comment, while true, is a bit deceptive.

Reporter: want to put something together addressing Owen's points, or shall I do it?

Review of attachment 125260 [details] [review]:

Setting patch to reviewed, since Owen reviewed it

Created attachment 160146 [details] [review]
Derek's patch, in git format, against current metacity, with slight tweaking

I am taking matters into my own hands and updating the patch.  Owen, if you have a moment, could you verify that this answers your issues raised in the review?

Another patch follows in a moment to unify the three branches of the keymap-updating code.

Created attachment 160147 [details] [review]
Unify the three branches of the keymap-reloading code

As before.

Review of attachment 160146 [details] [review]:

Just one comment. I'll attach a fixed up version of the patch. Need to push something to Mutter in this area so I can write another patch that modifies reload_keycodes.

::: src/core/keybindings.c
@@ +618,3 @@
+    {
+      meta_topic (META_DEBUG_KEYBINDINGS,
+                  "Unable to respond to XKB keyboard mapping changes");

Comment I made on the original patch still applies here.

if display->xkb_base_event_type == -1 then we don't have XKB and we'll get MappingNotify events, so this message is deceptive.
if display->xkb_base_event_type != -1 then the XkbSelectEvents call can't fail.

So there's no reason for a message here.

Created attachment 175063 [details] [review]
Update keybindings when XKB keyboard layout changes

Here's my version of Derek's patch. Removed the deceptive warning
message and added a useful commit message.

Created attachment 175065 [details] [review]
Update keybindings when XKB keyboard layout changes

Also with fix so it actually compiles against current Metacity.

Review of attachment 160147 [details] [review]:

::: src/core/keybindings.c
@@ +517,3 @@
                   "XKB mapping changed, will redo keybindings\n");
 
+      redo_keybindings = TRUE;

Don't see the point in this as compared to writing (redo_keycodes || redo_modifiers) below.

@@ +545,1 @@
       reload_keymap (display);

This wasn't called before in the MappingModifier case.

@@ +553,3 @@
+      if (redo_modifiers)
+        {
+          reload_modifiers (display);

It looks like a bug to me that this wasn't called in the MappingKeyboard case. MappingKeyboard requires reload_modmap() because that depends on key symbols. And when the loaded modifier map changes the modifiers in bindings can change.

Review of attachment 160147 [details] [review]:

::: src/core/keybindings.c
@@ +536,3 @@
                   "Received MappingKeyboard event, will reload keycodes and redo keybindings\n");
 
+      redo_keybindings = TRUE;

Also, this needs to set redo_keycodes

Created attachment 175067 [details] [review]
Unify keymap-reloading code branches

Simplify the keymap loading logic by unifying the different
branches; in the reorganization this patch fixes a bug where when
we got a MappingKeyboard event we wouldn't update virtual modifiers
correctly.

Based on a patch by Thomas Thurman <tthurman@gnome.org>

Comment on attachment 175065 [details] [review]
Update keybindings when XKB keyboard layout changes

looks right

Comment on attachment 175067 [details] [review]
Unify keymap-reloading code branches

seems right

Patches pushed with my fixes and Dan's review. Sorry for the slow response
at reviewing the last versions; lost track that it this was
blocking on me.

The one thing I'm left uncertain here is a paragraph in the XKB docs:

 The X Keyboard Extension uses XkbMapNotify and XkbNewKeyboardNotify
 events to track changes to the keyboard mapping. When an Xkb-aware
 client receives either event, it should call
 XkbRefreshKeyboardMapping to update the keyboard description used
 internally by the X library. To avoid duplicate events, the X server
 does not send core protocol MappingNotify events to a client that has
 selected for XkbMapNotify events.

We don't do that in this patch, yet the patch seems to work. Reading the
code in Xlib didn't immediately enlighten me. If we find additional
problems in this area, that is probably that. (Actually, GTK+ would
need to be doing that, since it's already selecting for XkbMapNotify
before Metacity gets into the picture, but it doesn't seem to do that
either.)
Attachment 175065 [details] pushed as c262e3d - Update keybindings when XKB keyboard layout changes
Attachment 175067 [details] pushed as ba2e5f7 - Unify keymap-reloading code branches