GNOME Bugzilla – Bug 673547
keycode replacement logic not reliably working
Last modified: 2012-10-26 12:45:11 UTC
Created attachment 211351 [details] [review] patch Sorry if this is not supposed to work. I tried to use the Arabic layout without changing the system keyboard settings, with the following: $ cd /usr/share/caribou/layouts/touch $ sudo cp ara.xml us.xml $ cd ~/gnome/checkout/caribou/caribou/antler ... edit keyboard_view.py to work standalone (see bug 673543) $ python keyboard_view.py then Arabic keyboard is shown. However, when I push any Arabic key button, key event with different keysym is sent (xev reports that XF86XK_WLAN here). I guess the XKB-based keycode replacement logic (seems to be derived from at-spi?) around XTestFakeKeyEvent is not working reliably. Using core mapping event seems to solve the problem (patch attached).
Created attachment 227235 [details] [review] xadapter: specify core device when calling XkbSetMap device_spec needs to be specified when calling XkbSetMap so that the keymap change affects XTest. -- I noticed that this bug can now be exposed when using Caribou on Czech layout,where some symbols ("$", "[", "]", etc) are not in level 1 and 2. After debugging, the root cause seems that Caribou doesn't set XkbDescRec->device_spec: http://lists.x.org/archives/xorg/2012-October/054993.html
Created attachment 227237 [details] [review] xadapter: use XkbChangeMap instead of XkbSetMap Optimize using XkbChangeMap instead of XkbSetMap. Also fix type of Xkb.ClientMap.syms. -- This patch is good to have as well.
Review of attachment 227235 [details] [review]: Looks good to me
Review of attachment 227237 [details] [review]: Reviewed, setting as needs-work in the sense of "I need info" ::: libcaribou/external-libs.vapi @@ +239,3 @@ public int AllEventsMask; + [CCode (cname = "XkbStateNotify")] There are several cases like this, where only the indentation were changed. Is this really necessary? ::: libcaribou/xadapter.vala @@ +162,3 @@ + changes.first_key_sym = (char) this.reserved_keycode; + changes.num_key_syms = this.xkbdesc.map.key_sym_map[this.reserved_keycode].width; + Xkb.change_map (this.xdisplay, this.xkbdesc, changes); Here it seems that you are using XkbChangeMap. But, the previous FIXME seems to suggest that was not working with XFree 4.3. How is the situation with this code? Is working or not with XFree 4.3?
(In reply to comment #4) > Review of attachment 227237 [details] [review]: > > Reviewed, setting as needs-work in the sense of "I need info" Thanks for the quick review! > ::: libcaribou/external-libs.vapi > @@ +239,3 @@ > public int AllEventsMask; > > + [CCode (cname = "XkbStateNotify")] > > There are several cases like this, where only the indentation were changed. Is > this really necessary? No, but some part of the current indentation looks unintentional for me. For example, while most of lines in external-libs.vapi start with 4 spaces, the above line starts with 3 spaces, and even other lines (like XkbAllMapComponents) start with 2 spaces. I'll preserve them if you like. > ::: libcaribou/xadapter.vala > @@ +162,3 @@ > + changes.first_key_sym = (char) this.reserved_keycode; > + changes.num_key_syms = > this.xkbdesc.map.key_sym_map[this.reserved_keycode].width; > + Xkb.change_map (this.xdisplay, this.xkbdesc, changes); > > Here it seems that you are using XkbChangeMap. But, the previous FIXME seems to > suggest that was not working with XFree 4.3. How is the situation with this > code? Is working or not with XFree 4.3? The comment seems to be copied around from at-spi: http://git.gnome.org/browse/at-spi/tree/registryd/deviceeventcontroller.c#n234 (modified at 2004-04-15). Do we still support it? Actually I've not tested with XFree86 4.3, so let me check anyway...
(In reply to comment #5) > (In reply to comment #4) > > Review of attachment 227237 [details] [review] [details]: > > > > Reviewed, setting as needs-work in the sense of "I need info" > > Thanks for the quick review! > > > ::: libcaribou/external-libs.vapi > > @@ +239,3 @@ > > public int AllEventsMask; > > > > + [CCode (cname = "XkbStateNotify")] > > > > There are several cases like this, where only the indentation were changed. Is > > this really necessary? > > No, but some part of the current indentation looks unintentional for me. > For example, while most of lines in external-libs.vapi start with 4 spaces, the > above line starts with 3 spaces, and even other lines (like > XkbAllMapComponents) start with 2 spaces. > > I'll preserve them if you like. If the new indentation is the one that make more sense I'm ok with changing it. Just wondering if it was a non-intentional change. > > ::: libcaribou/xadapter.vala > > @@ +162,3 @@ > > + changes.first_key_sym = (char) this.reserved_keycode; > > + changes.num_key_syms = > > this.xkbdesc.map.key_sym_map[this.reserved_keycode].width; > > + Xkb.change_map (this.xdisplay, this.xkbdesc, changes); > > > > Here it seems that you are using XkbChangeMap. But, the previous FIXME seems to > > suggest that was not working with XFree 4.3. How is the situation with this > > code? Is working or not with XFree 4.3? > > The comment seems to be copied around from at-spi: > http://git.gnome.org/browse/at-spi/tree/registryd/deviceeventcontroller.c#n234 > (modified at 2004-04-15). Do we still support it? > > Actually I've not tested with XFree86 4.3, so let me check anyway... Ok
Created attachment 227325 [details] [review] xadapter: specify core device when calling XkbSetMap device_spec needs to be specified when calling XkbSetMap so that the keymap change affects XTestFakeKeyEvent. Also fix the type of syms field in XkbClientMap Vala binding. -- I realized that the latter change is mandatory. So include it in this patch instead of the optimization patch.
Created attachment 227326 [details] [review] xadapter: use XkbChangeMap instead of XkbSetMap Optimize keycode replacement logic using XkbChangeMap instead of XkbSetMap. -- Added workaround for XFree86.
(In reply to comment #6) > If the new indentation is the one that make more sense I'm ok with > changing it. Just wondering if it was a non-intentional change. Sorry, the previous patch contained tabs and spaces that might confuse you. Fixed it in the new patch. > > Actually I've not tested with XFree86 4.3, so let me check anyway... > > Ok I've tested the code on RHEL 3.9, that shipped XFree86 4.3, and actually could reproduce the problem. Added a workaround.
Review of attachment 227325 [details] [review]: Looks good to me
Review of attachment 227326 [details] [review]: Looks good to me
Attachment 227325 [details] pushed as 661d4dc - xadapter: specify core device when calling XkbSetMap Attachment 227326 [details] pushed as e39364b - xadapter: use XkbChangeMap instead of XkbSetMap