Closed Bug 478030 Opened 12 years ago Closed 12 years ago

[TSF] The composition string is not displayed on WinXP + MS-IME

Categories

(Core :: Widget: Win32, defect)

x86
Windows XP
defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, intl)

Attachments

(1 file, 5 obsolete files)

On WinXP + MS-IME, the composition string is not displayed. But on WinVista, I cannot reproduce this bug.

The cause may be:
> |OnUpdateComposition| is called with pRangeNew == null.
> Therefore, we don't update IME selection. Looks like we need to create the log
> of |InsertTextAtSelection|. And we should use it if pRangeNew is null.
taking.
Severity: normal → major
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attached patch Patch v1.0 (obsolete) — Splinter Review
If pRangeNew is null, we can get the current range from pComposition.

This patch fixes the composition string invisible problem. But the composition string doesn't looks good for me. Because we fail to get the attrs. But it should be another bug.
Attachment #362497 - Flags: review?(chenn)
Jim, if you don't/cannot want to review TSF support code, please tell me it. I'll find another reviewer for it (Maybe, Kimura-san). Otherwise please review the patch ASAP.
This bug should be a blocker. Because the composing string is most important information for Japanese IME. So, without this fix, Japanese testers cannot test enough.
Severity: major → blocker
(In reply to comment #5)
> This bug should be a blocker. Because the composing string is most important
> information for Japanese IME. So, without this fix, Japanese testers cannot
> test enough.

Chinese users too.
Attached patch Patch v2.0 (obsolete) — Splinter Review
ok, the previous patch may be wrong approach.

OnUpdateComposition, the composing range should not be updated yet if TIP implements correctly.

SetSelection will be called after OnUpdateComposition for setting the IME selection to composition string. This is the cause of the XP selection broken bug of my previous comment.

Therefore, we should not dispatch text event in OnUpdateCompsition, instead of that, we should dispatch them in SetSelection. This patch works perfectly on Vista+MS-IME and XP+MS-IME.
Attachment #362497 - Attachment is obsolete: true
Attachment #362752 - Flags: review?(chenn)
Attachment #362497 - Flags: review?(chenn)
Thank you for your work, Masayuki-san. It looks good.

1) On Vista I can see flickering with the patch using Microsoft Pinyin.
 Steps: Use Microsoft Pinyin on Vista, type in "nihaoma", press space, and then use left and right keys to move the cursor. There is flickering when the cursor moves. Can you reproduce this? It might be unimportant.

2) TestWinTSF is broken on a debug build because of NS_NOTREACHED in GetProperty. Can you include the fix in your patch?

Thanks.
(In reply to comment #8)
> 1) On Vista I can see flickering with the patch using Microsoft Pinyin.
>  Steps: Use Microsoft Pinyin on Vista, type in "nihaoma", press space, and then
> use left and right keys to move the cursor. There is flickering when the cursor
> moves. Can you reproduce this? It might be unimportant.

mmmm, I cannot reproduce it, but maybe it can be happen by my patch. Because SetSelection is called many times. I think I should create a new flag which is set to true in OnUpdateComposition, and SendTextEventForCompositionString should be called only when the flag is true. Then, the flag should be cleared.

> 2) TestWinTSF is broken on a debug build because of NS_NOTREACHED in
> GetProperty. Can you include the fix in your patch?

OK, I'll check it.
Attached patch Patch v3.0 (work in progress) (obsolete) — Splinter Review
This is better. Text event is dispatched by both OnQueryComposition and SetSelection. nsTextStore cache the latest text event, and if current event is same as previous one, it doesn't dispatch the event again. This can suppress the flickering in some cases and stronger implementation for TIP implementation differences. However, this is not perfect solution for flickering. Because even when only caret position is changed, we need to dispatch new text event. I think that we should implement similar process to nsEditor. However, it should not be the scope of this bug. (and it's not important now)
Attachment #362752 - Attachment is obsolete: true
Attachment #362752 - Flags: review?(chenn)
Attachment #363078 - Attachment is patch: true
Attachment #363078 - Attachment mime type: application/octet-stream → text/plain
Attached patch Patch v3.1 (obsolete) — Splinter Review
I expanded the test. Now, it's implemented the attribute property related interfaces.
Attachment #363078 - Attachment is obsolete: true
Attachment #363304 - Flags: review?(chenn)
Attached patch Patch v3.1.1 (obsolete) — Splinter Review
removed an unused variable. sorry for the spam.
Attachment #363304 - Attachment is obsolete: true
Attachment #363309 - Flags: review?(chenn)
Attachment #363304 - Flags: review?(chenn)
Comment on attachment 363309 [details] [diff] [review]
Patch v3.1.1

Thank you!
Attachment #363309 - Flags: review?(chenn) → review+
+    if (mLastDispatchedTextEvent->rangeArray)
+      delete [] mLastDispatchedTextEvent->rangeArray;
+    delete mLastDispatchedTextEvent;

Is there a reason we can't make the nsEvent destructor virtual, give the subclasses proper destructors and avoid having to repeat code like this (and similar code elsewhere)?
Comment on attachment 363309 [details] [diff] [review]
Patch v3.1.1

+  mLastDispatchedTextEvent-> rangeArray = nsnull;

Unnecessary space.

+  // Clear the text event log

Not sure what you mean by "log" here. Just say "clear the saved text event" I think.
Attachment #363309 - Flags: superreview?(roc) → superreview+
(In reply to comment #14)
> +    if (mLastDispatchedTextEvent->rangeArray)
> +      delete [] mLastDispatchedTextEvent->rangeArray;
> +    delete mLastDispatchedTextEvent;
> 
> Is there a reason we can't make the nsEvent destructor virtual, give the
> subclasses proper destructors and avoid having to repeat code like this (and
> similar code elsewhere)?

here:
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#6737

but maybe, the nsTextEvent can not be an owner of the buffer.
checked-in.
http://hg.mozilla.org/mozilla-central/rev/9174e1bbfa9f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.