Closed Bug 1149172 Opened 5 years ago Closed 5 years ago

Query IMEStateManager for composition state

Categories

(Firefox for Android :: Keyboards and IME, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(3 files)

Right now we keep tracking of the current composition state ourselves (nsWindow::mIMEComposing, nsWindow::mIMEComposingStart, etc.).

However, we should query IMEStateManager for that state and that should make things more consistent between widget and content.
Small patch to piggyback on this bug
Attachment #8585524 - Flags: review?(esawin)
Small patch to piggyback on this bug
Attachment #8585526 - Flags: review?(esawin)
Use IMEStateManager to query the current composition states.
Attachment #8585527 - Flags: review?(esawin)
Comment on attachment 8585524 [details] [diff] [review]
Fix small bugs in IME focus handshake (v1)

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

Looks good.

::: mobile/android/base/GeckoEditable.java
@@ +764,5 @@
>                  mActionQueue.syncWithGecko();
>                  mListener.notifyIME(type);
> +
> +                if (type == NOTIFY_IME_OF_BLUR) {
> +                    mFocused = false;

Maybe add a comment on why we shouldn't set this before unmasking events and syncing.
Attachment #8585524 - Flags: review?(esawin) → review+
Comment on attachment 8585526 [details] [diff] [review]
Send well-formed IME events (v1)

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

Looks good.
Attachment #8585526 - Flags: review?(esawin) → review+
Comment on attachment 8585527 [details] [diff] [review]
Query IMEStateManager for composition state (v1)

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

I like the simplified state machine, looks good.

::: widget/android/nsWindow.cpp
@@ +1673,5 @@
>  /*
> + * Get the current composition object, if any.
> + */
> +nsRefPtr<mozilla::TextComposition>
> +nsWindow::GetIMEComposition()

I wish we could declare this function const, but then the rest of the chain would need to const-correct first.

@@ +1785,5 @@
>                  Text updates are passed on, so the Java text can shadow the
>                    Gecko text
>              */
>              AutoIMEMask selMask(mIMEMaskSelectionUpdate);
> +            auto composition(GetIMEComposition());

TextComposition is mostly const-correct, we could declare this one const.

@@ +1926,5 @@
>                    temporary events are not passed on to Java
>              */
>              AutoIMEMask selMask(mIMEMaskSelectionUpdate);
>              AutoIMEMask textMask(mIMEMaskTextUpdate);
> +            auto composition(GetIMEComposition());

Const.
Attachment #8585527 - Flags: review?(esawin) → review+
You need to log in before you can comment on or make changes to this bug.