532130 - [IMM32] Support IMR_DOCUMENTFEED of WM_IME_REQUEST

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement)

Description

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

Attachment: Patch v1.0

We should handle IMR_DOCUMENTFEED of WM_IME_REQUEST.

http://msdn.microsoft.com/en-us/library/aa915528.aspx
http://d.hatena.ne.jp/topiyama/20070703/p1
http://jp.emeditor.com/modules/newbb/viewtopic.php?topic_id=519&forum=4

This improves the quality of the conversion.

Comment 1

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

pushed to tryserver:
http://hg.mozilla.org/try/rev/c8329307431e

Comment 2

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

https://build.mozilla.org/tryserver-builds/[e-mail]-try-f1bc0ae98456/

test builds here.
# c8329307431e failed to build the win32 build by tryserver problem.

Comment 3

[Masatoshi Kimura [:emk]]

Comment on attachment 415415 [details] [diff] [review]
Patch v1.0

+        selectionLength -=
+          mCompositionString.Length() - (selectionOffset - mCompositionStart);
Will selectionLength never become negative?

+    } else if (selectionLength > 0) {
+      // If selection starts before composition string but the end is in the
+      // composition string, shrink the selection.
+      selectionLength -= mCompositionStart - selectionOffset;
+    }
Is selectionOffset + selectionLength always larger than mCompositionStart?

