348724 - (BidiKeyboardGtk2) nsBidiKeyboard for GTK2 backend

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Behnam Esfahbod [:zwnj]]

Add GTK2 backend for nsBidiKeyboard.

As GDK doesn't support this feature, xlib/xkblib must be use directly.

I'm going to attach my patch, which adds xkbfile dependecy to the trunk.

Comment 1

[Behnam Esfahbod [:zwnj]]

Attachment: Patch for RTL and Bidi keyboard detection, without support to set LTR/RTL layouts

Comment 2

[Behnam Esfahbod [:zwnj]]

Just note that "sy" should be added to the list too.

Comment 3

[Behnam Esfahbod [:zwnj]]

Feature requested on Gnome Bugzilla: Bug 353805 – Support for keyboard layouts

Comment 4

[Behnam Esfahbod [:zwnj]]

Attachment: [04] Uses pure XKBlib functions, able to use GDK's impls

Implements reading keyboard layouts.  Selecting keyboard layout coming soon. 

Just needs one GTK+ version check for now, and some other after implementation in GDK.

Comment 5

[Behnam Esfahbod [:zwnj]]

Attachment: [05] Use clean XKB functions, add forgotten "free"s, solve the Hebrew problem

Comment 6

[Behnam Esfahbod [:zwnj]]

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

Comment 7

[Behnam Esfahbod [:zwnj]]

Attachment: patch v6, simply use gtk api

GTK+ 2.12 will have needed API (gnome-bug 353805) and this patch simply uses that API to determine if bidi keyboard layouts have been set.

As the implementation for old GTK+ need a log of XKB code (which is hard to debug) for caching and updating, and this is just an enhancement, I don't care about old GTK+ versions for this bug.

Note that GTK+ 2.12 will be released sooner than Fx3.0.

[In fact, I want to work on other important bugs, so feel free to port GTK+ code, or write new one, if you have time.  But this patch works well for me]

Comment 8

[Simon Montagu :smontagu]

Comment on attachment 240463 [details] [diff] [review]
patch v6, simply use gtk api

>+    if (gtk_check_version (2, 12, 0) || gtk_check_version (2, 10, 1))
>+        *aIsRTL = (gdk_keymap_get_direction(NULL) == PANGO_DIRECTION_RTL);
>+
>+    return NS_OK;

>+#if GTK_CHECK_VERSION (2, 12, 0)
>+    if (gtk_check_version (2, 12, 0))
>+        mHaveBidiKeyboards = gdk_keymap_have_bidi_layouts(NULL);
>+#endif /* GTK_CHECK_VERSION */

These need comments saying what exactly you are testing for and why, and why you have both a compile-time check and a run-time check in SetupBidiKeyboards() and only a runtime check in IsLangRTL(). I would have expected the other way round, since all the code has to be compiled, but the call in IsLangRTL() will never be reached in incompatible versions. 

Also, if I understand http://developer.gnome.org/doc/API/gtk/gtk-feature-test-macros.html correctly, gtk_check_version returns NULL when the version is compatible, so shouldn't you be testing |if (!gtk_check_version(...))| ?

Comment 9

[Behnam Esfahbod [:zwnj]]

Attachment: patch v10, fix gtk version checking

Thanks Simon.

The first Gtk version checking was non-sence, as the function have been available from Gtk 2.0 or 2.2.

The second on now works well (I forgot to compare the return value to NULL), and now it's fixed.

Simon, to test the patch, I changed the 2.12 to 2.10 and tried it on my system's Gtk 2.8 and my own patched 2.10, and it worked well.

Also I moved the destructor to protected part, as suggested by IDL compiler.

Comment 10

[Behnam Esfahbod [:zwnj]]

