After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 154039 - gedit cannot determine Japanese input when autoindent is enable.
gedit cannot determine Japanese input when autoindent is enable.
Status: RESOLVED FIXED
Product: gtksourceview
Classification: Platform
Component: General
unspecified
Other All
: High major
: ---
Assigned To: GTK Sourceview maintainers
GTK Sourceview maintainers
: 164084 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2004-09-29 09:53 UTC by Takao Fujiwara
Modified: 2005-01-24 11:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch for gtksourceview.c (1.33 KB, patch)
2004-09-29 09:54 UTC, Takao Fujiwara
none Details | Review
patch for gtksourceview.c (534 bytes, patch)
2005-01-07 14:18 UTC, Takao Fujiwara
none Details | Review
patch for gtksourceview.c (2.38 KB, patch)
2005-01-09 11:47 UTC, Takao Fujiwara
needs-work Details | Review
patch for gktsourceview.c (3.75 KB, patch)
2005-01-11 07:26 UTC, Takao Fujiwara
needs-work Details | Review
canna installation (2.76 KB, text/plain)
2005-01-11 07:28 UTC, Takao Fujiwara
  Details
modified patch id=35612. (781 bytes, patch)
2005-01-15 12:01 UTC, Takao Fujiwara
needs-work Details | Review
Modified the patch id=36053 (697 bytes, patch)
2005-01-16 09:03 UTC, Takao Fujiwara
needs-work Details | Review
The patch with gtk_text_buffer_get_iter_at_mark (774 bytes, patch)
2005-01-18 07:18 UTC, Takao Fujiwara
accepted-commit_now Details | Review

Description Takao Fujiwara 2004-09-29 09:53:23 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
Comment 1 Takao Fujiwara 2004-09-29 09:54:34 UTC
Created attachment 32060 [details] [review]
patch for gtksourceview.c

I attached the patch.
Comment 2 Paolo Maggi 2004-09-29 10:13:56 UTC
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?
Comment 3 Takao Fujiwara 2004-09-29 12:35:34 UTC
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.
Comment 4 Takao Fujiwara 2004-12-01 02:24:20 UTC
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.
Comment 5 Takao Fujiwara 2005-01-07 14:15:58 UTC
In the previous days, I close this bug but now I have the good patch so reopen this.

I'm attaching the patch.

Comment 6 Takao Fujiwara 2005-01-07 14:18:23 UTC
Created attachment 35612 [details] [review]
patch for gtksourceview.c
Comment 7 Paolo Maggi 2005-01-07 17:26:59 UTC
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
Comment 8 Takao Fujiwara 2005-01-09 11:44:16 UTC
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.


Comment 9 Takao Fujiwara 2005-01-09 11:47:05 UTC
Created attachment 35714 [details] [review]
patch for gtksourceview.c

This is a callback version without need_im_reset.
Comment 10 Paolo Maggi 2005-01-09 23:21:03 UTC
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.
Comment 11 Takao Fujiwara 2005-01-11 07:26:29 UTC
Created attachment 35810 [details] [review]
patch for gktsourceview.c

This reviced patch 35714.
Comment 12 Takao Fujiwara 2005-01-11 07:28:08 UTC
Created attachment 35811 [details]
canna installation

This is an installation memo for a Japanese input method.
Comment 13 Takao Fujiwara 2005-01-11 07:47:17 UTC
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 14 Paolo Maggi 2005-01-11 09:04:26 UTC
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);
    }

    ..........
Comment 15 Paolo Maggi 2005-01-14 16:36:09 UTC
*** Bug 164084 has been marked as a duplicate of this bug. ***
Comment 16 Choe Hwanjin 2005-01-14 23:26:40 UTC
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);
Comment 17 Paolo Maggi 2005-01-15 11:02:03 UTC
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?
Comment 18 Takao Fujiwara 2005-01-15 11:58:47 UTC
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.
Comment 19 Takao Fujiwara 2005-01-15 12:01:46 UTC
Created attachment 36053 [details] [review]
modified patch id=35612.

the simple patch without accessing the im private member but commiting the
preedit string.
Comment 20 Paolo Maggi 2005-01-15 16:40:09 UTC
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.
Comment 21 Choe Hwanjin 2005-01-16 04:56:39 UTC
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.
Comment 22 Choe Hwanjin 2005-01-16 05:17:00 UTC
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);
Comment 23 Takao Fujiwara 2005-01-16 09:03:25 UTC
Created attachment 36089 [details] [review]
Modified the patch id=36053
Comment 24 Takao Fujiwara 2005-01-16 09:20:15 UTC
> 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.

Comment 25 Choe Hwanjin 2005-01-16 14:41:11 UTC
>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.
Comment 26 Takao Fujiwara 2005-01-17 09:17:18 UTC
> 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?
Comment 27 Choe Hwanjin 2005-01-17 10:11:17 UTC
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"
Comment 28 Takao Fujiwara 2005-01-18 07:16:27 UTC
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.
Comment 29 Takao Fujiwara 2005-01-18 07:18:36 UTC
Created attachment 36156 [details] [review]
The patch with gtk_text_buffer_get_iter_at_mark
Comment 30 Choe Hwanjin 2005-01-18 14:15:14 UTC
Ok, now it works fine.
Comment 31 Paolo Maggi 2005-01-21 13:52:19 UTC
Ok, if you guys agree I'm going to commit the Takao's patch.
Comment 32 Paolo Maggi 2005-01-21 13:57:55 UTC
May you guys please test if a similar problem exists when pressing the TAB and
"Insert spaces instead of tabs" is active?
Comment 33 Choe Hwanjin 2005-01-21 14:38:09 UTC
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 :)
Comment 34 Paolo Maggi 2005-01-21 15:15:42 UTC
What does it happen if you press TAB when the "preedit" is open?
Comment 35 Choe Hwanjin 2005-01-21 15:26:03 UTC
The preedit string is commited and the tab is inserted. And this behavior is
what most korean want. 
Comment 36 Paolo Maggi 2005-01-23 20:27:57 UTC
Comment on attachment 36156 [details] [review]
The patch with gtk_text_buffer_get_iter_at_mark

/me is going to commit this patch.
Comment 37 Paolo Maggi 2005-01-24 11:42:12 UTC
Thanks for your help.

Fixed in CVS HEAD.