+  if (!pReconv) {
+    PRUint32 len = paragraph.Length();
+    *oResult = sizeof(RECONVERTSTRING) + len * sizeof(WCHAR);
:
+  PRUint32 len = paragraph.Length();
+  PRUint32 needSize = sizeof(RECONVERTSTRING) + len * sizeof(WCHAR);
Place this two lines above the if statement to avoid unnecessary duplication.

> +  DWORD tmpSize = pReconv->dwSize;
> +  ::ZeroMemory(pReconv, tmpSize);
> +  pReconv->dwSize            = tmpSize;
* Are you sure pReconv->dwSize is set by caller? Both of two sample codes from comment #0 don't preserve dwSize.
* Is it required to clear the buffer in the first place? All structure members are explicitely set below.
* ::ZeroMemory(reinterpret_cast<LPVOID>(lParam + sizeof(pReconv->dwSize)),
               pReconv->dwSize - sizeof(pReconv->dwSize));
  even if it's required.

> +  pReconv->dwTargetStrLen    = selectionLength * sizeof(WCHAR);
Per MSDN, length should be character counts.
http://msdn.microsoft.com/en-us/library/dd319107%28VS.85%29.aspx

Comment 4

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

(In reply to comment #3)
> (From update of attachment 415415 [details] [diff] [review])
> +        selectionLength -=
> +          mCompositionString.Length() - (selectionOffset - mCompositionStart);
> Will selectionLength never become negative?

Yes. The selectionOffset is:

selectionOffset > mCompositionStart &&
selectionOffset <= mCompositionStart + mCompositionString.Length()

> +    } else if (selectionLength > 0) {
> +      // If selection starts before composition string but the end is in the
> +      // composition string, shrink the selection.
> +      selectionLength -= mCompositionStart - selectionOffset;
> +    }
> Is selectionOffset + selectionLength always larger than mCompositionStart?

Ah, maybe the condition of the block is wrong.

> > +  DWORD tmpSize = pReconv->dwSize;
> > +  ::ZeroMemory(pReconv, tmpSize);
> > +  pReconv->dwSize            = tmpSize;
> * Are you sure pReconv->dwSize is set by caller? Both of two sample codes from
> comment #0 don't preserve dwSize.

I think it should be so because the caller can limit the buffer size. We should ignore such cases.

> > +  pReconv->dwTargetStrLen    = selectionLength * sizeof(WCHAR);
> Per MSDN, length should be character counts.
> http://msdn.microsoft.com/en-us/library/dd319107%28VS.85%29.aspx

Thanks!

Comment 5

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

> * Is it required to clear the buffer in the first place? All structure members
> are explicitely set below.

Yes, the caller can send a fixed length buffer always. So, dwSize can be larger than the needed size. We need to clear the all buffer in such case.

Comment 6

[Masatoshi Kimura [:emk]]

(In reply to comment #4)
> > +        selectionLength -=
> > +          mCompositionString.Length() - (selectionOffset - mCompositionStart);
> > Will selectionLength never become negative?
> 
> Yes. The selectionOffset is:
> 
> selectionOffset > mCompositionStart &&
> selectionOffset <= mCompositionStart + mCompositionString.Length()

Is the following condition also guaranteed?
selectionOffset + selectionLength >= mCompositionStart + mCompositionString.Length()

(In reply to comment #5)
> > * Is it required to clear the buffer in the first place? All structure members
> > are explicitely set below.
> 
> Yes, the caller can send a fixed length buffer always. So, dwSize can be larger
> than the needed size. We need to clear the all buffer in such case.

Why the unused buffer needs to be cleared? I think it's harmless to leave the unused garbage if it is not referred by offset/len pairs.

Comment 7

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

(In reply to comment #6)
> (In reply to comment #4)
> > > +        selectionLength -=
> > > +          mCompositionString.Length() - (selectionOffset - mCompositionStart);
> > > Will selectionLength never become negative?
> > 
> > Yes. The selectionOffset is:
> > 
> > selectionOffset > mCompositionStart &&
> > selectionOffset <= mCompositionStart + mCompositionString.Length()
> 
> Is the following condition also guaranteed?
> selectionOffset + selectionLength >= mCompositionStart +
> mCompositionString.Length()

Yes, because there is previous if statement.

Comment 8

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

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > > +        selectionLength -=
> > > > +          mCompositionString.Length() - (selectionOffset - mCompositionStart);
> > > > Will selectionLength never become negative?
> > > 
> > > Yes. The selectionOffset is:
> > > 
> > > selectionOffset > mCompositionStart &&
> > > selectionOffset <= mCompositionStart + mCompositionString.Length()
> > 
> > Is the following condition also guaranteed?
> > selectionOffset + selectionLength >= mCompositionStart +
> > mCompositionString.Length()
> 
> Yes, because there is previous if statement.

Er, I misread your comment, please wait a moment.

Comment 9

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

Attachment: Patch v1.1

Comment 10

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

Comment on attachment 416222 [details] [diff] [review]
Patch v1.1

er, there is mistake.

Comment 11

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

Attachment: Patch v1.2

I hope this is correct...

Comment 12

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

Comment on attachment 416226 [details] [diff] [review]
Patch v1.2

Um... Something was broken.

Comment 13

[Masatoshi Kimura [:emk]]

You will need to consider the following five cases.
1. mCompositionStart + mCompositionString.Length() < selectionOffset
2. mCompositionStart <= selectionOffset && selectionOffset < mCompositionStart + mCompositionString.Length()
2.1. mCompositionStart + mCompositionString.Length() < selectionOffset + selectionLength
2.2. selectionOffset + selectionLength <= mCompositionStart + mCompositionString.Length()
3. selectionOffset < mCompositionStart
3.1. mCompositionStart < selectionOffset + selectionLength
3.2. selectionOffset + selectionLength <= mCompositionStart

> +  PRUint32 needSize = sizeof(RECONVERTSTRING) + len * sizeof(WCHAR);
> +  *oResult = needSize;
> +
> +  if (!pReconv) {
Don't set the return value here because the method can fail after that.

Comment 14

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

Attachment: Patch v1.3

ok, this is simple.

We don't need to cut the composition string if we set the dwCompStrLen and dwCompStrOffset correctly.

And I changed another point. If the selection covers two or more paragraphs, this patch returns all paragraphs which include the selection range.

Comment 15

[Masatoshi Kimura [:emk]]

Comment on attachment 416386 [details] [diff] [review]
Patch v1.3

> +  if (mIsComposing && ShouldDrawCompositionStringOurselves()) {
> +    pReconv->dwCompStrLen      = mCompositionString.Length();
> +    pReconv->dwCompStrOffset   =
> +      (mCompositionStart - paragraphStart) * sizeof(WCHAR);
Is (mCompositionStart - paragraphStart) always positive? I heard that WXG could have line separators in the composition string. Could you pass all contents of the editor instead of only a paragraph?

> +    // Set composition string information to the target string.
> +    pReconv->dwTargetStrLen    = pReconv->dwCompStrLen;
> +    pReconv->dwTargetStrOffset = pReconv->dwCompStrOffset;
Is this always equal to the entire composition string? I thought it means "対象文節".

Comment 16

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

Attachment: Patch v1.4

How about this?

This patch has two modes:
1. When it's not in composition, or it's in composition but we don't manage composition string, we use the selection range.
2. Otherwise, ignores the selection. Instead, uses the composition string range.

This patch defines that the current paragraph starts from the next character of the previous LF of the target offset and ends at the previous character of the next CR of the target end offset.

Comment 17

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

Attachment: Patch v1.4.1

Uses nsDependentSubstring for the paragraph. That doesn't allocate/copy the memory.

Comment 18

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

http://hg.mozilla.org/mozilla-central/rev/d029f744960d

Thank you, Kimura-san.

Comment 19

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

oops, backed-out the patch because I found a bug in the patch.

Comment 20

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

Attachment: Patch v1.5

Sorry for the re-review.

In the previous patch, nsIMM32Handler::GetTargetClauseRange doesn't return correct length. See the difference between this patch and the previous one.

Comment 21

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

Comment on attachment 417645 [details] [diff] [review]
Patch v1.5

No, this is still buggy.

Comment 22

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

Attachment: Patch v1.6

> +  if (hasCompositionString) {
> +    pReconv->dwCompStrLen      = targetLength;
> +    pReconv->dwCompStrOffset   =
> +      (targetOffset - paragraphStart) * sizeof(WCHAR);
> +    // Set composition target clause information
> +    PRUint32 offset, length;
> +    if (!GetTargetClauseRange(&offset, &length)) {
> +      PR_LOG(gIMM32Log, PR_LOG_ALWAYS,
> +        ("IMM32: HandleDocumentFeed, FAILED, by GetTargetClauseRange\n"));
> +      return PR_FALSE;
> +    }
> +    pReconv->dwTargetStrLen    = offset - mCompositionStart;
> +    pReconv->dwTargetStrOffset = length;

Here are three bugs:
1. Sets to wrong variable (offset -> len and len -> offset).
2. The offset should be byte size (needs |* sizeof(WCHAR)|)
3. The offset should be subtracted with paragraphStart, not mCompositionStart.

In nsIMM32Handler::GetTargetClauseRange:

>   if (!found) {
>     // The all composition string is targetted when there is no ATTR_TARGET_*
>     // clause. E.g., there is only ATTR_INPUT
>     *aLength = mCompositionString.Length();
>     return PR_TRUE;
>   }

I added this. When there are no ATTR_TARGET_*, we should just return the length of composition string, not 0.

Comment 23

[Masatoshi Kimura [:emk]]

Comment on attachment 417649 [details] [diff] [review]
Patch v1.6

Sorry to overlook the bugs.
Off-topic: Google IME didn't use context even if native TSF support was enabled.

Comment 24

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

http://hg.mozilla.org/mozilla-central/rev/49e75469c968