(In reply to comment #8)
> (From update of attachment 240463 [details] [diff] [review] [edit])
> >+    if (gtk_check_version (2, 12, 0) || gtk_check_version (2, 10, 1))
> >+        *aIsRTL = (gdk_keymap_get_direction(NULL) == PANGO_DIRECTION_RTL);
> >+
> >+    return NS_OK;

Fixed in the best way: I removed the version-checking line.

> >+#if GTK_CHECK_VERSION (2, 12, 0)
> >+    if (gtk_check_version (2, 12, 0))
> >+        mHaveBidiKeyboards = gdk_keymap_have_bidi_layouts(NULL);
> >+#endif /* GTK_CHECK_VERSION */
> 
> These need comments saying what exactly you are testing for and why, and why
> you have both a compile-time check and a run-time check in SetupBidiKeyboards()
> and only a runtime check in IsLangRTL(). I would have expected the other way
> round, since all the code has to be compiled, but the call in IsLangRTL() will
> never be reached in incompatible versions. 

BTW, the first call have been in the API for years (but had detection problems), so it doesn't need any compile time checking.

The second one is new and will be on GTK 2.12 API, so I need to check if the function exist in the API on compile time.  But I need to check it run time, to make it possible to run the app which have been built on a GTK >= 2.12 on older GTK versions.

I can put this comments on the code if you think they're necessary.  Also I can add the compile-time check to the first call too, just to make the compiled version (on old systems) a bit smaller.

Comment 11

[Behdad Esfahbod]

> The second one is new and will be on GTK 2.12 API, so I need to check if the
> function exist in the API on compile time.  But I need to check it run time, to
> make it possible to run the app which have been built on a GTK >= 2.12 on older
> GTK versions.

That is inherently broken and only works with lazy linking.  It's at best a hack, and should be avoided.

Comment 12

[Reed Loden [:reed]]

Any update on this?

Comment 13

[Behnam Esfahbod [:zwnj]]

Well, in Linux Runtime Requirements, the GKT+ version is 2.10.x, but the function I use here was added to 2.12.  I'm not sure what we can do now.

There won't be any problem on compile time, but if it's compiled on a system with 2.12 version, it (probably) won't run on a system with 2.10; which I think is not acceptable.  Am I right?

We can use the not-so-good code of the 1.8 branch, which doesn't work of Hebrew at all.

Simon, Behdad, any idea?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why not just use a dlsym trick to call the function only when it's available? We do that all over the place (maybe less lately, but it's still a valid strategy).

Comment 15

[Behnam Esfahbod [:zwnj]]

