121257 - gtk2 needs to get hooked up to i18n text input methods

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Christopher Blizzard (:blizzard)]

gtk2 needs to get hooked up to other text input methods.

Comment 1

[Jay Yan]

add Ervin, Katakai to the cc list.

Comment 2

[Brian Yuan]

Add myself to the cc list.

Comment 3

[Jay Yan]

[e-mail] is working on this bug.

Comment 4

[John Sun]

I have completed the basic frame, and can input Chinese to mozilla with gtk2.
I need some time to polish it. Next week I'll upload a patch.

Comment 5

[John Sun]

Attachment: A framework for hook i18n input method

It's very ,very rough. Don't include corresponding Makefile.in

From now, I'll focus on it, give patch frequently

Comment 6

[John Sun]

Attachment: A framework for hooking i18n input method

It's very ,very rough. Don't include corresponding Makefile.in

From now, I'll focus on it, give patch frequently

Comment 7

[John Sun]

Chris,
Could you assign this bug to me? I'm working on it.

Comment 8

[John Sun]

Attachment: 1st patch seeking review

Chris,
Could you pls review the patch, or let a suitable person do that. THX!

This is not a pefect patch, but can work.

Known problems
1. Can't support XIMPreeditPosition or XIMStatusArea due to
   gtk's support on XIM -- gtk2.0.2
2. Can't clear preedit string in a editable widget/control when it lose focus.
3. Sometime caret display is not correct caused by this.

Addtionally, the patch changed gtk2/Makefile.in for 
1. Add a macro USE_XIM
2. Add flag for compiling XIM feedback related to pango

Comment 9

[Christopher Blizzard (:blizzard)]

I've asked Owen Taylor to have a look at this.

However, I'd like to say that I'd rather just leave the IM code in all the time.
 There's no reason to have the USE_XIM ifdefs at all, since I don't think it's
something you can leave out of gtk2.

Comment 10

[John Sun]

Ok, once the i18n code is verified stable enough by tests here (beijing), I'll
remove those USE_XIM ifdefs.

Comment 11

[John Sun]

Owen,
How about my patch? 

Comment 12

[otaylor]

Is there some overview somewhere of how IME works in Mozilla? I'm
having trouble reviewing the patch from lack of knowledge of the
overall infrastructure.

A few small comments:

+//#include <gtk/gtkimcontextxim.h>

gtkimcontextxim.h is not an installed header, so the commenting
out is definitely right...


Going from UTF-8 => locale string => PRUnichar is a little backwards.
Surely it would be better to skip the locale string intermediate.
(Which would seem to allow getting rid of nsGtkIMEHelper.[ch] which
looks like a carry over from the old GTK+-1.2/XIM stuff)

    locale_string = g_locale_from_utf8(utf8_str, -1, &bytes_read,
+                                       &bytes_written, 0);
+
+    destStrLen = 0;
+    uniStrLen = nsGtkIMEHelper::GetSingleton()->MultiByteToUnicode((char*)
+        locale_string, nsCRT::strlen(locale_string), &uniStr, &destStrLen);


I would suggest that gtkimmulticontext.h should be only included from
nsWindow.cpp and nsWindow.h should just include gtkimcontext.h; 
gtkimcontext.h will pull in pango.h so there is no reason to include
pango headers explicitely.

Comment 13

[John Sun]

Attachment: v2

Changes compared to v1 ( (a) is according to Owen's suggestion)
a) Use unicode trasfer function from glib2 instead of internal charset
converter service.
b) Add a work around for a gtk2 IME's bug -- After the ref of GtkIMContext get
to 0, close the TOPLEVEL window cause SIGSEGV error.
   http://bugzilla.gnome.org/show_bug.cgi?id=81651
   This will cause mozilla to core dump. For example, after selecting a
profile.
c) Other trivial polishing.

Owen,
It seems there are no IME related docs, even those overview docs about how IME
works in Mozilla, just source code.
Can you review it again at first? you is good at gtk2 IME, then I'll ask
someone
who is good at mozilla's IME support to review it again.
Thanks!

Comment 14

[John Sun]

Attachment: (.tgz)a small program to trigger the gtk2 IME bug

The gtk2 IME bug is 
http://bugzilla.gnome.org/show_bug.cgi?id=88940

Comment 15

[John Sun]

sorry, there is a error,
In comment #13, the gtk2 bug's url shoudl be
http://bugzilla.gnome.org/show_bug.cgi?id=88940

Comment 16

[John Sun]

kakatai,
Could you help to review my patch? Thanks!

Comment 17

[Masaki Katakai]

Sure. I'll try to build and test the patch.

Comment 18

[Masaki Katakai]

Hi John,

