Quick typing erases a converting text

VERIFIED FIXED in mozilla1.1final

Status

()

P3
normal
VERIFIED FIXED
18 years ago
10 years ago

People

(Reporter: kazhik, Assigned: shanjian)

Tracking

({intl})

Trunk
mozilla1.1final
x86
Windows 98
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

18 years ago
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

18 years ago
Roy, please take a look at this.

Koike san, is this a window specific problem?
Assignee: nhotta → yokoyama
(Reporter)

Comment 2

18 years ago
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

18 years ago
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
(Reporter)

Comment 4

18 years ago
Slow typing sometimes reproduces this problem, but I can't find steps to
reproduce.
(Reporter)

Comment 5

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

Comment 6

18 years ago
setting bug status to New.  kazhik, I've updated your bugzilla permissions.  
Please email me if you have any questions.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 7

18 years ago
Changed QA contact to ylong@netscape.com.
Keywords: intl
QA Contact: teruko → ylong

Comment 8

18 years ago
Updating the target milestone.
Target Milestone: --- → Future
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

16 years ago
setting a new milestone.
Target Milestone: Future → mozilla1.2beta

Comment 11

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

Comment 12

16 years ago
My keyboard is 106.

Comment 13

16 years ago
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.
(Reporter)

Updated

16 years ago
Blocks: 157673

Comment 14

16 years ago
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?
Assignee: yokoyama → shanjian
Status: ASSIGNED → NEW
Keywords: nsbeta1+
Whiteboard: [adt2]
(Reporter)

Comment 15

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

Kamio-san, attach your patch here.
(Assignee)

Comment 16

16 years ago
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. 
Status: NEW → ASSIGNED

Comment 17

16 years ago
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

16 years ago
Created attachment 92676 [details] [diff] [review]
a patch to erase prioritizing
(Assignee)

Comment 19

16 years ago
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

16 years ago
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?
(Assignee)

Comment 21

16 years ago
Created attachment 93173 [details] [diff] [review]
patch
(Assignee)

Comment 22

16 years ago
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

16 years ago
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 :-)
(Assignee)

Comment 24

16 years ago
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

16 years ago
Created attachment 93324 [details] [diff] [review]
Proposed patch

Here is the complete patch.
Attachment #92676 - Attachment is obsolete: true
Attachment #93173 - Attachment is obsolete: true
(Assignee)

Comment 26

16 years ago
Comment on attachment 93324 [details] [diff] [review]
Proposed patch

r=shanjian
Attachment #93324 - Flags: review+
(Assignee)

Comment 27

16 years ago
kin, could you sr this bug?

Comment 28

16 years ago
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

16 years ago
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=kin@netscape.com.
Attachment #93324 - Flags: superreview+
(Assignee)

Comment 30

16 years ago
Created attachment 95181 [details] [diff] [review]
update with kin's comment
Attachment #93324 - Attachment is obsolete: true
(Assignee)

Comment 31

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

carry over r/sr
Attachment #95181 - Flags: superreview+
Attachment #95181 - Flags: review+
(Assignee)

Comment 32

16 years ago
patch checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 33

16 years ago
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)) {
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 34

16 years ago
shotaro, you are right. I forgot to update my patch before I made modification
as suggested by kin. I added that part.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED

Comment 35

16 years ago
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.
Attachment #95181 - Flags: approval+
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
Keywords: mozilla1.0.1+, mozilla1.1
Target Milestone: mozilla1.2beta → mozilla1.1final

Comment 37

16 years ago
+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?
(Assignee)

Comment 38

16 years ago
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. 

(Assignee)

Comment 39

16 years ago
patch checked into 1.1 branch. 
(Assignee)

Comment 40

16 years ago
patch checked into 1.0 branch. 
Keywords: mozilla1.0.1+ → fixed1.0.1

Comment 41

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

Comment 42

16 years ago
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

16 years ago
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

16 years ago
Does this affect all Windows IME for CJK?
(Assignee)

Comment 45

16 years ago
theoritically, it should affect all IME. 
(Reporter)

Comment 46

16 years ago
Verified with 2002081508-trunk/Win2k. The most irritating bug for Japanese
people seems to be fixed!
Status: RESOLVED → VERIFIED

Comment 47

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

Comment 48

16 years ago
FYI: it is possible event loop work being done in bug 157144 may make using
PeekMessage unnecessary.
(Reporter)

Comment 49

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

Comment 50

16 years ago
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

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

Comment 52

16 years ago
From comment #49, this was verified in 1.0.1.
Keywords: verified1.0.1

Updated

16 years ago
Depends on: 180372

Updated

16 years ago
No longer blocks: 157673
Blocks: 145313
You need to log in before you can comment on or make changes to this bug.