GNOME Bugzilla – Bug 265670
Some IM commits reversely on preedit characters
Last modified: 2007-07-27 05:06:28 UTC
Description of Problem: When korean input methods like nabi and hangul LE which commit character while preedit buffer is not empty, the characters which get committed will be inserted after preedit buffer, hence the character is comitted reversely. This will prevent users in korean to use gtkhtml based application. Steps to reproduce the problem: 1. Run evolution -> New mail 2. Turn on korean input method 3. type "rkskek" Actual Results: The characters are comitted as "ek sk rk" Expected Results: The characters should be comitted as "rk sk ek" How often does this happen? everytime Additional Information:
Debugged as follow: - Currently when you enter string into preedit buffer, gtkhtml will create preedit area (a text with black background attr) at the cursor position. - The problem arises when a input method commits a string while preedit buffer is not empty, the string will be committed at the cursor position again - Hence the string is behind of "preedit area" as the preedit area did not move. - When the input method commits another string, it will be at the back of the first committed string
Created attachment 44176 [details] [review] proposed patch - first draft
Attached is the proposed patch. I have tested in CJK input methods and they worked okay with it. Bascially moves the position to do commit before the preedit area, moves the preedit area and jump it back to orginial cursor position.
This patch makes sense to me, committed to the gnome-2-8-devel branch. Thu Sep 16 18:05:52 2004 Owen Taylor <otaylor@redhat.com> * gtkhtml.c (gtk_html_im_commit_cb): Commit text before any preedit text, not after. This is needed to get partial commits to work properly. (#65670, Leon Ho)
I found out a problem on the current implementation is it affects normal non-IM typing coz it uses commit as well. This is a better patch for ignoring the positioning when typing in english. Apologies on this. Test case tested: 1. moving cursor then type 2. return then type 3. insert between characters on a. normal english typing b. korean (partial commit) c. traditional chinese (full commit) d. indic hindi
Created attachment 44210 [details] [review] new proposed patch file the above description
You need to test with other more input methods when you add patches to the upstream. ATOK uim tested?
Nakai - are you saying that the new version doesn't work with those input methods or rather that you think that I personally need to test with every possible input method before I put something into CVS? What I do is evaluate patches based on whether they make sense or not. Does the patch look correct? Does it properly conform to the GTK+ input method model? Now it turns out I missed something about the first patch - that html->priv->im_pre_pos is apparently not kept up to date when there is no preedit. But that has nothing to do with testing across 50 different input methods. There aren't 50 different ways that the GTK+ input method framework works, there is one way. And if an input method doesn't conform to it, then a bug needs to be fixed somewhere - in the input method, in gtkimcontextxim, whereever.
Leon, looking at this in detail, it looks like your patch conflicts with Nakai's patch from bug 262637. (Sorry for missing this the first time .... the ChangeLog entry only described the undo changes, not the change to delete existing preedit text.) I don't think Nakai's patch is completely right ... it assumes that the input method will A) Emit ::preedit-changed again (it's perfectly legal to emit ::commit without changing the preedit string) B) Emit ::preedit-changed *after* ::commit. (It's perfectly legal to emit these signals in the opposite order.) I'll try to come with a new patch.
Created attachment 44236 [details] [review] Rework preedit-changed/commit handling
The patch I just attached is a fairly major rewrite of how preedit-changed and commit are handled. Note the patch needs to be applied after the patch in bug 266206. * I split the the preedit-changed code into a separate remove_preedit() and update_preedit() and called these both from preedit-changed and commit. This should resolve the problems I mentioned above. * I changed the code to insert the preedit string after the selection when both are present, even if the cursor is at the beginning of the selection. This simplified the code, but more importantly, I think revealing to the user that the selection is mark to cursor is wrong. (Yes, GtkHTML, unlike GtkTextView does blink the cursor when the selection is present, but that's pretty hard to see.) * I changed the code to hide the cursor for the combination of preedit + selection; I think showing a wrong cursor or wrong selection is worse than not showing a cursor. * I added fairly extensive comments about how * I removed most of the D_IM() debug statements because they were making the code hard to read. (Personal preference...) I'll do some testing of this myself, but testing with different input methods would be much appreciated.
Created attachment 44237 [details] [review] New attempt, fixes couple of bugs in the last
New patch - Fixes cursor_position being in (despite GtkIMContext documentation) characters - Fixes a reentrancy crash from gtk_im_context_commit(). (Don't think this was new with my patch) I've tested it with IIIMF hangul and Japanese language engines kinput2/XIM default input method Cyrillic (transliterated) input method And it seems to work well with all of them. I'll probably land this in the gnome-2-8-devel branch in the next few days I'm building a version with this patch in Rawhide if Red Hat people want to test.
Please apply this patch. Evolution suddenly crashes without this patch when I input characters with SCIM. I've been using with this patch for three months, I've never met crashes. Thanks.
Evolution without even the above patch just works now with immodule except XIM - the above patch isn't applicable cleanly anymore though, Evolution doesn't work properly with XIM even if that's applied. when I tried with kinput2, it freezes.
Created attachment 81197 [details] [review] updated patch from fedora cvs This is the one from the current fedora packages. Should apply since it's used in the package still.
I dont have much context on accessibility/IM stuff. I dont think I can do a good job in reviewing this patch. Any help in reviewing/verifying this patch is much appreciated.
I just realized that the patch in comment #16 is indeed in Fedora CVS but we're not using it. I have no idea why; this patch pre-dates me. Fedora CVS shows that the patch was disabled some time between Fedora Core 3 and 4, but the ChangeLog in the spec file gives no information about why or even that it was done at all. The corresponding Red Hat bug is here: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=130751 Comment #15 seems to imply that this bug is already fixed. Can someone verify this?
Im not sure, what to do here. Is the bug still valid here? Does the patch fixes the problem? Is the bug still valid in fedora (given that the patch is commented)?
Please resubmit the right patch if the bug is still valid.
I believe this is fixed now. At least I cannot reproduce with current Fedora development - tested with both scim-hangul and nabi. evolution-2.11.5-2.fc8 gtkhtml3-3.15.5-1.fc8 So I think this can be closed.
I have tested this bug and it can't reproduce too. I think it can be closed too.