GNOME Bugzilla – Bug 116626
Use keyboard map contents to detect RTL groups
Last modified: 2011-02-04 16:16:24 UTC
Ilya Konstantinov <future@shiny.co.il> wrote to me in August 2002: Hi Owen, I've written a function which detects whether a group belongs to an RTL script more reliably, by scanning the keysyms table and looking for Hebrew and Arabic keysyms. I appologize in advance for any C++-isms, as I've tested it originally on kxkb (KDE's keyboard switcher) code. Also, glib-ify it where appropriate (hmm, g_assert?). I think this function can safely replace the current "group name"-based RTL script detection in GTK+. This will make sure we'll show the proper cursor even if the Xkb group isn't named "Israelian" or "Arabic". Instead, or in addition to: XkbGetNames(dpy, XkbGroupNamesMask, xkbDesc); you would also want to run: XkbGetUpdatedMap(dpy, XkbKeySymsMask, xkbDesc); on every keyboard / mapping update Xkb event.
Created attachment 18007 [details] Code from email
This should probably not be so hardcoded, so Syriac and Thaana keymaps work too.
The Pango patch in bug 70451 (http://bugzilla.gnome.org/showattachment.cgi?attach_id=15871) adds PangoDirection pango_unichar_direction(gunichar ch) which probably could be used here in combination with gdk_keyval_to_unicode().
Owen, can you please give a hing in which function I should look for this functionality? I can prepare a patch in a couple of hours then.
Sorry for the noise. I found it and am working on the patch.
Created attachment 25369 [details] [review] Patch to fix the problem. Against CVS
I attached a patch to fix the problem using the proposed way. There are still small problems I see. I'm trying to fix them. One more thing I changed that may be wrong, I changed it so that returns PANGO_DIRECTION_NEUTRAL instead of PANGO_DIRECTION_LTR if there is no Xkb. Don't know if it harms.
Created attachment 25372 [details] [review] Patch that deprecates the previous patch
The patch that I just attached, improves the efficiency of the keymap direction code by caching the direction for three most recently used maps. Right now I do it conversatively by assuming that the string returned by gdk_x11_get_xatom_name_for_display(display, xkb->names->groups[group]) may change in the future, so I duplicate it in my cache. If it's not the case, I can simply just store and compare pointers to these returned names. Another thing I don't know is that I have used strcmp. Is there anything like g_ascii_strcmp? I guess it's Ok from the other points of view.
Behdad asked me to comment on this bit of code: for (;;) { int width = XkbKeyGroupsWidth(xkb, code); int w; for (w = 0; w < width; ++w) { KeySym sym = XkbKeySymEntry(xkb, code, w, group); This isn’t right; XkbKeyGroupsWidth() returns the number of groups. Should be using XkbKeyGroupWidth (xkb, code, group) instead which returns the number of shift levels. And rename the variable w to 'level' or 'shift_level' or something more appropriate. Also, the Xkb code uses int for the group, so I don't know why this code uses unsigned char. Untested version: static PangoDirection get_direction (XkbDescRec *xkb, gint group) { KeyCode code = xkb->min_key_code; gint ltr = 0; gint rtl = 0; /* We use an infinite loop here instead of 'for' since max_key_code * is often the maximum value of KeyCode (=CARD8), and a for loop * ends up overflowing and running endlessly. */ for (;;) { gint width = XkbKeyGroupWidth (xkb, code, group); gint level; for (level = 0; level < width; level++) { KeySym sym = XkbKeySymEntry (xkb, code, level, group); switch (pango_unichar_direction (gdk_keyval_to_unicode (sym))) { case PANGO_DIRECTION_RTL: rtl++; break; case PANGO_DIRECTION_LTR: ltr++; break; default: break; } } if (code == xkb->max_key_code) break; code++; } if (rtl > ltr) return PANGO_DIRECTION_RTL; else return PANGO_DIRECTION_LTR; }
Forgot to comment on the counting part. Behdad and I agreed that it made more sense to compare the number of RTL and LTR keys rather than look for a single LTR.
Created attachment 26082 [details] [review] Patch agains CVS This is a new patch, applied the comments from Noah, cleaned the style, and reverted back to not strdup the group name. It steel does strcmp the group names that may not be necessary. I have put enough comment in the code for that, but the current state is pretty safe, and not really inefficient.
Mass changing gtk+ bugs with target milestone of 2.4.2 to target 2.4.4, as Matthias said he was trying to do himself on IRC and was asking for help with. If you see this message, it means I was successful at fixing the borken-ness in bugzilla :) Sorry for the spam; just query on this message and delete all emails you get with this message, since there will probably be a lot.
Mozilla's use of gdk_keyboard_get_direction has brought an interesting thought: http://bugzilla.mozilla.org/show_bug.cgi?id=203671 Apparently, many platforms lack XKB. The patch presently at Mozilla's Bugzilla includes a non-XKB-based detection algorithm (used if XKB isn't available). I want to integrate this part of the patch right into GTK, so Mozilla could simply call into GTK and expect the Right Thing to happen.
=== +struct _DirectionCacheEntry { === { goes on the next line === static PangoDirection -get_direction (GdkKeymapX11 *keymap_x11) +get_direction (XkbDescRec *xkb, gint group) === Parameters go on separate lines and are aligned. === + /* We use an infinite loop here instead of 'for' since max_key_code + * is often the maximum value of KeyCode (=CARD8), and a for loop + * ends up overflowing and running endlessly. === Just use an int and a for loop and omit the comment. If we commented the source code with every bug that might occur ... -) === + PangoDirection dir = pango_unichar_direction + (gdk_keyval_to_unicode (sym)); === Don't break before the ( ... there's no requirement to fit into 80 column but if you want to shorten the line, put the declaraton PangoDirection dir; separately. === + switch (dir) { + case PANGO_DIRECTION_RTL: === { goes on the next line. === + group_name = (gchar *) gdk_x11_get_xatom_name_for_display (display, xkb->names->groups[group]); === Cast here is wrong, use the right type to start with. === + /* As per documentation we assume the string returned by + * gdk_x11_get_xatom_name_for_display is persistent and we + * don't copy it. + */ === If something is documented, it's not just an assumption, it's the way it works and you don't need to put in a comment :-). === + /* do we really need to do strcmp? if + * gdk_x11_get_xatom_name_for_display returns the + * same pointer for the same string across multiple + * calls, then we can simply compare pointers. it's + * seems to be highly probable, but the current code + * is safe. + */ === Store the atom in the cache entry not the name, then you can just compare atoms === @@ -458,16 +577,13 @@ gdk_keymap_get_direction (GdkKeymap *key GdkKeymapX11 *keymap_x11 = GDK_KEYMAP_X11 (keymap); if (!keymap_x11->have_direction) - { - keymap_x11->current_direction = get_direction (keymap_x11); - keymap_x11->have_direction = TRUE; - } + find_direction (keymap_x11); === It's really bad if a getter results in a signal being emitted. You probably need a gboolean parameter to find direction.
Created attachment 29960 [details] [review] Updated patch against CVS Thanks a lot Owen for the comments. Updated all. > Store the atom in the cache entry not the name, then you > can just compare atoms That's what I first did, but doesn't work: if you setxkbmap when running gedit, the atom does not change, but the name changes.
"if you setxkbmap when running gedit, the atom does not change, but the name changes." Errr, try again, that's not possible. The name for the same atom will always be the same.
Created attachment 29964 [details] [review] Final patch ;) Errr... I still got to learn what an atom is :-). This one should do it all, promise :@).
Created attachment 30107 [details] [review] New patch against HEAD Removed debugging printf.
Any news on this? The patch should be (hopefully) ready now.
The updated patch addresses most of owens points, but a few remain: + /* We use an infinite loop here instead of 'for' since max_key_code + * is often the maximum value of KeyCode (=CARD8), and a for loop + * ends up overflowing and running endlessly. + */ + for (code = xkb->min_key_code; code <= xkb->max_key_code; code++) Comment is out of sync with code, and should just be removed @@ -483,16 +590,13 @@ gdk_keymap_get_direction (GdkKeymap *key GdkKeymapX11 *keymap_x11 = GDK_KEYMAP_X11 (keymap); if (!keymap_x11->have_direction) - { - keymap_x11->current_direction = get_direction (keymap_x11); - keymap_x11->have_direction = TRUE; - } + _gdk_keymap_direction_changed (keymap_x11); As owen mentioned, we don't want the direction_changed signal to be emitted from the getter.
2004-11-29 Matthias Clasen <mclasen@redhat.com> Determine the direction of XKB groups from their content, not by looking for hardcoded keymap names. (#116626, patch by Behdad Esfahbod, based on an earlier patch by Ilya Konstantinov) * gdk/x11/gdkkeys-x11.c (struct _GdkKeymapX11): Cache directions for XKB groups. (get_direction): Determine direction of group by looking at directions of keysyms. (update_direction): Maintain the cache of group directions. (gdk_keymap_get_direction): Use update_direction().
Current code doesn't work for Israel (Hebrew) keyboard layout, as they have update-case Latin characters on the 2nd level, which are more than Hebrew characters. So the rtl_minus_ltr would be 0 or -1 for Israel layout. I REOPEN this bug, and will attach patch for CVS to fix this problem, by change to check the first shift-level of every layout, as it's the main layout of the language, and make it a little faster. Note that we're going to check shift-levels in Mozilla, to make it able to change the caret on changing the level (caps-lock, or holding the shift/altgr key)
Created attachment 72940 [details] [review] [01] just check the first shift-level mozilla-bug: 348724
I agree that we should only check the first level, but I believe shift or caps-lock should not change any logic. Mclasen, ok to commit to HEAD and branch?
Behnams argument makes sense to me. Behdad, why would shift or caps lock influence the outcome ? Would it not be a bug in the keyboard layout if shift or caps lock change the directionality of symbols ?
Fixed on HEAD and gtk-2-10 branch. 2006-09-20 Behnam Esfahbod <behnam@zwnj.org> * gdk/x11/gdkkeys-x11.c: (get_direction): just check the first shift-level of keyboard layout for RTL and LTR keysyms() (compliment to #116626)