Created attachment 13172 [details] [review]
imxim patch for proper Thai XIM openning

Sorry, the symptom was not a crash. (It did crash in
2.1.0, but it had been fixed before 2.1.3.)
Rather, it's about dysfunction.

With my second proposal regarding the additional callback,
I've tried hacking out an experimental patch as attached.

Created attachment 13186 [details] [review]
experimental patch for StringConversionCallback

With Thai XIM, what are returned when GtkIMContext queries a list of
supported input styles? If it has XNPreeditCallback, then
GtkIMContextXIM
is doing a right thing, otherwise, there may be something wrong with
chooing the input style either in the setup_styles() or in the 
choose_better_style()?

As to setting XNStringCallback, it will be perhaps better simply call
XSetICValues () without checking the locale name.
  - It is not true to judge from locale name - not all platforms has
     thai XIM implementation with XNStringCallback. 
  - The XSetICValues() call will return XNStringCallback if it is not
     supported by the XIM

[If we really want to check before calling XSetICValues, we may want
to chek the supported ic values(XNQueryICValuesList) to see if 
XNStringCallback is included or not, but I doubt that it is very
trustworthy
with all XIM implementations.]
  

The supported styles returned are
- 0x810 (PreeditNone|StatusNone)
- 0x408 (PreeditNothing|StatusNothing)


About the locale checking, yeah, my patches were dirty hacks,
just to show "if" it's forced like this, it works.
Please feel free to adjust to what you think suitable.

It's weird that when I change the code back (remove the locale
checkings), it still works. A similar confusion had once
happenned when I first added the locale checkings. GTK+
appeared stubborn to change and I had to enforce with
stronger condition (i.e. in he setup_style() itself) before
it changed.

So, I wonder if there is any saving to status file regarding
the preedit-style and status-style properties.

If not, it may be my own confusion.

In addition, I've found two more defects in the callback
function in my last patch:
- In case ForwardChar, q is not updated in the for loop.
  It should be q = g_utf8_next_char (q);
- In case BackwardChar, the string (text or surrounding) 
  should be reversed (according to the protocol).

I've finished the summarized patch for the callback and for 
making Thai uses imxim by default.

To test the patch with Thai XIM, you need another patch
for XFree86 which adds capability to correct the input
sequence to the XIM (will be attached later).

Created attachment 13303 [details] [review]
Summarized patch for the callback and for making Thai uses imxim by default

Created attachment 13304 [details] [review]
XFree86 patch for testing Thai XIM (xfree86-assigned sequence no. = 5553)

Note: The patches were based on Hideki Hiura's explanation on
the actual semantics of StringConversionCallback. Many thanks
to him.

FYI: I find XFree86 has accepted the patch to its CVS.

the crash/misbehavior you were seeing is almost certainly
a dup of bug 103549, where the selected input style would
depend on the contents of unitialized memory.

Patch review comments:

 - 'if (i > 0) break;' should be broken onto two lines
 - You need to handle failure of g_locale_from_utf8(); it's
   quite likely the text in the widget won't be representable
   in the locale encoding.
 - You need to handle failure of malloc().
 - The algorithm for getting the range really doesn't
   look right to me. I've mailed xfree86-i18n and 
   Hideki Hiura for additional clarification.

Thank you for the patch review.

> - 'if (i > 0) break;' should be broken onto two lines

Done.

> - You need to handle failure of g_locale_from_utf8(); it's
>   quite likely the text in the widget won't be representable
>   in the locale encoding.

It's already handled by the "if (text)" block after the
"switch (conv_data->direction)" block. Any failure will be blocked here.
(Comment added.)

> - You need to handle failure of malloc().

Done.

> - The algorithm for getting the range really doesn't
>   look right to me. I've mailed xfree86-i18n and
>   Hideki Hiura for additional clarification.

Waiting for response.

Created attachment 13792 [details] [review]
string conversion callback polishing

I propose another patch that summarizes
all changes, plus XIM protocol fixing, and
querying the IM values before setting the
callback.

The XIM protocol part, however, depends on
signness of the XIMStringConversionPosition
type in Xlib.h, for which a patch is under
submission to XFree86. This may need
confirmation from X11 as well.

Created attachment 14473 [details] [review]
The summarized patch against GTK+ 2.2.1

While waiting for the response about the
protocol change, which may take several
steps, I propose a pretty feasible way for
Thai input method: raise the priority of
Bug #81031, in which some patches for
imthai have been proposed.

Thep, is your patch still correct according to whatever has been
decided about the protocol? How can it be tested?

There has been no response regarding the XIMStringConversionPosition
signedness so far. So, only positive values are safe for the time being.

However, I think trying to cover it won't hurt. I have worked out another
patch against CVS head, with comment around the code in question added.
The code block for negative cases shall never be activated until the X
protocol is fixed. (Until then, XIM servers should not issue negative 
positions at all.)

For testing, just set locale to th_TH, with XMODIFIERS="@im=BasicCheck".
Of course, XKB map needs to be "th", e.g. "setxkbmap us,th -option 
grp:alt_shift_toggle". With XFree86 4.3.0, the Thai XIM should be able
to check & correct Thai input sequences. For example, this is an invalid
sequence:
[THO THAHAN] + [MAI EK] + [SARA II]
[U+0E17] + [U+0E48] + [U+0E35]
qwerty key strokes for th group: mju
SARA II should be inserted between THO THAHAN and MAI EK.

Created attachment 24546 [details] [review]
A proposed patch against CVS head (2004-02-16 snapshot)

Looks pretty good; what I saw looking at the patch was:

 - A couple of formatting problems (no space in "function("
 - I think your patch leaked ic_values in 
   set_string_conversion_callback. Since we already 
   getting the same values when we open the IM initially,
   we might as well do the check only once there.
 - Without explicit casts to gshort for the position,
   GCC will optimize out things like:

    conv_data->position < 0

   because it knows it is positive.

I made a few changes to clean the above up; could you
test the new patch? If it works for you, we'll try to get
it into a 2.4.x dot release.
   

Created attachment 25548 [details] [review]
My version of patch

Thank you for the patch review.
I have tested your patch and it works fine.

Patch tested. Should it be committed?

Owen already basically gave a commit-after-testing approval. Since you say it
works ok, we should get it in, ideally before 2.4.1.

So, I have committed Owen's patch to the HEAD. Is there other branch to commit?

--- ChangeLog   23 Apr 2004 20:01:53 -0000      1.5348
+++ ChangeLog   24 Apr 2004 09:51:57 -0000
@@ -1,3 +1,22 @@
+2004-04-24  Theppitak Karoonboonyanan  <thep@linux.thai.net>
+
+       Patch to add support for string conversion callbacks to
+       GtkIMContextXIM (#101814)
+
+       * modules/input/gtkimcontextxim.c: Set the string conversion callback
+       if supported by the XIC.
+
+       (struct _GtkIMContextXIM): Add string_conversion_callback member.
+
+       (struct _GtkXIMInfo, setup_im): Check and keep flag inidicating
+       whether string conversion callback is supported.
+
+       (gtk_im_context_get_ic, +set_string_conversion_callback,
+       +string_conversion_callback): Also initialize string conversion
+       callback, if supported, along with the IC initialization.
+
+       * modules/input/imxim.c: Make "xim" module default for Thai as well.
+
 2004-04-23  Matthias Clasen  <mclasen@redhat.com>

        * gtk/gtkclipboard.c (gtk_clipboard_wait_for_targets): Correctly

Mark bug as fixed.