Created attachment 44108 [details] [review]
e-text patch

ichael's Feedback

"I wonder given this change if the code shouldn't just connect to the
signals whenever it sets up the imcontext?

I also notice that the set_property method doesn't un-hook from any
events from the old im_context if it is re-set, which would be a bug.

Maybe set_property should just hook and unhook the signals itself, so
the signal handlers are always setup, since this change will
effectively do the same thing at a different time"

The foolowing patch implements this.

Created attachment 44113 [details] [review]
e-text patch

committed to head

Rolledback patch, this caused the following issue.

In calender adding some words to one appointment's summary, other
appointment's These are the steps to reproduce the issue

Steps:
1. Launch evolution and click local folder->Calendar;
2. New some appointments,at least two;
3. Left click one appointment,input some words to change the summary.

Results:
The other appointments also add some words to summary the same time.
But when 
you focus on other appointments,their summarys didn't change actually.

summary will change.

Created attachment 44125 [details] [review]
e-text patch

Comments from NotZed
"
I don't really like this much, but it may do, other solutions might get
complex.

Can you just keep track of the last canvas item which was given focus,
and apply any events to that only?  Or do you keep getting events even
when some other totally unrelated widget gets focus (in which case this
patch wont work either).

Or can it just have a global imcontext for the widget, and always listen
to it, but just apply its input to the currently active object.

But as i said to start with, if it works it may do."

In short we need to achieve this.
1. Redirect all the im_context callbacks to the text widget which do
have the current focus.
2. If a text widget goes out of focus and is still in the preedit
mode, but with no other text widgets gaining focus, keep the signals
connected.
3. If a new text widget gains focus, disconnect the callbacks from the
old one (either in mode 1 or 2) and connect the signals here.

im_context's and parent of the text widgets can be different, keeping
track of the parent GnomeCanvas does not make sense as out focus is
only the previous text widget which the events were connected to,
irrespective of the parent.

The only way things can go haywire here is if we are getting
consecutive focus in's or consecutive focus out's from different EText
widgets and our original save_text widget thingy gets erased without
disconnecting the im_context signals from it, then in that case even
the original code also won't hold. (such a case should not happen)

I tried some other ways but as you said there does not seem to be any
easier/cleaner way of doing this.

Enough on this. Pl. gimme a go and I will commit this code.

ok, i guess this will do then.

i don't know how the new gnome 2.8 approval crap and branching
interrelates to actually being apply to apply the patch though.

committed to head. Will commit to 2.8 branch if a go is given for that
too.

*** bug 264697 has been marked as a duplicate of this bug. ***

Your fix breaks Japanese input usability.
Revert it, please.

Created attachment 44139 [details] [review]
patch for gal/e-text/e-text.c

Created attachment 44140 [details] [review]
Updated patch, applied for e-text.[ch]

Obsolete: http://bugzilla.gnome.org/attachment.cgi?id=44139
Latest: http://bugzilla.gnome.org/attachment.cgi?id=44140

The patch does:
- Remove preedit_len and add
im_preedit,im_preedit_attrs,im_preedit_cursor to e-text.h
- Fix memory leak in e_text_preedit_changed_cb()
- Reduce the redundant gtk_im_context_get_preedit_string() call
- Remove slow codes from e_text_draw()
- Use gchar* instead of GString
- Remove pango_layout_*() funcs from preedit handling to speed up
- Move gtk_im_context_filter_keypress() funcs to
  GDK_KEY_PRESS section. Because the keypress func
  doesn't make sense in GDK_KEY_RELEASE section.

Did you test with the latest patches ? I don't think anything there
makes the code slower. If it is slow, it was like that before too,
nobody complained before (I'm not a native ja im user to judge of the
response of this code)

Seems like the patch creates another bug for the original issue. Check
the case where the focus goes in out of the commit. Crl-space 'a',
space, space, <select an item from preedit popup>, 'i'. In my tests it
appeared wrong the preedit selection changed the already committed
ewntry too.

