GeckoEditable should handle cancel/commit composition requests

RESOLVED FIXED in Firefox 40

Status

()

Firefox for Android
Keyboards and IME
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

Trunk
Firefox 40
All
Android
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Right now GeckoEditable passes those requests to GeckoInputConnection, but GeckoEditable should handle them itself. The reason is that when GeckoEditable receives NOTIFY_IME_TO_COMMIT_COMPOSITION or NOTIFY_IME_TO_CANCEL_COMPOSITION, the composition has already changed on the Gecko side. GeckoEditable should update the text to reflect the changes, instead of asking GeckoInputConnection to make more changes to the composition.
(Assignee)

Comment 1

3 years ago
Created attachment 8589364 [details] [diff] [review]
Handle commit/cancel composition events in GeckoEditable (v1)

In the case of committing a composition, because Gecko already committed the composition, GeckoEditable should remove composition spans from the text and send a text notification.

In the case of canceling a composition, because Gecko already removed the composition, GeckoEditable should already be updated through text change notifications.
Attachment #8589364 - Flags: review?(esawin)
Comment on attachment 8589364 [details] [diff] [review]
Handle commit/cancel composition events in GeckoEditable (v1)

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

I'm not sure I understand the GeckoInputConnection changes, what was the rationale behind restarting the input before and why do we not need it any more?

::: mobile/android/base/GeckoEditable.java
@@ +765,5 @@
>              return;
> +
> +        } else if (type == NOTIFY_IME_TO_COMMIT_COMPOSITION) {
> +            // Gecko already committed its composition, and
> +            // we should remove the composiion on our side as well.

s/composiion/composition

@@ +769,5 @@
> +            // we should remove the composiion on our side as well.
> +            boolean wasComposing = false;
> +            final Object[] spans = mText.getSpans(0, mText.length(), Object.class);
> +
> +            if (spans == null) {

The docs says the return value is T[], no mention of null. I would expect that we just get an empty array when no spans are available.

@@ +802,5 @@
> +                final Object[] spans = mText.getSpans(0, mText.length(), Object.class);
> +                if (spans != null) {
> +                    for (Object span : spans) {
> +                        if ((mText.getSpanFlags(span) & Spanned.SPAN_COMPOSING) != 0) {
> +                            throw new IllegalStateException("composition not cancelled");

If this is a new approach to throw exceptions only in debug builds, I like it, but we should probably log a warning or error in all builds.

@@ +814,1 @@
>          geckoPostToIc(new Runnable() {

The structure of this function is inconsistent, mixing if/else blocks and early returns. Why not just move this into an else block and avoid the early exits?
Attachment #8589364 - Flags: review?(esawin) → feedback+
(Assignee)

Comment 3

3 years ago
Created attachment 8591772 [details] [diff] [review]
Handle commit/cancel composition events in GeckoEditable (v2)

(In reply to Eugen Sawin [:esawin] from comment #2)
> Comment on attachment 8589364 [details] [diff] [review]
> Handle commit/cancel composition events in GeckoEditable (v1)
> 
> Review of attachment 8589364 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure I understand the GeckoInputConnection changes, what was the
> rationale behind restarting the input before and why do we not need it any
> more?

I think restarting the keyboard was a bandaid to get keyboards to play nicely, when we cancel or commit the composition on our side. With this patch I think we don't need the bandaid anymore.

> @@ +769,5 @@
> > +            // we should remove the composiion on our side as well.
> > +            boolean wasComposing = false;
> > +            final Object[] spans = mText.getSpans(0, mText.length(), Object.class);
> > +
> > +            if (spans == null) {
> 
> The docs says the return value is T[], no mention of null. I would expect
> that we just get an empty array when no spans are available.

I seemed to remember Android sources checking for null when using getSpans, but I can't find it anymore, so I removed the null check.

> @@ +802,5 @@
> > +                final Object[] spans = mText.getSpans(0, mText.length(), Object.class);
> > +                if (spans != null) {
> > +                    for (Object span : spans) {
> > +                        if ((mText.getSpanFlags(span) & Spanned.SPAN_COMPOSING) != 0) {
> > +                            throw new IllegalStateException("composition not cancelled");
> 
> If this is a new approach to throw exceptions only in debug builds, I like
> it, but we should probably log a warning or error in all builds.

I think it depends on the situation. In this case, we want an assert: we check and throw in debug, but do nothing in release because we assume it's working correctly. If we cannot make that assumption in release, we should throw in release as well.

> @@ +814,1 @@
> >          geckoPostToIc(new Runnable() {
> 
> The structure of this function is inconsistent, mixing if/else blocks and
> early returns. Why not just move this into an else block and avoid the early
> exits?

I think we should return early if we can. I did move some of the code blocks in to separate methods to make things cleaner.
Attachment #8589364 - Attachment is obsolete: true
Attachment #8591772 - Flags: review?(esawin)
Comment on attachment 8591772 [details] [diff] [review]
Handle commit/cancel composition events in GeckoEditable (v2)

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

Looks good with a nit (null check) addressed.

::: mobile/android/base/GeckoEditable.java
@@ +766,5 @@
> +        // Composition should have been cancelled on our side
> +        // through text update notifications; verify that here.
> +        if (DEBUG) {
> +            final Object[] spans = mText.getSpans(0, mText.length(), Object.class);
> +            if (spans != null) {

This should be redundant, it's also inconsistent to notifyCommitComposition.

@@ +816,1 @@
>          geckoPostToIc(new Runnable() {

I think this looks like a classical if/else or switch block, the early exits imply to me that there are non-exit paths within the other conditional blocks, but there aren't any. But I think it's OK this way, if you prefer it, as long as it's consistent.
Attachment #8591772 - Flags: review?(esawin) → review+
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/25be62f13677
https://hg.mozilla.org/mozilla-central/rev/25be62f13677
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
You need to log in before you can comment on or make changes to this bug.