558976 - IME related methods of nsIWidget should be merged

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

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

I think that nsIWidget::ResetInputState() was named for calling gtk_im_context_reset(). However, currently, we are using this method as "commit composition" method.

gtk_im_context_reset() is a notification for IMEs when cursor position is changed. The behavior which was committed is just a specific behavior of old IMEs. E.g., uim and iBus don't commit by gtk_im_context_reset() with some conversion engines. By this difference, looks like nsEditor::ForceCompositionEnd() has strange optimization. The editor uses nsEditor::ForceCompositionEnd() for both "committing" and "resetting".

Currently, each widget code can listen the cursor position change event itself, and it's only needed on gtk2, so, editor shouldn't call nsEditor::ForceCompositionEnd() for the "resetting". I.e., we don't need a method for "resetting" now.

Therefore,  we should rename it to nsIWidget::FinishComposition() and it should take an argument "aCompositionFinalizedAs" which is an int value.

enum {
  SYSTEM_DEFAULT,
  COMMIT_COMPOSITION,
  CANCEL_COMPOSITION
};

If that is CANCEL_COMPOSITION, widget should discard the composition, i.e., we can remove nsIWidget::CancelComposition() which is used in only a few places. When that is SYSTEM_DEFAULT, we shouldn't do any hacky code, this should be default behavior of the new method. COMMIT_COMPOSITION is for current behavior but I'm not sure whether we need this behavior or not. (If HTML5's editable element has this method, some web application developers might be happy.)

And also, for iBus and uim, editor should be able to know whether the "committing" is succeeded state by PRBool result. So, the new API should be:

class nsIWidget
{
enum {
  SYSTEM_DEFAULT,
  COMMIT_COMPOSITION,
  CANCEL_COMPOSITION
};

PRBool FinishComposition(aCompositionFinalizedAs = SYSTEM_DEFAULT);
}

Does somebody have some ideas for this?

Comment 1

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

I think that nsIWidget::CancelIMEComposition() can be removed on current code because even if we are canceling the composition, widget will send text event with empty string. So, the deference between ResetInputState() and CancelIMECompsotion() is only the content of the text event. Even if nsEditor will need the cancel composition, it can do it by ignoring the text event content.

So, the param isn't needed I think.

Comment 2

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

On GTK, we cannot ensure that the composition is always committed or canceled via API call. My current plan is, Add NotifyIME(Notification, void*) to nsIWidget and remove ResetInputState(), CancelIMEComposition(), OnIMEFocusChange(), OnIMETextChange() and OnIMESelectionChange(). The Notification is enum:

enum Notification {
  REQUEST_COMMIT_COMPOSITION,
  REQUEST_CANCEL_COMPOSITION,
  NOTIFY_FOCUS_IN,
  NOTIFY_FOCUS_OUT,
  NOTIFY_TEXT_CHANGE,
  NOTIFY_SELECTION_CHANGE
};

And the detail of text change is passed by the second argument.

Comment 3

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

Attachment: part.1 Merge nsIWidget::ResetInputState(), nsIWidget::CancelComposition(), nsIWidget::OnIMEFocusChange() and nsIWidget::OnIMESelectionChange()

This patch merges a couple of API methods which don't need additional arguments. By this change, we will never need to add new API which doesn't need additional argument.

It might be possible to make cross process APIs simple too. However, even if so, it should be another bug.

Comment 4

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

Attachment: part.6 Rename nsIWidget::OnIMETextChange() to nsIWidget::NotifyIMEOfTextChange()

This is an optional change for part.1. This renames OnIMETextChange() to similar name of NotifyIME(). I think that the new name is better for other developers to understand its job.

Comment 5

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

Attachment: part.2 Implement nsIWidget::NotifyIME() on Windows

Comment 6

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

Attachment: part.3 Implement nsIWidget::NotifyIME() on Cocoa

Comment 7

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

Attachment: part.4 Implement nsIWidget::NotifyIME() on GTK

Comment 8

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

Attachment: part.5 Implement nsIWidget::NotifyIME() on Android

Comment 9

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

Comment on attachment 720994 [details] [diff] [review]
part.2 Implement nsIWidget::NotifyIME() on Windows

The change for Windows is very simple :-)

