GNOME Bugzilla – Bug 154039
gedit cannot determine Japanese input when autoindent is enable.
Last modified: 2005-01-24 11:42:12 UTC
Enter key cannot be effective when autoindent is enable and IM is open. To reproduce: 1. Invoke gedit on Japanese locales. 2. Choose [Edit] -> [Preferences] and launch [Preferences] dialog. 3. Select [Auto Indent] in categories and enable [Enable auto indentation]. 4. Input some space or tab chars. 5. Input 'Shift+Space' to change Kanji-mode and input any Japanese. Then cannot determine Japanese input but the cursor starts a next line. I attached the patch
Created attachment 32060 [details] [review] patch for gtksourceview.c I attached the patch.
I don't know the IM stuff very well... could you please add some comments to your patch? Note that you are using a coding style different from the one used in gtksourceview. Please, use the gtksourceview style in your patch. gustavo: what do you think?
When changed to Kanji-mode and input any chars, preedit window has the forcus and shows those chars. When input return key, preedit window decide the chars and send chars to gedit. However this problem gtksouceview returns TRUE before preedit window accepts return key so preedit window cannot decide the chars. To resolve this, if IM is on, return FALSE and the preedit window receive return key. Could you tell me gtksourceview style? I would like to modify this.
I'm sorry. This patch has a few problems so once I close this bug and after I revise the patch, I'll reopen this bug. Thanks much for your evaluation.
In the previous days, I close this bug but now I have the good patch so reopen this. I'm attaching the patch.
Created attachment 35612 [details] [review] patch for gtksourceview.c
Hi Takao, in your patch you are modifying a non public field and as you can see by the attached IRC conversation I had with Mathias and Owen (of the gtk+ team) this is not a good idea. At the same time, it seems there is not a better solution ATM. So before committing I'd like to make a try. Could you please try to reset the im_context directly instead of setting the im_need_reset parameter? Does it work as expected? If it does not work, I will commit your patch by adding a FIXME comment with a reference to bug #163251. <paolo> mclasen: hi. May you please comment on patch for bug #154039. Is need_im_reset in GtkTextView public? <deadchip> paolo: nothing that's neither a GObject property or settable trough a function call is really public <mclasen> paolo: I think owen would be more qualified to comment on im issues, but I can take a look <paolo> mclasen: thanks <owen> paolo:Please don't commit that <owen> paolo: Writing to fields of an object structure is *NEVER* acceptable <paolo> owen: ok, I had the same impression. Any idea on how to fix it? <mclasen> owen: GtkTextView derivatives probably need some way to set that flag <owen> paolo: Not a lot, really. At least without GtkTextView changes <owen> Maybe that patch is OK as a temporary thing.... though certainly don't commit it without a GTK+ bug open to allow doing beter --- owen is now known as o_lunch <paolo> owen: mclasen: what about resetting the im_context direcly instead setting the flag? <o_lunch> paolo: You could propose it to the bug reporter, not sure if it gives the right behavior. <paolo> hmmm... ok, but in that way the preedit state will be cleared <paolo> so I don't think it will gives the desired behavior
Hi Paolo, I have the two way to resove this issue. One is to set im_need_reset and it is already attached patch. Another is to have the flag in GtkSourceView. I'm attaching the patch.
Created attachment 35714 [details] [review] patch for gtksourceview.c This is a callback version without need_im_reset.
Comment on attachment 35714 [details] [review] patch for gtksourceview.c Hi, thanks for the new patch. Could you please add some comments to the code? Have you tested it properly? I am not a pre-edit user so I don't know how to test it in the proper way. Any suggestion on how to test it (note that I have not japanese locale installed and I have an italian keyword)? Few comments follow: > GtkSourceBuffer *source_buffer; > gint old_lines; >+ gboolean imflag; Add a space before imflag. What does imflag represent? Please add a comment. >+static void >+im_preedit_changed_cb (GtkIMContext *im_context, gpointer data) >+{ >+ GtkSourceView *view = GTK_SOURCE_VIEW (data); >+ if (!view->priv->imflag) >+ view->priv->imflag = TRUE; >+ else { >+ gchar *str; >+ gint pos; >+ gtk_im_context_get_preedit_string (im_context, &str, NULL, &pos); >+ if (str && str[0] == '\0') { >+ view->priv->imflag = FALSE; >+ } >+ if (str) >+ g_free (str); >+ } >+} It is not clear what you are trying to do here? If I have read the code well, imflag is used to know if pre-edit is active or not. If this is true... why aren't you using "preedit-start" and "preedit-end"? Remember that I'm by no means a preedit expert... so my comment may be completely wrong in this case :) hmmm... rereading the code... may be imflag means: "pre-edit is active and there is some pre-edited text". Is this right? Does RETURN work as "commit" while preediting text? BTW, you don't need to check if str != NULL before calling g_free (str), in fact, if mem is NULL, g_free simply returns. >+ gchar *str; Please, set str to NULL. >+ gint pos; >+ >+ gtk_im_context_get_preedit_string (GTK_TEXT_VIEW (widget)->im_context, >+ &str, >+ NULL, >+ &pos); >+ >+ if (str && str[0] != '\0' && view->priv->imflag) >+ { >+ g_free (str); >+ return FALSE; >+ } Please, add a comment to this code. >+ if (str) >+ g_free (str); >+ Don't check for str != NULL. I hope my comments make sense. Thanks again for your patch.
Created attachment 35810 [details] [review] patch for gktsourceview.c This reviced patch 35714.
Created attachment 35811 [details] canna installation This is an installation memo for a Japanese input method.
OK, I added some comments in the code. Please review it. > Have you tested it properly? Yes, I'll commit this patch in Sun internally today. two week later, the patch will be tested by each locale. > Any suggestion on how to test it (note that I have not japanese locale > installed and I have an italian keyword)? Oh, I don't know the Italian input method. I think the easiest way is to install Japanese locale with your Linux distribution. There are XIM and IIIM to use IM. http://www.openi18n.org/modules.php?op=modload&name=Sections&file=index&req=viewarticle&artid=30&page=1 We use ATOKX which is a commercial IM for ja and IIIM. I just attached the installation for canna with XIM. > It is not clear what you are trying to do here? I added comment in the code. > If this is true... why aren't you using "preedit-start" and "preedit-end"? Unfortunatelly both "preedit-start" and "preedit-end" are not called in ATOK IM when preedit is open/close so I use "preedit-changed". > Does RETURN work as "commit" while preediting text? Yes, it is right. > BTW, you don't need to check if str != NULL before calling g_free (str), in > fact, if mem is NULL, g_free simply returns. Thanks for the comment. I revised the point. If I don't reply this two week later, it means the patch is Go.
Comment on attachment 35810 [details] [review] patch for gktsourceview.c Thanks for the new version of the patch. I have only a few comments now. >+ gtk_im_context_get_preedit_string (im_context, &str, NULL, &pos); >+ if (str && str[0] == '\0') { >+ view->priv->imflag = FALSE; >+ } >+ if (str) >+ g_free (str); Do not check for str != NULL, simply use "g_free (str)" without the "if(str)" > > if (indent != NULL) > { >+ gchar *str = NULL; >+ gint pos; >+ >+ gtk_im_context_get_preedit_string (GTK_TEXT_VIEW (widget)->im_context, >+ &str, >+ NULL, >+ &pos); >+ >+ /* if the user inputs RETURN and the preedit area is open, >+ * the string on the area needs to be commited. We send >+ * the status to IM. >+ */ >+ if (str && str[0] != '\0' && view->priv->imflag) >+ { >+ g_free (str); >+ return FALSE; >+ } >+ g_free (str); >+ I'd modify this code in the following way: if (indent != NULL) { /* if the user inputs RETURN and the preedit area is open, * the string on the area needs to be commited. We send * the status to IM. */ if (view->priv->imflag) { gchar *str = NULL; gint pos; gtk_im_context_get_preedit_string (GTK_TEXT_VIEW (widget)->im_context, &str, NULL, &pos); if (str && str[0] != '\0')) { g_free (str); return FALSE; } g_free (str); } ..........
*** Bug 164084 has been marked as a duplicate of this bug. ***
How about to call gtk_im_context_reset() before making any changes on GtkTextView? Here is the patch: --- gtksourceview.c.orig 2005-01-15 08:01:45.000000000 +0900 +++ gtksourceview.c 2005-01-15 08:08:39.000000000 +0900 @@ -1693,6 +1693,11 @@ key = event->keyval; + /* we need to reset before get GtkTextIter, + * or GtkTextIter is not valid */ + if (key == GDK_Return) + gtk_im_context_reset(GTK_TEXT_VIEW(view)->im_context); + mark = gtk_text_buffer_get_insert (buf); gtk_text_buffer_get_iter_at_mark (buf, &cur, mark);
Shouldn't RETURN work as "commit" while preediting text? If I have understand well how gtk_im_context_reset works, in this way you will not be able to use RETURN to commit text if auto indentation is enabled. Am I smoking crack?
Unfortunately Choe's suggestion caused a regression bug. i.e. the preedit are is commited but the cursor also starts a next line. OK, I modified my second patch id=35612. I'm attaching the patch.
Created attachment 36053 [details] [review] modified patch id=35612. the simple patch without accessing the im private member but commiting the preedit string.
Comment on attachment 36053 [details] [review] modified patch id=35612. Hi, > if (indent != NULL) > { >+ /* If the preedit area is open, we need to reset the >+ * im context without starting a next line, then the >+ * preedit area is commited and closed. If the preedit >+ * area is closed, we should start a next line. >+ */ >+ if (gtk_im_context_filter_keypress (GTK_TEXT_VIEW(view)->im_context, event)) Does gtk_im_context_filter_keypress commit the pre-edited text? >+ { >+ gtk_im_context_reset(GTK_TEXT_VIEW(view)->im_context); According to documentation: "gtk_im_context_filter_keypress allows an input method to inteybrnally handle a key press event. If this function returns TRUE, then no further processing should be done for this keystroke". So, why are you calling gtk_im_context_reset? Shouldn't the contex be resetted by gtk_im_context_filter_keypress if needed? >+ return TRUE; >+ } >+ > /* Insert new line and auto-indent. */ > gtk_text_buffer_begin_user_action (buf); > gtk_text_buffer_insert (buf, &cur, "\n", 1); You are leaking "indent". I'd move your code just before "indent" is initialized.
Hmm, there are some different behaviors between korean input method and japanese one. With above patch, imhangul (korean input method) works fine. Korean want the preedit to be commited and the cursor to move down to the next line with RETURN key. Does japanese want the cursor not to move down? To snatch keyevent before the im context process the events makes the GtkTextView have old text that is before the last im context status does not applied. So I think the last im context's status should be applied by gtk_im_context_reset(). Anyway the solution is maybe GtkTextView process a keyevent before making any changes the text buffer.
I made another patch: --- gtksourceview.c.orig 2005-01-15 08:01:45.000000000 +0900 +++ gtksourceview.c 2005-01-16 14:15:06.000000000 +0900 @@ -1710,6 +1710,13 @@ if (indent != NULL) { + if (gtk_im_context_filter_keypress(GTK_TEXT_VIEW(view)->im_context, + event)) + return TRUE; + + /* now the GtkTextIter is invalid, so get iter again */ + gtk_text_buffer_get_iter_at_mark (buf, &cur, mark); + /* Insert new line and auto-indent. */ gtk_text_buffer_begin_user_action (buf); gtk_text_buffer_insert (buf, &cur, "\n", 1);
Created attachment 36089 [details] [review] Modified the patch id=36053
> According to documentation: "gtk_im_context_filter_keypress allows an input > method to inteybrnally handle a key press event. If this function returns TRUE, > then no further processing should be done for this keystroke". So, why are you > calling gtk_im_context_reset? Shouldn't the contex be resetted by > gtk_im_context_filter_keypress if needed? That's right. At first, I misunderstood of the return value of the function key_press_cb so unnecessary patch was created. gtk_im_context_filter_keypress called the commit function internally so we can send %TRUE simply. I think gtk_text_buffer_get_iter_at_mark is not needed. I tested the attached patch with Japanese IM and I'm no problem even if the indent chars include multibyte spaces and singlebyte spaces. So that will be ok. TEST CASE: <tab><space>a<ctrl+space>a<return>aa<return><return> 1. <tab><space> creats indent spaces. 2. Input the ascii 'a'. 3. the <ctrl+space> change IM to Kanji-mode. 4. Input a<return>, then the multibyte 'a' is commited. 5. Input aa<return>, then the multibyte 'aa' is commited. 6. <return> starts the next line.
>I think gtk_text_buffer_get_iter_at_mark is not needed. I tested the attached >patch with Japanese IM and I'm no problem even if the indent chars include >multibyte spaces and singlebyte spaces. So that will be ok. No, for korean input method, gtk_im_context_filter_keypress() add the preedit string on RETURN key, so the text is updated. This make the GtkTextIter invalid. You should get the iter again.
> No, for korean input method, gtk_im_context_filter_keypress() add the preedit > string on RETURN key, so the text is updated. This make the GtkTextIter invalid. > You should get the iter again. I tried to use Korean IM but I could not see any problems. # ps -ef | grep kol sun 26236 26194 0 18:06 ? 00:00:00 httx -if kole sun 26238 26236 0 18:06 ? 00:00:00 htt_xbe -if kole sun 26247 26238 0 18:06 ? 00:00:00 com.sun.iiim.kole.palette sun 26252 26238 0 18:06 ? 00:00:00 com.sun.iiim.kole.option sun 26264 26238 0 18:06 ? 00:00:00 com.sun.iiim.kole.keyboard Could you explain how to reproduce your problem by step?
I didn't test it with iiimf-kole. You may test imhangul (http://kldp.net/projects/imhangul/) or nabi (korean xim http://kldp.net/projects/nabi/) or iiimf-hangul. test case: Press spaces several times to make indent. Then press shift-space to change input mode to krean. Press 'rkskek' and enter. Then the preedit string '°¡³ª´Ù' should be commited and the cursor should move down to next line. install guide of imhangul: download http://kldp.net/frs/download.php/1443/imhangul-0.9.11.tar.bz2 untar, ./configure && make && make install. make will automatically update the gtk.immodules file. then you can see this input module on the input method menu of gtk2 apps. install guide of nabi: download http://kldp.net/frs/download.php/1651/nabi-0.15.tar.gz untar, ./configure --prefix="where you want" && make && make install export XMODIFIERS="@im=nabi"
I used imhangul and I may understand your problem. If gtk_text_buffer_get_iter_at_mark is missed, when input RETURN to commit the string, the cursor does not start a next line. Is it right? The problem does not occur on Japanese IM because when input RETURN we need the commiting only. OK, I mean Japanese IM does not need gtk_text_buffer_get_iter_at_mark but to call the function is no problem. I attached the patch. Please review it.
Created attachment 36156 [details] [review] The patch with gtk_text_buffer_get_iter_at_mark
Ok, now it works fine.
Ok, if you guys agree I'm going to commit the Takao's patch.
May you guys please test if a similar problem exists when pressing the TAB and "Insert spaces instead of tabs" is active?
When I press tab with "Insert spaces instead of tabs" option, spaces are inserted. But auto indentation is not spaces if the indent characters of previous line is not spaces. And if this is the intended behavior, then it works fine :)
What does it happen if you press TAB when the "preedit" is open?
The preedit string is commited and the tab is inserted. And this behavior is what most korean want.
Comment on attachment 36156 [details] [review] The patch with gtk_text_buffer_get_iter_at_mark /me is going to commit this patch.
Thanks for your help. Fixed in CVS HEAD.