57332 - Quick typing erases a converting text

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P3)

Description

[Koike Kazuhiko]

Quick typing erases a converting text.	

Steps to reproduce:
(1) Turn on IME.
(2) Type a text and hit space key to convert it.
(3) Type a next text quickly, before the previous text is converted
and highlighted. The previous text disappears.

This bug was reported on Japanese bugzilla.
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=320

Comment 1

[nhottanscp]

Roy, please take a look at this.

Koike san, is this a window specific problem?

Comment 2

[Koike Kazuhiko]

I can reproduce this on Win98 and WinNT.

kinput2 on Linux converts a text very quickly. I can't type a text
before a converted text appears.

Comment 3

[Teruko Kobayashi]

Koike san, 
I cannot reproduce this by just following the steps of reproduce 
in today's Win32 build on Win98J. In the original Japanese bugzilla, 
the reporter did something, then, this could be reproduciable.

If you can reproduce this, please give us more detail.
Thanks

Comment 4

[Koike Kazuhiko]

Slow typing sometimes reproduces this problem, but I can't find steps to
reproduce.

Comment 5

[Koike Kazuhiko]

Two people besides me reproduced this bug.
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=320

Comment 6

[Asa Dotzler [:asa]]

setting bug status to New.  kazhik, I've updated your bugzilla permissions.  
Please email me if you have any questions.

Comment 7

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 8

[Roy Yokoyama]

Updating the target milestone.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This bug is able to reproduce on 2002060804/Windows2000 and XP.

On Desktop PC(PentiumIII 1GHz, Memory 768Mbyte, Windows2000).
It isn't able to reproduce when daily use, 
But It is able to reproduce when Very Heigh bit-rate movie play on MediaPlayer.

*(Heigh bit-rate movie...640x480x24bit,MPEG1(CQ),23.97frames/sec,44.5Mbyte/90sec)