In calendar, in week view mode where there are a more than one EText's
when you have more than one summary per day, see all sort of wierd
stuff it do.

Bring up weekly view
Take a day, enter a summary for say 9.00-9.30am
Enter another for next 10-10.30
Enter some text in first in preedit mode, click on the middle of next
one and enter some more.
Press 'Enter', even with the fix for 
http://bugzilla.gnome.org/show_bug.cgi?id=264404

You'll get pango_attr_splice index error etc if you play around with this,

A couple of good things about your patch.
fixes a mem leak in preedit cb.
have sort of right approach, may be if you work on this some more it
will solve the above issues too, if a modified version of this patch
makes e-text better I'm all for this. But this version unfortunately
does not do it.

I have a slew of other stuff to look upon. I will happily remove my
patch if your one addresses these issues too.

Which input method are you using for this test?
Input method implementations are various even in immodule.
And input method operations are various too.
I want to build up detail set of Japanese input method
test environment with sane scientific steps, which
even non-native speakers can judge and accept the
patch. Engineering is not poem, so I believe we can do.

I'm using im-canna and im-xim(kinput2) but 'a <space> <space> i' works
and your patch doesn't change in my
environment.

Nobody could not point out the problem for your
patch. Because it is the first patch to let preedit
string show up, the change is not yet in release version,
and Evolution itself is still not recommended in Japan.

I'll see the issues you comment.

I'm using IIIM input method server with atok12, may be there's a
performance difference for the LE too (or I got a fast machine) Not sure.

Don't think atok12 is free, may be I can ask our input person here
more on setup/test cases etc.

Hope you can reproduce the issues with the calender weekview as I have
said above.

Optimizing open-source software Evolution to
the proprietary software ATOK doesn't make sence. 

Please test with other open-source Japanese input method
on your environment. Input long Japanese sentence to get
multiple segments and move to prev/next segment with
right/left key.

I'll get atok and test it.
Not atok-x but atok12 ?

I think atok12 had problem with glibc-2.2 or later and
they didn't fix it. Hm.

Well atok12/atok-x are basically the same the later being the Solaris
port for the same version in linux. As I said before I woild love to
test this with canna etc. but am very time crunched.

If you're native ja user it'll be easy for you to create some test
cases urself (which non native people can also run) for atokx, pl.
check that you cover the cases when multiple e-text widgets which are
childern of the same parent gnomecanvas is also taken into account as
in calender scenatios as I have written, you can also test it using
day/month views apart from what I said before. If this works well with
opensource/atok and with single/multiple instance of e-text then we
have a good enough implementation.

What determines the success of an app like eolvution is to what extent
it can lure windows users, so in addition to do testing with free
ims/LEs, we also need to test with atokx which was originally an input
method in windows environment, got ported to *nix'es later.

All GNOME apps should work for the proprietary software
too, I agree. GNOME aims the powerful platform.

But this bug and the evolution bug of your
http://bugzilla.gnome.org/show_bug.cgi?id=264404
don't happen on open source input method, with gal CVS
HEAD and evolution CVS HEAD. Those are supposed as
ATOKX specific bug. (Not yet decided, though.)

The community should not skew open source software
for the proprietaries. For ATOKX bugs, we cannot fix
ATOKX source codes so we need to discuss with them and
ask them to fix if they're wrong. We should not get
rid of their chances to fix ATOKX bugs. And the Linux
distributors do not need to push ATOKX specific fix
to the upstream, because they have chances to add
patches to their products localy.

So let's check which is wrong for this issue,
ATOKX or other.

In gal/e-text widget, this bug is not an issue with atokx, all other
apps/widgets in gnome work well with this scenario except e-text
widget, so fix should be in e-text.

I think the evolution issue will happen with other input method too as
'Enter' events are blocked from sending downstream, so commit is
having problems. Pl. double check on this one using free input methods.
 