Comment 10

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

Comment on attachment 720995 [details] [diff] [review]
part.3 Implement nsIWidget::NotifyIME() on Cocoa

nsCocoaWindow has never implemented IME related methods actually. So, we can just remove ResetInput() from it.

On nsChildView, this patch just merges the methods to new NotifyIME().

Comment 11

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

Comment on attachment 720998 [details] [diff] [review]
part.5 Implement nsIWidget::NotifyIME() on Android

Just merges the methods to the new NotifyIME().

Comment 12

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

Comment on attachment 720997 [details] [diff] [review]
part.4 Implement nsIWidget::NotifyIME() on GTK

* Renames nsGtkIMModule::ResetInputState() to CommitIMEComposition() since it matches the actual behavior.

* Merges the methods to the new NotifyIME(). As I mentioned in the TODO comment, we should replace NOTIFY_IME_OF_CURSOR_POS_CHANGED with NOTIFY_IME_OF_SELECTION_CHANGE. And then, we should just call gtk_im_context_reset() at that time rather than committing the composition. However, it needs more work, so, let's put off it to another bug.

Comment 13

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

FYI: The other platforms (qt, gonk and os2) don't implement any methods. So, we don't need to change on them.

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 720997 [details] [diff] [review]
part.4 Implement nsIWidget::NotifyIME() on GTK

>+        // TODO: We should replae NOTIFY_IME_OF_CURSOR_POS_CHANGED with
>+        //       NOTIFY_IME_OF_SELECTION_CHANGE.  The required behavior is
>+        //       really different from committing composition.
>+        case NOTIFY_IME_OF_CURSOR_POS_CHANGED:

replae -> replace

>+        case REQUEST_TO_COMMIT_COMPOSITION:
>+            return mIMModule ? mIMModule->CommitIMEComposition(this) : NS_OK;
>+        case REQUEST_TO_CANCEL_COMPOSITION:
>+            return mIMModule ? mIMModule->CancelIMEComposition(this) : NS_OK;
>+        case NOTIFY_IME_OF_FOCUS:
>+            if (mIMModule) {
>+                mIMModule->OnFocusChangeInGecko(true);
>+            }
>+            return NS_OK;
>+        case NOTIFY_IME_OF_BLUR:
>+            if (mIMModule) {
>+                mIMModule->OnFocusChangeInGecko(false);
>+            }
>+            return NS_OK;
>+        default:
>+            return NS_ERROR_NOT_IMPLEMENTED;

This would be a little simpler with a single "if (!mIMModule) return NS_OK"
before the switch.

>+    NS_IMETHOD NotifyIME(NotificationToIME aNotification);

Can you add MOZ_OVERRIDE to this declaration, please?

Please fold this patch into part 1 (as I assume IME notifications will not be
effective with only part 1).

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 720998 [details] [diff] [review]
part.5 Implement nsIWidget::NotifyIME() on Android

Review of attachment 720998 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! I will file a bug to update the names on the Java side.

::: widget/android/nsWindow.h
@@ -134,1 @@
>      NS_IMETHOD OnIMETextChange(uint32_t aStart, uint32_t aOldEnd, uint32_t aNewEnd);

Should you change OnIMETextChange to NotifyIMEOfTextChange according to part 6?

Comment 16

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

