Closed Bug 1152123 Opened 7 years ago Closed 7 years ago

GeckoEditable should handle cancel/commit composition requests

Categories

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

All
Android
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: jchen, Assigned: jchen)

Details

Attachments

(1 file, 1 obsolete file)

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.
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+
(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+
https://hg.mozilla.org/mozilla-central/rev/25be62f13677
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.