>In gal/e-text widget, this bug is not an issue with atokx, all other
>apps/widgets in gnome work well with this scenario except e-text
>widget, so fix should be in e-text.

I disagree. Atokx might have weird bugs which will happen
only on the complex widgets like Evolution.

Now we need to compare two codes for gal.
- current gal CVS HEAD (your code)
- current gal CVS HEAD and my patch (my code):
http://bugzilla.gnome.org/attachment.cgi?id=44140

But I still cannot catch where is the wrong.
I added such code.

Index: e-text/e-text.c
===================================================================
RCS file: /cvs/gnome/gal/gal/e-text/e-text.c,v
retrieving revision 1.158
diff -u -r1.158 e-text.c
--- e-text/e-text.c	30 Aug 2004 17:58:34 -0000	1.158
+++ e-text/e-text.c	6 Sep 2004 09:34:11 -0000
@@ -1368,6 +1368,8 @@
 	GnomeCanvas *canvas;
 	GtkWidget *widget;
 
+g_print("e_text_draw\n");
+
 	text = E_TEXT (item);
 	canvas = GNOME_CANVAS_ITEM(text)->canvas;
 	widget = GTK_WIDGET(canvas);
@@ -2275,6 +2277,7 @@
 			gint ret;
 
 			if (text->im_context && gtk_im_context_filter_keypress
(text->im_context, (GdkEventKey*)event)) {
+g_print("filter_keypress\n");
 				text->need_im_reset = TRUE;
 				return 1;
 			}

Your code shows like this:
e_text_draw
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
e_text_draw
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
filter_keypress
...

But my code is like this:
e_text_draw
filter_keypress
e_text_draw
filter_keypress
e_text_draw
filter_keypress
e_text_draw
filter_keypress
e_text_draw
filter_keypress
...

I'd like to wait Atokx comes to my desktop for detail
compare test.

Atokx candidate window focus bug was reported 2 years ago.
Hm.

http://famao.tdiary.net/20021023.html (Japanese)
http://www.momonga-linux.org/~famao/libatokxhack-0.1-1m.src.rpm

The document is saying the bug depends on window manager's
behavior.

Now ATOK-X came on my desktop.

Let me confirm your environment:

Your ATOKX works through IIim immodule(im-iiim.so),
or through XIM immodule(im-xim.so)?

You can check it with right click menu ->[Input Methods].

Created attachment 44383 [details] [review]
Updated patch

ATOKX seems to have some focus issue which should be called
as a bug or some inconsistency to GNOME desktop. The above
libatokxhack module really fixes it. And without it,
ATOKX sends preedit and commit events in amazing order.

The above updated patch now handles such ATOKX behavior.
It works with your a, ' ', ' ', i test case and still
doesn't effect to the other open source input methods.

The current gal CVS code causes slowness on ATOKX too.
Input 'se' several tens of times and press space to convert
and you'll get many kanji clauses. Use Shift+Right and
Shift+Left to move forward and backward, and I'm sure
you'll feel it is really slow.

I understand why this slowness happens. The CVS gal code
inserts preedit text and calculate positions in each time that
e_text_draw() is called. But insert_preedit_text()
takes costs more than the usual e_text_draw() time,
so some e_text_draw() calls are just queued and not executed.

Hm, I've got the time for the proof.

With my patch, e_text_draw() function returns
around 100 micro seconds. But current gal code
costs around 130-150 micro seconds for insert_preedit_text()
in e_text_draw().

adding patch keyword

adding perf and memory keywords

any news on this?
i guess this is still valid for evo 2.6?

(In reply to comment #0)
> 2. Activate "To:" field, input 'Ctrl + Space' and change to Kanji-mode.

This does not exist anymore nowadays.
Input Methods available in 3.2 on Fedora16 are:
* Simple
* Cedilla
* IBus
* X INput Method

What are the steps nowadays to reproduce this?

Closing this bug report as no further information has been provided. Please feel free to reopen this bug if you can provide the information asked for.
Thanks!