Closed Bug 478030 Opened 12 years ago Closed 12 years ago
[TSF] The composition string is not displayed on Win
XP + MS-IME
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.
I first reported this issue in the original TSF bug: https://bugzilla.mozilla.org/show_bug.cgi?id=88831#c107
Severity: normal → major
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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.
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.
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.
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.
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)
I expanded the test. Now, it's implemented the attribute property related interfaces.
removed an unused variable. sorry for the spam.
Comment on attachment 363309 [details] [diff] [review] Patch v3.1.1 Thank you!
Attachment #363309 - Flags: review?(chenn) → review+
Attachment #363309 - Flags: superreview?(roc)
+ 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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
10 years ago
You need to log in before you can comment on or make changes to this bug.