(In reply to Karl Tomlinson (:karlt) from comment #14)
> >+        case REQUEST_TO_COMMIT_COMPOSITION:
> >+            return mIMModule ? mIMModule->CommitIMEComposition(this) : NS_OK;
> >+        case REQUEST_TO_CANCEL_COMPOSITION:
> >+            return mIMModule ? mIMModule->CancelIMEComposition(this) : NS_OK;
> >+        case NOTIFY_IME_OF_FOCUS:
> >+            if (mIMModule) {
> >+                mIMModule->OnFocusChangeInGecko(true);
> >+            }
> >+            return NS_OK;
> >+        case NOTIFY_IME_OF_BLUR:
> >+            if (mIMModule) {
> >+                mIMModule->OnFocusChangeInGecko(false);
> >+            }
> >+            return NS_OK;
> >+        default:
> >+            return NS_ERROR_NOT_IMPLEMENTED;
> 
> This would be a little simpler with a single "if (!mIMModule) return NS_OK"
> before the switch.

Hmm, but if the notification isn't supported on GTK widget, it should return NS_ERROR_NOT_IMPLEMENTED. Do you like to make another switch for !mIMModule case?

(In reply to Jim Chen [:jchen :nchen] from comment #15)
> Comment on attachment 720998 [details] [diff] [review]
> part.5 Implement nsIWidget::NotifyIME() on Android
> 
> Review of attachment 720998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! I will file a bug to update the names on the Java side.
> 
> ::: widget/android/nsWindow.h
> @@ -134,1 @@
> >      NS_IMETHOD OnIMETextChange(uint32_t aStart, uint32_t aOldEnd, uint32_t aNewEnd);
> 
> Should you change OnIMETextChange to NotifyIMEOfTextChange according to part
> 6?

I did it at part.6. The change is just renaming. Therefore, I didn't request review to each widget reviewer.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 720995 [details] [diff] [review]
part.3 Implement nsIWidget::NotifyIME() on Cocoa

Looks fine to me.

Comment 18

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

Attachment: part.4 Implement nsIWidget::NotifyIME() on GTK

How is this? !mIMModule case should not been never run. So, returning NS_ERROR_NOT_AVAILABLE is better.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 721499 [details] [diff] [review]
part.4 Implement nsIWidget::NotifyIME() on GTK

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> Hmm, but if the notification isn't supported on GTK widget, it should return
> NS_ERROR_NOT_IMPLEMENTED. Do you like to make another switch for !mIMModule
> case?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> How is this? !mIMModule case should not been never run. So, returning
> NS_ERROR_NOT_AVAILABLE is better.

>+    if (MOZ_UNLIKELY(!mIMModule)) {
>+        switch (aNotification) {
>+            case NOTIFY_IME_OF_CURSOR_POS_CHANGED:
>+            case REQUEST_TO_COMMIT_COMPOSITION:
>+            case REQUEST_TO_CANCEL_COMPOSITION:
>+            case NOTIFY_IME_OF_FOCUS:
>+            case NOTIFY_IME_OF_BLUR:
>+              return NS_ERROR_NOT_AVAILABLE;
>+            default:
>+              break;
>+        }
>+    }

I would have just returned NS_ERROR_NOT_AVAILABLE or even NS_OK regardless of aNotification here.

If there is no mIMModule, then there is nothing to notify, so any notification can be successful.

If it is important to get an error code in this situation, then is it worth the extra switch statement to distinguish between NS_ERROR_NOT_AVAILABLE and NS_ERROR_NOT_IMPLEMENTED?

Comment 20

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

According to my experience, NS_ERROR_NOT_IMPLEMENTED is sometimes useful. For example, if a platform returns NS_ERROR_NOT_IMPLEMENTED for an API call, then, the caller can do a fallback handling or just skip something processed later. Therefore, always returning NS_ERROR_NOT_IMPLEMENTED is better for callers.

Anyway, normally, mIMModule is not null except the APIs called with wrong widget. So, I think that the switch statement's running cost can be ignored. I mean that this is the best.

Thank you for the review.

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/240c40db506c
https://hg.mozilla.org/integration/mozilla-inbound/rev/be97307e760c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6534852134f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/37a32c17c862
https://hg.mozilla.org/integration/mozilla-inbound/rev/70e09aed19c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb81225f61bf

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/240c40db506c
https://hg.mozilla.org/mozilla-central/rev/be97307e760c
https://hg.mozilla.org/mozilla-central/rev/6534852134f7
https://hg.mozilla.org/mozilla-central/rev/37a32c17c862
https://hg.mozilla.org/mozilla-central/rev/70e09aed19c1
https://hg.mozilla.org/mozilla-central/rev/cb81225f61bf