On Notebook PC(Cerelon 650MHz Memory 256Mbyte, WindowsXP).
It is able to reproduce *always*.(Slowly typeing is'nt able to reproduce.)

I think, getting IME returned value thread and editor's thread are same?
On Japanease system, this problem is *Major* bug.
This problem is very displeasure.(T_T)

Comment 10

[Roy Yokoyama]

setting a new milestone.

Comment 11

[Roy Yokoyama]

Nakano-san/Koike-san:  our iQA thinks it's related to 106 Japanese keyboard.  
Which keyboard
              do you use to reproduce this bug?

Comment 12

[Koike Kazuhiko]

My keyboard is 106.

Comment 13

[Roy Yokoyama]

ok, i got 106 Japanese keyboard and driver installed ; but I still failed to
reproduce this bug
on my desktop (W2K-Ja, 0.5 GB Ram, 800 MHz).  Japanse bugzilla indicates that it's 
often reproducible on a *CPU* intensive machine.    What do I need to do?  I
understand
number of Japanese users are frustrated by this bug.  I want to fix this as much
as they do.

Comment 14

[Frank Tang]

major user feedback for m1.0
nsbeta1+ and adt2.
reassign to shanjian. roy is too busy for TSF and unicode app stuff. 
shanjian- can you reproduce this ?
Is this a IME performance issue or a threading issue. Can this be solve by
adding some lock or queue mechanism?

Comment 15

[Koike Kazuhiko]

Kamio-san made a patch for this bug.
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=320

Kamio-san, attach your patch here.

Comment 16

[Shanjian Li]

I took a look at the patch in bugzilla jp, I thing the patch identifed the
problem. But I have difficult reading japanese. So could somebody post the patch
here and give me some explaination of what's happening? There is one thing we
need to worry about, that is the possible side-effect of this patch. 

Comment 17

[Shotaro Kamio]

The following is our diagnosis.
The main problem is the order of processing messages.

When we hit keys, WM_KEYUP and WM_KEYDOWN messages are sent.
When we hit space key to convert to kanji, IME sends WM_IME_COMPOSITION message
to bring us converted chars.

Ordinary message order is like this:
WM_KEYDOWN
(hit space key)
WM_IME_COMPOSITION
WM_KEYUP
WM_KEYDOWN (hitting next chars)

Quick typing case is processed in Mozilla like this:
WM_KEYDOWN
(hit space key)
WM_KEYUP
WM_KEYDOWN (this is next chars message)
WM_IME_COMPOSITION
In this case, the composition string becomes zero length string.
So, converted chars disapper.
( The composition string means strings we get from 
NS_IMM_GETCOMPOSITIONSTRINGW(hIMEContext, GCS_RESULTSTR,...)
in nsWindow.cpp)

Current implementation of nsAppShell.cpp for windows is as follows.
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsAppShell.cpp#110
110     // Give priority to system messages (in particular keyboard, mouse,
111     // timer, and paint messages).
112      if (::PeekMessage(&msg, NULL, WM_KEYFIRST, WM_KEYLAST, PM_REMOVE) ||
113          ::PeekMessage(&msg, NULL, WM_MOUSEFIRST, WM_MOUSELAST, PM_REMOVE) || 
114          ::PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {

This gives the priority of processing WM_KEY* message.
The behavior mentioned above is caused by this code.

In our opinion, this prioritizing does not bring us large performance
improvement. It could be much little.
We suggest to remove this prioritizing.

I'll post new patch.

Comment 18

[Shotaro Kamio]

Attachment: a patch to erase prioritizing

Comment 19

[Shanjian Li]

Shataro Kamio,
Thanks a lot for providing the patch and explaination. Your analysis of the
problem is correct, but your patch might cause some regression. Jst's check in
v3.31 for bug 36849 is one of the reasons. I would suggest a more conservative
approach. That is to use "WM_IME_KEYLAST" to replace "WM_KEYLAST". Could you try
it on your machine? I could not reproduce the problem on mine. 

Comment 20

[Shotaro Kamio]

Shanjian's idea (use "WM_IME_KEYLAST" to replace "WM_KEYLAST") performes well in
our tests.
One things to worry is that the values of messages have small leap.
The value 0x109 - 0x10C are not used now.
We don't know this is reverved for future use or not.
Except this worry, this idea is good.

We have an idea to treat the worry above.
This is to give the priority to IME messages like this:
------------
     // Give priority to system messages (in particular keyboard, mouse,
     // timer, and paint messages).
-     if (::PeekMessage(&msg, NULL, WM_KEYFIRST, WM_KEYLAST, PM_REMOVE) ||
+     if (::PeekMessage(&msg, NULL, WM_IME_STARTCOMPOSITION, WM_IME_KEYLAST, 
+PM_REMOVE)||
+         ::PeekMessage(&msg, NULL, WM_KEYFIRST, WM_KEYLAST, PM_REMOVE) ||
          ::PeekMessage(&msg, NULL, WM_MOUSEFIRST, WM_MOUSELAST, PM_REMOVE) || 
          ::PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {
------------
This change, also, performes well in our tests.

Unfortunately, IME Messages has one noticeable and bad thing.
It is that the values of messages are not continuous!
  WM_IME_STARTCOMPOSITION(0x010D) - WM_IME_KEYLAST(0x010F)
  WM_IME_SETCONTEXT(0x0281) - WM_IME_KEYUP(0x0291) 
We tried to process these messages in the same priority by PeekMessage().
But it seems impossible.
As the result, our idea above is compromised one.
Though our idea works well now, we couldn't say this cause no bugs.
Shanjian's idea also.
(Because some IME messages are not handled well.)

We couldn't clearly decide which idea we should do.
Do you have any idea, Shanjian?

Comment 21

[Shanjian Li]

Attachment: patch

Comment 22

[Shanjian Li]

Shotaro Kamio,

I just posted a patch. The gap of 0x109 - 0x10C is not a concern for me. Even
they do put some new message there, there should not be no harm if we process
them by order. The other range of IME message is a problem. Besides other
messages, we did use WM_IME_CHAR. My patch addressed this problem. Could you try
it on your machine? 

Comment 23

[Shotaro Kamio]

Shanjian, your patch works good. Thanks.
In addition, line #179 of nsAppShell.cpp has same problem.
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsAppShell.cpp#177
Please fix it also.
And then, review and check-in :-)

Comment 24

[Shanjian Li]

Shotaro Kamio,
Would you mind posting a complete patch here? In that case, I can review the
code and all we need is a sr. That can make things go faster.

Comment 25

[Shotaro Kamio]

Attachment: Proposed patch

Here is the complete patch.

Comment 26

[Shanjian Li]

Comment on attachment 93324 [details] [diff] [review]
Proposed patch

r=shanjian

Comment 27

[Shanjian Li]

kin, could you sr this bug?

Comment 28

[kinmoz]

Comment on attachment 93324 [details] [diff] [review]
Proposed patch

Sorry for the delay, I was on vacation all last week ...

-- Can we remove the "My"?


+BOOL MyPeekKeyAndIMEMessage(LPMSG msg, HWND hwnd)


-- You can leave off the |else| since there is no other code path to follow:


+    return ::PeekMessage(msg, hwnd, lpMsg->message, lpMsg->message,
PM_REMOVE);
+  }
+  else
+    return false;
+}


-- Are you sure we don't need to factor in the
   WM_IME_STARTCOMPOSITION(0x010D) - WM_IME_KEYLAST(0x010F) messages?

Comment 29

[kinmoz]

Comment on attachment 93324 [details] [diff] [review]
Proposed patch

Bah I just noticed you are using WM_IME_KEYLAST.

Just address my first 2 comments and you can have sr=[e-mail].

Comment 30

[Shanjian Li]

Attachment: update with kin's comment

Comment 31

[Shanjian Li]

Comment on attachment 95181 [details] [diff] [review]
update with kin's comment

carry over r/sr

Comment 32

[Shanjian Li]

patch checked in to trunk.

Comment 33

[Shotaro Kamio]

Oh, shanjian, we must do same thing at line #179 also.

177     // Give priority to system messages (in particular keyboard, mouse,
178     // timer, and paint messages).
179     if (::PeekMessage(&msg, NULL, WM_KEYFIRST, WM_KEYLAST, PM_REMOVE) ||
180         ::PeekMessage(&msg, NULL, WM_MOUSEFIRST, WM_MOUSELAST, PM_REMOVE) || 
181         ::PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {

Comment 34

[Shanjian Li]

shotaro, you are right. I forgot to update my patch before I made modification
as suggested by kin. I added that part.

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 95181 [details] [diff] [review]
update with kin's comment

a=asa (on behalf of drivers) for checkin to the 1.1 branch. This needs to be
checked in quickly because 1.1 is nearly complete.

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

Approved for 1.0 branch as well; please check in ASAP.  When it's fixed in the
1.0 branch, please change keyword mozilla1.0.1+ to fixed1.0.1

Comment 37

[kill this account]

+BOOL PeekKeyAndIMEMessage(LPMSG msg, HWND hwnd)
...
+  b1 = ::PeekMessage(&msg1, NULL, WM_KEYFIRST, WM_IME_KEYLAST, PM_NOREMOVE);
+  b2 = ::PeekMessage(&msg2, NULL, WM_IME_SETCONTEXT, WM_IME_KEYUP, PM_NOREMOVE);
...
+    return ::PeekMessage(msg, hwnd, lpMsg->message, lpMsg->message, PM_REMOVE);

Does it matter that the first 2 peek calls use NULL for window and the 3rd call
uses hwnd?

Comment 38

[Shanjian Li]

That's a catch. It shouldn't matter in the context, NULL is used any way. But we
should make it consistent. I will do that. 

Comment 39

[Shanjian Li]

patch checked into 1.1 branch. 

Comment 40

[Shanjian Li]

patch checked into 1.0 branch. 

Comment 41

[Jaime Rodriguez, Jr.]

posthumus adt1.0.1+. pls remember to get get both Drivers' and ADT approval
before checking into the 1.0 branch.

Comment 42

[kill this account]

just an FYI: anyone interested in the event might want to cc: themself to
bug 157144. It is possible we may radically change the way we handle the event
queues.

Comment 43

[Yuying Long]

KOIKE Kazuhiko or Shotaro Kamio:
Could any of you please kindly verify this fix? I never able to reproduce it
maybe my machine is not slow enough or type not fast enough.  Seems the fix
works base on comment #23.

Comment 44

[bobj]

Does this affect all Windows IME for CJK?

Comment 45

[Shanjian Li]

theoritically, it should affect all IME. 

Comment 46

[Koike Kazuhiko]

Verified with 2002081508-trunk/Win2k. The most irritating bug for Japanese
people seems to be fixed!

Comment 47

[Yuying Long]

Thanks a lot KOIKE Kazuhiko! It would be really great that you can verify it on
branch build as well.

Comment 48

[kill this account]

FYI: it is possible event loop work being done in bug 157144 may make using
PeekMessage unnecessary.

Comment 49

[Koike Kazuhiko]

Verified with 1.0branch and 1.1branch.
1.0branch: 2002081706/Win98
1.1branch: 2002081419/Win98

Comment 50

[kill this account]

This would be more efficient if it first tested if the queue had -any- input; eg:

 BOOL PeekKeyAndIMEMessage(LPMSG msg, HWND hwnd)
 {
+   if (!HIWORD(GetQueueStatus(QS_INPUT)))
+      return false;
   MSG msg1, msg2, *lpMsg;
   BOOL b1, b2;
   b1 = ::PeekMessage(&msg1, NULL, WM_KEYFIRST, WM_IME_KEYLAST, PM_NOREMOVE);
   b2 = ::PeekMessage(&msg2, NULL, WM_IME_SETCONTEXT, WM_IME_KEYUP,  ...

(this is assuming that we keep this code; ie: bug 132759 and/or bug 157144 do not
remove the need for this)

Comment 51

[kill this account]

I've opened bug 164084 to discuss whether we should remove the PeekMessage code.
Please cc: yourself if interested.

Comment 52

[Teruko Kobayashi]

From comment #49, this was verified in 1.0.1.