I could not build Mozilla with GTK2. I put
only --enable-default-toolkit=gtk2 but it failed.
Could you telll me how to build, exact operation? 

Comment 19

[John Sun]

katakai,
Could you show me the error msg?
If glib2/gtk2 installed, it should be ok with --enable-default-toolkit=gtk2.

Comment 20

[John Sun]

katakai,
What's going on with you gtk2 build?

Comment 21

[Jay Yan]

katakai, please see bug 156593. that may be helpful.

Comment 22

[Masaki Katakai]

Hi John,

Very sorry for late response,

It is great patch!! I can type japanese on gtk2 Mozilla now.
But often dumps core at destroy of window by X error. I don't
know which Mozilla or GTK2 causes problem but I can talk to
Tajima-san. He is my friend.

Also this implementation is ic-per-widget style. Yes,
as you know we have implemented ic-per-window for gtk
and make that as default style. I'll attach the sample
codes for ic-per-window (most codes from gtk/). Please
evaluate it.

> NS_REINTERPRET_CAST(PangoAttrList*, aFeedback));

The line causes compile error on my environment -
Forte Developer 7 C++ 5.4.

	Error: Cannot cast away constness.

I'm not sure why.

> #ifdef USE_XIM
>     if (!mIsTopLevel && mIMContext && !mFocusChild)
>         gtk_im_context_focus_in(mIMContext);
> #endif

The position of gtk_im_context_focus_in() is now top
of SetFocus() but it's better after call of any
unset focus - such as owningWindow->mFocusChild->LoseFocus();

IMSetTextRange() is working basically but preedit text
can not be drawn with correct attribute when I used
Japanese Input Method - Wnn6 on Solaris. For example,

1. enable IM
2. type some thing
3. hit space
	candidate will be converted
4. hit left arrow key
	candidate will be back to original text and
	reversed text will be shrunk by arrow key.
	however, the operation is not working properly

I'll try to look into this later.

> #ifdef USE_XIM
>     if (mIMContext) {
>         gtk_im_context_reset(mIMContext);
>         if (gRefCount > 1) {
>             g_object_unref(G_OBJECT(mIMContext));
>             gRefCount--;
>         }
>         mIMContext = nsnull;
>     }
> #endif

It would be safe that the position of calling gtk_im_context_reset()
is before any gtk_widget_destroy(). Currently, you call the function
at the bottom of nsWindow::Destroy() but it should be before
gtk_widget_destroy() for safety.