(In reply to comment #14)
> Why not just use a dlsym trick to call the function only when it's available?
> We do that all over the place (maybe less lately, but it's still a valid
> strategy).

Sorry, I don't know what dlsym is.  I'm going to google it; but would you please show me an example in the cvs?


Here are what I wrote:

+#if GTK_CHECK_VERSION (2, 12, 0)
+    if (gtk_check_version (2, 12, 0) == NULL)
+        mHaveBidiKeyboards = gdk_keymap_have_bidi_layouts(NULL);
+#endif /* GTK_CHECK_VERSION */

The gtk_check_version is the run-time check; which Behdad says is not acceptable, but I'm not sure, as it should work on almost all Linux distros.

Comment 16

[Behnam Esfahbod [:zwnj]]

Ok, I found what dlsym() does.  Seems using gtk_check_version is as risky as dlsym; as for both of them, a lazy linker is necessary.

So, Simon, would you review the last patch please?

Who can does the sr?

Thanks Robert.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://mxr.mozilla.org/seamonkey/source/widget/src/gtk2/nsSound.cpp#118
Lazy linking is NOT necessary here. It just works.

I can do sr's.

Comment 18

[Behdad Esfahbod]

Behnam, dlsym() is runtime-linking.  If the symbol is not available, you just get a NULL returned, no big deal.  With normal linking (lazy or not), as soon as the symbol is not found the program is terminated.

Comment 19

[Behnam Esfahbod [:zwnj]]

Attachment: patch v11, use PR_LoadLibrary and PR_FindFunctionSymbol

Simon, I decided to not call SetHaveBidiKeyboards() on every IsLangRTL() call; instead I used cached mHaveBidiKeyboards.  This means if user changes her having-bidi-keyboard-layouts status when mozilla is running, the cursor mode remains the same.

If it's necessary to get that work too, that would be a good idea to call SetHaveBidiKeyboards() on another function with less overall calls.

BTW I changed the additional function name, and it doesn't match the win32 one, as their work is a bit different.  Don't you mind?

Comment 20

[Simon Montagu :smontagu]

Did you mean to set the review request on attachment 233786 [details] [diff] [review] instead of the latest version?

Comment 21

[Behnam Esfahbod [:zwnj]]

(In reply to comment #20)
> Did you mean to set the review request on attachment 233786 [details] [diff] [review] instead of the
> latest version?

No, sorry.  I'll fix the review requests now.

Comment 22

[Simon Montagu :smontagu]

Comment on attachment 303718 [details] [diff] [review]
patch v11, use PR_LoadLibrary and PR_FindFunctionSymbol

(In reply to comment #19)
> Simon, I decided to not call SetHaveBidiKeyboards() on every IsLangRTL() call;
> instead I used cached mHaveBidiKeyboards.  This means if user changes her
> having-bidi-keyboard-layouts status when mozilla is running, the cursor mode
> remains the same.

Users don't install and remove keyboards all that often, so I guess that's OK.

r=me on the bidi functionality, but you need either addl. review or sr from someone who knows from GTK.

Comment 23

[Behdad Esfahbod]

(In reply to comment #22)
> (From update of attachment 303718 [details] [diff] [review])
> (In reply to comment #19)
> > Simon, I decided to not call SetHaveBidiKeyboards() on every IsLangRTL() call;
> > instead I used cached mHaveBidiKeyboards.  This means if user changes her
> > having-bidi-keyboard-layouts status when mozilla is running, the cursor mode
> > remains the same.
> 
> Users don't install and remove keyboards all that often, so I guess that's OK.
> 
> r=me on the bidi functionality, but you need either addl. review or sr from
> someone who knows from GTK.

Looks good except that this line is not multi-screen-safe:

+    *aIsRTL = (gdk_keymap_get_direction(NULL) == PANGO_DIRECTION_RTL);

The NULL is the problem.  Not sure if Firefox tries to respect per-screen settings or not.

Comment 24

[Behnam Esfahbod [:zwnj]]

Thanks Behdad.

(In reply to comment #23)
> Looks good except that this line is not multi-screen-safe:
> 
> +    *aIsRTL = (gdk_keymap_get_direction(NULL) == PANGO_DIRECTION_RTL);
> 
> The NULL is the problem.  Not sure if Firefox tries to respect per-screen
> settings or not.

Looking around nsWindow, I found out that there's nothing unsafe for mutli-screen there, as we get the right windows from the widgets all the time.

The problem here is that nsBidiKeyboard isn't designed that way and the widget is not passed to the functions.  So there's no way to get the default window.

The best solution would be to get the active window from window-manager, which I *think* should be same as passing NULL to this function.

Simon, can we improve nsIBidiKeyboard?  Would it be a waste?  It wouldn't be approved for 1.9, right?

Comment 25

[Simon Montagu :smontagu]

(In reply to comment #24)

> Simon, can we improve nsIBidiKeyboard?  Would it be a waste?  It wouldn't be
> approved for 1.9, right?

That would be classic post-1.9 material.

Comment 26

[Behnam Esfahbod [:zwnj]]

Attachment: patch v12, use PR_LoadLibrary and PR_FindFunctionSymbol (both just once in constructor)

Thanks Simon. I will work on the multi-display problem later.

I just updated the patch to use both PR_* functions just once in the constructor.  Simon, if it's OK, please obsolete the older one.

So, can we get this patch in 1.9 please?  It works fine for more than 99% of users, and *may* show incorrect direction for the rest.  This is lot better than not having bidi-keyboard in GTK+ at all.

Comment 27

[Behnam Esfahbod [:zwnj]]

Comment on attachment 315527 [details] [diff] [review]
patch v12, use PR_LoadLibrary and PR_FindFunctionSymbol (both just once in constructor)

Asking approval1.9, as we had bidi-caret support in 1.8 (with some bugs for Hebrew) and there's no support on 1.9 right now.

Thanks.

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315527 [details] [diff] [review]
patch v12, use PR_LoadLibrary and PR_FindFunctionSymbol (both just once in constructor)

a1.9=beltzner

Comment 29

[:Gavin Sharp [email: [e-mail]]]

mozilla/widget/src/gtk2/nsBidiKeyboard.cpp 	1.9
mozilla/widget/src/gtk2/nsBidiKeyboard.h 	1.5