278061 - Navi bar of ATOK does not appear

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P2)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

On other applications, when we use ATOK, the navi bar appear on cursor position.
But on Mozilla does not appear the navi bar.
We need to add two functions.

1. Set composition window position.
 We don't set composition window position because we don't use composition window.
 But if we don't set composition window position, the navi bar doesn't appear.
2. Implement IMR_QUERYCHARPOSITION event on WM_IMEREQUEST.
 The navi bar is positioned to the returned cursor position by
 IMR_QUERYCHARPOSITION.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.0

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.0(widget/src/windows)

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.0(editor)

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.0(dom/public, content/events)

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.0(widget/public)

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.0(extensions/typeaheadfind)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.0(view/src)

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Oops...
Wait to review.
I thought of more simple implementation.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1

I could not make more simple patch.
But I fixed some bugs of previous patch.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1(widget/src/windows)

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1(editor)

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1(dom/public, content/events)

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1(widget/public, view/src)

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1(extensions/typeaheadfind)

Comment 15

[timeless]

Comment on attachment 171136 [details] [diff] [review]
Patch rv1.1(editor)

> interface nsIEditorIMESupport : nsISupports
> {
...
>   void  EndComposition();
I know that this interface is using InitialCaps, but it really shouldn't.

>   [noscript] void  QueryComposition(in nsTextEventReplyPtr aReply);
If you could change the [noscript] functions to interCaps, that'd be nice :) -
note that it doesn't change anything because xpidl will make the functions
InitialCaps in the .h files.

>   void  ForceCompositionEnd();
The scriptable versions would need to be changed in concert w/ any js consumers
(hopeful there aren't many, i only see one consumer:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/ui/composer/content/
ComposerCommands.js&rev=1.198&mark=931-932#926
), and can be done in another bug.

would you please fix this line:
>    * GetReconvertionString()  Get the reconvertion string
to match the actual function name:
>   [noscript] void  GetReconversionString(in nsReconversionEventReplyPtr aReply);

>    * Notify for IME when the editor got focus.
it'd be nice if someone explained what this actually meant :(
I can't see why these methods are [noscript]
>   [noscript] void NotifyIMEOnFocus();
>    * Notify for IME when the editor lost focus.
>   [noscript] void NotifyIMEOnBlur();

please fix the spelling/function:
>+   * GetQueryCharPositioin()  Get the query char position
>+  [noscript] void  GetQueryCaretRect(in nsQueryCaretRectEventReplyPtr aReply);

i'm not at all fond of the this function name or the style you're using for it,
but oh well :(

>Index: editor/libeditor/base/nsEditor.cpp
>+nsEditor::GetQueryCaretRect(nsQueryCaretRectEventReply* aReply)
>+{
>+  nsCOMPtr<nsISelection> selection
please use |rv| instead of |result|:
>+  nsresult result = GetSelection(getter_AddRefs(selection));
>+  if (NS_FAILED(result))
>+    return result;

Comment 16

[timeless]

Comment on attachment 171135 [details] [diff] [review]
Patch rv1.1(widget/src/windows)

+  // We don't support other of left most postion or sIMECursorPosition.
Are you trying to say:
We only support insertion at the cursor position or at the leftmost position.
?

+  // Because array of sIMECompCharPos may be saving wrong position after
+  // compostion string is converted.

I can't understand this, i can understand that there are ways for stuff to be
corrupted.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to comment #16)
> (From update of attachment 171135 [details] [diff] [review] [edit])
> +  // We don't support other of left most postion or sIMECursorPosition.
> Are you trying to say:
> We only support insertion at the cursor position or at the leftmost position.
> ?
Yes, I will rewrite comment so.

> +  // Because array of sIMECompCharPos may be saving wrong position after
> +  // compostion string is converted.
> 
> I can't understand this, i can understand that there are ways for stuff to be
> corrupted.

You are right.
The composition string is converted by user.
But sIMECompCharPos is saved by inputted the new char.
When composition string is converted, we don't re-get the new positions.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1.1(editor)

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1.1(widget/src/windows)

I rewrote the comment.
review+ is carried over.

Comment 20

[timeless]

Comment on attachment 171989 [details] [diff] [review]
Patch rv1.1.1(editor)

+  void NotifyIMEOnBlur();

that should be interCaps. you can ask the original reviewer candidate for the
next version of this patch, i'm out of feedback.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1.2(editor)

Comment 22

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 172321 [details] [diff] [review]
Patch rv1.1.2(editor)

Deferring to smontagu who obviously knows/understands better this part of the
code than I do.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 171137 [details] [diff] [review]
Patch rv1.1(dom/public, content/events)

r=jst

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1.2

Boris Zbarsky:

if you grant this patch, please check-in.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 172941 [details] [diff] [review]
Patch rv1.1.2

>Index: editor/idl/nsIEditorIMESupport.idl

Please fix whatever consumers are broken by these changes... For example,
ComposerCommands.js calls ForceCompositionEnd().

The rest of the patch looks ok, but marking sr- pending consumers being fixed.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1.3(editor/ui)

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1.3

Comment 28

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 172976 [details] [diff] [review]
Patch rv1.1.3(editor/ui)

r/rs=[e-mail]
this patch should not be separated from 1.1.2 (editor), really...

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 172978 [details] [diff] [review]
Patch rv1.1.3

Thank you, Daniel.
Boris, please re-superreview it.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 172978 [details] [diff] [review]
Patch rv1.1.3

sr=bzbarsky

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Thank you, Boris.

Jungshik:

Could you check-in my patch?

Comment 32

[Boris Zbarsky [:bzbarsky]]

Last patch checked in.

Comment 33

[Jungshik Shin]

   aReply = nsnull;
   return NS_ERROR_FAILURE;
 }
 
+NS_METHOD
+nsDOMUIEvent::GetQueryCaretRectReply(nsQueryCaretRectEventReply** aReply)
+{
+  if (mEvent->eventStructType == NS_QUERYCARETRECT_EVENT)
+  {
+    *aReply = &(NS_STATIC_CAST(nsQueryCaretRectEvent*, mEvent)->theReply);
+    return NS_OK;
+  }
+  aReply = nsnull;

There are two instances of |aReply = nsnull|. You meant |*aReply = nsnull|,
didn't you?

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch

Jungshik:

Maybe. I copied from above function.
Can you review it?

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch

Sorry, here is.

Comment 36

[Jungshik Shin]

Comment on attachment 173175 [details] [diff] [review]
Patch

r=jshin

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173175 [details] [diff] [review]
Patch

sr=bzbarsky; I'm not going to be able to check this in till late tonight,
probably, so if you find someone else go for it.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Thank you Boris.
Jungshik, could you check-in the patch?

Comment 39

[Jungshik Shin]

just landed. 

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Thank you, everyone.