IME related methods of nsIWidget should be merged

RESOLVED FIXED in mozilla22

Status

()

RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla22
inputmethod
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

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?
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.
Summary: Rename nsIWidget::ResetInputState() to nsIWidget::FinishComposition() and replace nsIWidget::CancelComposition() by it → Rename nsIWidget::ResetInputState() to nsIWidget::FinishComposition() and remove nsIWidget::CancelComposition()
Summary: Rename nsIWidget::ResetInputState() to nsIWidget::FinishComposition() and remove nsIWidget::CancelComposition() → Rename nsIWidget::ResetInputState() to nsIWidget::FinishComposition() and remove nsIWidget::CancelIMEComposition()
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.
Summary: Rename nsIWidget::ResetInputState() to nsIWidget::FinishComposition() and remove nsIWidget::CancelIMEComposition() → IME related methods of nsIWidget should be merged
Depends on: 805767
Created attachment 720991 [details] [diff] [review]
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.
Attachment #720991 - Flags: superreview?(roc)
Attachment #720991 - Flags: review?(roc)
Created attachment 720993 [details] [diff] [review]
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.
Attachment #720993 - Flags: superreview?(roc)
Attachment #720993 - Flags: review?(roc)
Created attachment 720994 [details] [diff] [review]
part.2 Implement nsIWidget::NotifyIME() on Windows
Created attachment 720995 [details] [diff] [review]
part.3 Implement nsIWidget::NotifyIME() on Cocoa
Created attachment 720997 [details] [diff] [review]
part.4 Implement nsIWidget::NotifyIME() on GTK
Created attachment 720998 [details] [diff] [review]
part.5 Implement nsIWidget::NotifyIME() on Android
Attachment #720991 - Flags: superreview?(roc)
Attachment #720991 - Flags: superreview+
Attachment #720991 - Flags: review?(roc)
Attachment #720991 - Flags: review+
Attachment #720993 - Flags: superreview?(roc)
Attachment #720993 - Flags: superreview+
Attachment #720993 - Flags: review?(roc)
Attachment #720993 - Flags: review+
Comment on attachment 720994 [details] [diff] [review]
part.2 Implement nsIWidget::NotifyIME() on Windows

The change for Windows is very simple :-)
Attachment #720994 - Flags: review?(jmathies)
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().
Attachment #720995 - Flags: review?(smichaud)
Comment on attachment 720998 [details] [diff] [review]
part.5 Implement nsIWidget::NotifyIME() on Android

Just merges the methods to the new NotifyIME().
Attachment #720998 - Flags: review?(nchen)
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.
Attachment #720997 - Flags: review?(karlt)
FYI: The other platforms (qt, gonk and os2) don't implement any methods. So, we don't need to change on them.
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).
Attachment #720997 - Flags: review?(karlt) → review+
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?
Attachment #720998 - Flags: review?(nchen) → review+

Updated

6 years ago
Attachment #720994 - Flags: review?(jmathies) → review+
(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 on attachment 720995 [details] [diff] [review]
part.3 Implement nsIWidget::NotifyIME() on Cocoa

Looks fine to me.
Attachment #720995 - Flags: review?(smichaud) → review+
Created attachment 721499 [details] [diff] [review]
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.
Attachment #720997 - Attachment is obsolete: true
Attachment #721499 - Flags: review?(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?
Attachment #721499 - Flags: review?(karlt) → review+
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.
Keywords: inputmethod
You need to log in before you can comment on or make changes to this bug.