>     case eWindowType_child: {

> #ifdef USE_XIM
>         if (!mIMContext)
>             mIMContext = gtk_im_multicontext_new();

mIMContext is created for every nsWindow of WindowType_child type.
The cost of the creation of mIMContext is not small and it
seems to cause performance regression.

We have implemented ic-per-shell style for Mozilla with gtk
which is that we can share IC per shell window. It is good
for performance, also usability. For example, if we browse
a page for sign up, type something and commit japanese
with IME, hit next button, IC is newly created for the next
page, so users have to hit shift+space/ctrl+space key again
to enable IME.

Let me attach the example codes for ic-per-shell. The most
codes of gtk/ are used.

Comment 23

[Masaki Katakai]

Attachment: ic-per-shell example coes and some changes for my comments

Comment 24

[Kyle Yuan]

mass CCed gtk2 related bugs to [e-mail], remove browser-china-
[e-mail] from the CC list.

Comment 25

[John Sun]

Attachment: v3

According to Katakai's suggestion and other minor changes, many codes are from
Katakai.

Sorry for so late to make the new patch, due to be busy on other issue.

Katakai, you're a real expert. Thanks you very much.
Pls review the patch.

Comment 26

[Jay Yan]

I downloaded and test this patch just now, it works fine for me. I can input
Chinese now.

真好! sorry if you can not read Chinese, just now I wrote in Chinese, the
meaning is "very good!"

Comment 27

[Masaki Katakai]

Hmm, preedit string is not reversed on my environment.

John/Jay, how about on your enviroment?

I have a question, what is "unit" of start_index of
end_index of PangAttr?

		START_OFFSET(count) = aPangoAttr->start_index;
		END_OFFSET(count) = aPangoAttr->end_index;

It seems that when I put japanese a full width character
(2 for bytes, 1 for unicode), these set,

   start_index = 0
   end_index = 3

For 2 fullwidth characters,

   start_index = 0
   end_index = 6

I'm sorry I don't know PangoAttr well but if 1 character is
added, 3 index increased.

The IM attribute should be,

1 character

   start_index = 0
   end_index = 1

2 characters

   start_index = 0
   end_index = 2

We need to convert the start index and end index by using
"character"(unicode) unit here to display IM attribute
properly.

Comment 28

[John Sun]

Hi, Katakai,
Thanks for your reviewing.
My environment is Solaris 9 and redhat linux 7.2.
I'm looking through what you talked about. 

BTW, because gtk2 is not perfect ( especially on IM support), as a comparion,
you can use gtk-demo to see what will happen about that. gtk-demo should comes
along with your gtk2 environment.

Comment 29

[Masaki Katakai]

Yes, I have verified that gtk-demo textwidget can
work properly for underline/reverse attribute.
Japanese IME usually use preedit for many characters,
also using both underline/reverse style at same time.

Comment 30

[John Sun]

All is fine for Chinese enviromment under Solaris9 and Linux.
I think the "unit" of start_index of end_index of PangAttr should be
character(unicode) as Katakai said. 
I just took a look at the source of gtk IM module, the unit of *_index of PangAttr
completely depends on the IM server.
It seems there is some difference between Chinese IM server and Japanese IM server. 

Comment 31

[Masaki Katakai]

I have talked with Tajima-san who is working on GTK2/Pango IM in Sun.
The unit is UTF-8 encoded so 1 character=3 is correct.

We need to *convert* those start_index and end_index from utf-8 to utf-16
bases as well as preedit string.

To get the index based on utf-16, I think using g_utf8_to_utf16
is the easiest way. For example,

int start=0;

 uniStr = g_utf8_to_utf16(aPreeditString+aPangoAttr->start_index,
                         aPangoAttr->end_index - aPangoAttr->start_index,
                         NULL, &uniStrLen, NULL);

 START_OFFSET(count) = start;     // start pos
 END_OFFSET(count) = start + uniStrLen; // end pos

I'll attached the example codes using g_utf8_to_utf16() to get
position. Please evaluate. I've verified that is working on
Japanese IME on Solaris 9.

Comment 32

[Masaki Katakai]

Attachment: use g_utf8_to_utf16() to convert PangoAttr from utf-8 based to utf-16

Comment 33

[John Sun]

katakai,
Thank you at first.
You are rigth -- "The unit is UTF-8 encoded"
I ever took a look at the source as below, but too careless to draw a correct
conclusion, ashamed :(


add_feedback_attr (PangoAttrList *attrs,
		   const gchar   *str,
		   XIMFeedback    feedback,
		   gint           start_pos,
		   gint           end_pos)
{
  PangoAttribute *attr;
// This!  
  gint start_index = g_utf8_offset_to_pointer (str, start_pos) - str;
  gint end_index = g_utf8_offset_to_pointer (str, end_pos) - str;

  if (feedback & XIMUnderline)
    {
      attr = pango_attr_underline_new (PANGO_UNDERLINE_SINGLE);
      attr->start_index = start_index;
      attr->end_index = end_index;

      pango_attr_list_change (attrs, attr);
    }

  if (feedback & XIMReverse)
    {
      attr = pango_attr_foreground_new (0xffff, 0xffff, 0xffff);
      attr->start_index = start_index;
      attr->end_index = end_index;

      pango_attr_list_change (attrs, attr);

      attr = pango_attr_background_new (0, 0, 0);
      attr->start_index = start_index;
      attr->end_index = end_index;

      pango_attr_list_change (attrs, attr);
    }

  if (feedback & ~FEEDBACK_MASK)
    g_warning ("Unrendered feedback style: %#lx", feedback & ~FEEDBACK_MASK);
}

Comment 34

[John Sun]

Attachment: v4

Thank katakai again for his great help.

Make a change according to katakai's idea.
But don't suppose 
a) the value of aPangoAttr->start_index of first segment is 0
b) the value of aPangoAttr->start_index follows that of aPangoAttr->end_index
of last segment.
Deal with the tranfer according to more general condition.

Katakai,
pls review it again.

Comment 35

[Masaki Katakai]

Comment on attachment 100386 [details] [diff] [review]
v4

r=katakai

Verified the patch working with Japanese IME.

Comment 36

[John Sun]

Thank katakai a lot.

Blizzard, pls sr the patch. THX in advacne.

Comment 37

[Christopher Blizzard (:blizzard)]

The code looks good.  One question, though.  Are we ever going to build with XIM
disabled? Can you even do that with gtk2?  I don't think so.  If there's no good
reason to use the USE_XIM define, then let's just put it in the code without the
#ifdef.

Comment 38

[Christopher Blizzard (:blizzard)]

Comment on attachment 100386 [details] [diff] [review]
v4

sr=blizzard

After discussion on irc about the USE_XIM issue.  Let's get this checked in.

Comment 39

[Bolian Yin]

checked in Trunk.

Comment 40

[Christopher Blizzard (:blizzard)]

I've disabled the XIM code on the trunk for now.  Please see bug #176514 for
discussions.