GNOME Bugzilla – Bug 353805
Detecting Bidi Keyboard Layouts
Last modified: 2007-06-30 01:44:50 UTC
On Mozilla bug 348724 - nsBidiKeyboard for GTK2 backend we need support for keyboard layouts, getting user's layouts and a way to enable one of them. More info: https://bugzilla.mozilla.org/show_bug.cgi?id=348724 It's possible to use xklavier for that bug, but it will add xklavier dependency, which is not always provided where gtk+ is. OTOH GKT+ and other GTK+-based applications may use this info to enable some bidi feature (i'm going to file them on separate bugs) (also look at bug 312006 - gedit - Adding BidiAssist to gedit's cvs) If fact, a user is a *bidi user* iff she has keyboard layouts in both LTR and RTL directions. Thats how Mozilla is going to treat a bidi user. It's not necessary to enable bidi features for RTL locales, as the user may never want to write in any LTR language.
Behnam, Gtk+ already queries user's keyboard layout to set the line direction. It does that using Xkb.
I know, but that's only for *current* layout. There's no way to get all keymaps user have set. So you cannot find out whether user have any other layout, if has, is it RTL or LTR. All GDK has right now is: gdk_keymap_get_default and gdk_keymap_get_for_display. For X11, we can use xklavier or xkblib/xkbrules (see patch of https://bugzilla.mozilla.org/show_bug.cgi?id=348724), but it's not cross platform.
What I'm trying to say is that we don't need xklavier for that. Currently we are doing: update_direction (keymap_x11, xkb_event->state.locked_group); So we are specifically accessing the current group. I'm sure we can access all groups using xkb.
Ok, maybe there's no need for xklavier nor xkbrules. I'm going to find out how to get them in X11, which maybe fix mozilla's bug. But for other bug depend on this, you need a cross-platform API for that.
(In reply to comment #4) > Ok, maybe there's no need for xklavier nor xkbrules. > > I'm going to find out how to get them in X11, which maybe fix mozilla's bug. > But for other bug depend on this, you need a cross-platform API for that. The way this stuff typically works is that we fix stuff in the X11 backend and let other backends catchup as requests+help fomes in.
Created attachment 73431 [details] [review] patch v1 Move getting, initializing, and updateing of cache from (update_direction) to (get_direction_from_cache). Add (get_num_groups) which returns the number of keyboard layouts. Add (gdk_keymap_have_bidi_directions) which runs (get_direction_from_cache) for each (get_num_groups) and return true if both LTR and RTL groups do exist. Add "gboolean gdk_keymap_have_bidi_directions (GdkKeymap *keymap);" to the API. Change XkbGroupLock to XkbStateGroup, which is the real layout user gets.
Created attachment 73442 [details] [review] patch v1 - more readable previous patch, more readable
Comment on attachment 73442 [details] [review] patch v1 - more readable >+int static here. >+get_num_groups (GdkKeymap *keymap, XkbDescPtr xkb) >+{ >+ Display *display = KEYMAP_XDISPLAY (keymap); >+ XkbGetControls(display, XkbSlowKeysMask, xkb); >+ XkbGetUpdatedMap (display, XkbKeySymsMask | XkbKeyTypesMask | >+ XkbModifierMapMask | XkbVirtualModsMask, xkb); >+ return xkb->ctrls->num_groups; >+} This needs #if HAVE_XKB guards. >+gboolean >+gdk_keymap_have_bidi_directions (GdkKeymap *keymap) Have_bidi_directions doesn't make much sense. have_bidi_layouts maybe.
Created attachment 73445 [details] [review] patch v2 Addressed previous comment. Thanks Behdad. May I commit?
> May I commit? Matthias should decide. But regardless, you should add documentation block and add the symbol to gdk.symbols in the right place, and hook it into documentation too.
Created attachment 73545 [details] [review] patch v4, previous one, with symbols and docs * previous patch for gdk/gdkkeys.h and gdk/x11/gdkkeys-x11.c * add gdk_keymap_have_bidi_layouts to gdk/gdk.symbols * write doc for gdk_keymap_have_bidi_layouts in code and docs/reference/gdk/tmpl/keys.sgml * update doc for gdk_keymap_get_direction in docs/reference/gdk/tmpl/keys.sgml and copy it to code Matthias?
I don't understand keyboard layouts much at all, much less RTL and Bidi stuff, so I'm having a hard time understanding the impact of this bug. Why is it marked as a GNOME showstopper?
(In reply to comment #12) > I don't understand keyboard layouts much at all, much less RTL and Bidi stuff, > so I'm having a hard time understanding the impact of this bug. Why is it > marked as a GNOME showstopper? No it's not a showstopper. It's kinda confusing. People use the Gnome target as what they want to see it in, rather than what it should be blocking... It's partly bugzilla's fault for having so many underdocumented overlapping parameters of course :-).
(In reply to comment #13) > No it's not a showstopper. It's kinda confusing. People use the Gnome target > as what they want to see it in, rather than what it should be blocking... It's > partly bugzilla's fault for having so many underdocumented overlapping > parameters of course :-). Or perhaps the fault of users for not clicking on "Gnome target" next to the drop down box to find out that it *is* fully documented and explained. ;-) To be fair, though, I do understand that we have so many links on show_bug.cgi that very few are going to follow all of them, so I don't really blame this on bugzilla users. I just figure it means we need to run an education campaign. And it just so happens that my comments in this bug are part of such a campaign. :-) Anyway, thanks for the info; I'm unsetting the gnome target.
(In reply to comment #14) > (In reply to comment #13) > > No it's not a showstopper. It's kinda confusing. People use the Gnome target > > as what they want to see it in, rather than what it should be blocking... It's > > partly bugzilla's fault for having so many underdocumented overlapping > > parameters of course :-). > > Or perhaps the fault of users for not clicking on "Gnome target" next to the > drop down box to find out that it *is* fully documented and explained. ;-) To > be fair, though, I do understand that we have so many links on show_bug.cgi > that very few are going to follow all of them, so I don't really blame this on > bugzilla users. I just figure it means we need to run an education campaign. > And it just so happens that my comments in this bug are part of such a > campaign. :-) > > Anyway, thanks for the info; I'm unsetting the gnome target. Does disabling some of the fields for certain groups of users help? Non-developers, developers, developers of the module the bug is filed against, and bugmasters...
(In reply to comment #15) > Does disabling some of the fields for certain groups of users help? > Non-developers, developers, developers of the module the bug is filed against, > and bugmasters... Not really. We would like to have anyone with editbugs privileges be able to nominate showstopper bugs, because the bugsquad might not be able to catch them all. (Note that users without editbugs have nearly every field disabled, so gnome-target isn't much of a special case for them). We do have a disabling of the fields for certain groups of products -- namely, gnome-version and gnome-target are disabled & hidden for products not part of either the desktop, admin, bindings, or platform classification, but that's a bit different. Besides, I think the problem will be solved pretty naturally by having regular showstopper review reports (http://live.gnome.org/Bugsquad/ShowstopperReviews) sent to d-d-l. That'll make it clear what gnome-target is for. Andre's working on one right now, I was just trying to make sure the list only contained relevant bugs... :-) If you have any other good ideas, I'm open to hearing them. We probably ought to do so in a separate bug, though.
I have no principal objections to gdk_keymap_have_bidi_layouts() as an api addition for HEAD. There are some small things in the patch to clean up: +static int +get_num_groups (GdkKeymap *keymap, XkbDescPtr xkb) Please put parameters on separate lines and line them up. - update_direction (keymap_x11, XkbGroupLock (&state_rec)); + update_direction (keymap_x11, XkbStateGroup (&state_rec)); Why this change ? It may be correct, but it looks like it changes the behaviour of gdk_keymap_get_direction ? + register int i; Please don't use register. It doesn't add anything. RCS file: /cvs/gnome/gtk+/docs/reference/gdk/tmpl/keys.sgml,v No need to put the function docs in two places. Just put it all in the inline docs.
Created attachment 79915 [details] [review] patch v10, previous one, applied the comments All comments, except the XkbStateGroup issue, have been applied.
(In reply to comment #17) > - update_direction (keymap_x11, XkbGroupLock (&state_rec)); > + update_direction (keymap_x11, XkbStateGroup (&state_rec)); > > > Why this change ? It may be correct, but it looks like it changes > the behaviour of gdk_keymap_get_direction ? #define XkbStateGroup(s) ((s)->base_group+(s)->latched_group+XkbGroupLock(s)) IMHO as base_group and latched_group are almost not used nowadays, using XkbGroupLock is enough, but using XkbStateGroup is the right way. CCing SVU.
Well, SVU confirmed that using base_group and latched_group is almost deprecated (on IRC). Also note that if some app uses one of these, and then uses gdk_keymap_get_direction would get the wrong result. So this patch is going to fix another unrecognized bug too. So would you please just commit this for the 2.12 release?
Yeah, looks fine to add this 2007-06-29 Matthias Clasen <mclasen@redhat.com> * gdk/gdk.symbols: * gdk/gdkkeys.h: * gdk/x11/gdkkeys-x11.c (gdk_keymap_have_bidi_layouts): New function to determine if keyboard layouts for both LTR and LTR languages are in use. Refactor the direction caching code to make this information available. (#353805, Behnam Esfahbod)