TextEditingDelta support for Android #28175

Renzo-Olivares · 2021-08-18T22:44:51Z

Description

This change aims to collect the changes made to the editing value in the Android text input plugin, and send them to the framework through a platform channel.

Framework PR: flutter/flutter#88477

Related Issues

Partially fixes flutter/flutter#87972

Tests

Added tests for TextEditingDelta verifying the creation of various deltas.

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

justinmc

Thanks for building this, it really makes the design doc more real. I left a bunch of questions in the comments, but I think the biggest decisions to make are:

Are we sending the right data? Does it meet the needs to the developers that will use this, and are we able to send this data for all of Flutter's platforms?
Is this free of asynchronous race condition bugs? Maybe with this simple API, it's up to the consumer in the framework to avoid async problems.

justinmc · 2021-08-18T23:03:48Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

+  @Override
+  public SpannableStringBuilder append(char text) {
+    Log.e("DELTAS", "insert #1 is called");
+    return append(String.valueOf(text));


I guess this is the actual implementation of this append, you're just overriding it in order to be able to log here? And below.

Oh and you don't seem to need to override these to determine the type, you can just infer it inside of replace.

Yeah all these overrides where just to see if we actually hit these calls.

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

justinmc · 2021-08-18T23:33:01Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

          selectionEnd,
          composingStart,
          composingEnd);
+      textInputChannel.updateEditingStateWithDelta(


You may have explained this to me already somewhere, but why not send this as a part of updateEditingState?

I think putting it in updateEditingState implies that we are going to be updating the TextEditingValue with it, but in fact they are two separate entities. One may want to subscribe to the TextEditingDelta without the TextEditingValue to create their own model.

True... Do you still have a POC somewhere of actually creating a simple rich text editor in the framework? Did you use the TextEditingValue coming from updateEditingState at all, or did you do everything with the deltas only?

I'm thinking that if you did need both, it may be very hard to correlate the calls to updateEditingState and updateEditingStateWithDelta with each other in the framework since they are called asynchronously.

In that POC I did everything with TextEditingValue, and just used the deltas as extra information to expand/contract ranges.

I agree a developer should only subscribe to one or the other. I think how it would work on this new model is that a developer either subscribes to updateEditingValue or updateEditingValueWithDelta. If using updateEditingValue then they are using the plain text model where the TextEditingValue is always replaced by the new one. If using updateEditingValueWithDelta they subscribe to deltas that they can apply to their TextEditingValue/or their custom equivalent. This delta should also include the new composing/selection region from the engine so a developer could also apply that to their model.

In regards to how framework updates will be consumed by the engine for now the framework would keep using setEditingState, completely replacing the TextEditingValue on the engine side.

justinmc · 2021-08-18T23:35:30Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

+  // Fields for delta.
+  private String oldText;
+  private String deltaText;
+  private String deltaType;
+  private int deltaStart;
+  private int deltaEnd;


Would it be cleaner to have a separate TextEditingDelta class?

Definitely!

justinmc · 2021-08-18T23:37:12Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

+        "DELTAS",
+        "replace(" + start + ", " + end + ", " + tb + ", " + tbstart + ", " + tbend + ")");
+
+    final boolean isDeletionGreaterThanOne = end - (start + tbend) > 1;


If we had a TextEditingDelta class (as I mentioned above), then maybe we could encapsulate most of this logic.

justinmc · 2021-08-18T23:38:50Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

+    } else if (isReplaced) {
+      Log.e("DELTAS", "REPLACEMENT");
+      setDeltas(toString(), tb.subSequence(tbstart, tbend).toString(), "REPLACEMENT", start, end);
+    }


Is it ever possible that setDeltas won't be called here? It seems like bugs could arise from that, like sending old deltas data in updateEditingStateWithDelta.

And is it ever possible that some crazy change could happen that wouldn't be supported by a single TextEditingDelta? Maybe Android is massaging these types of changes into a single call to replace for you. Like what if you had two insertions at once?

"bbb" => "cbbbc"

Is there any way we can test something like that? I guess Android would just replace the entire "bbb" string rather than trying to do an insertion at the beginning and another at the end.

I think that might be possible not to hit setDeltas we definitely need to make sure to cover all the cases here. Yeah in the case you mention android would replace the entire string with "cbbbc". I think in this case all we can say is what the platform gives us, and in Android's case it is that the entire word was replaced.

justinmc · 2021-08-18T23:39:27Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

+
+    if (isEqual) {
+      Log.e("DELTAS", "EQUALITY");
+      setDeltas(toString(), "", "EQUALITY", -1, -1);


Would it be better to just not call updateEditingStateWithDelta at all in this case?

Yes, but it is called every time the editing state changes see https://github.com/flutter/engine/pull/28175/files#diff-6d16487c29b469c59861d81341897ed392331e365647f6c13c18459800cc854dR653 I think this is the place we want to put it to keep it in sync with the actual text editing changes.

The editing state could change for reasons not pertaining to the actual raw text chagning, such as a change in the selection/composing region, so I think we need this equality case for these cases.

Ah so if the user just moves the selection around with the arrow keys, they will get a bunch of EQUALITY deltas.

I wonder how exactly these calls would be used in the framework, similar to my other comment here, to help me see how these equality deltas would be used.

A use case of this could be.

Developer subscribes to updateEditingValueWithDelta. They update the currentTextEditingValue by applying the delta to it.

Say we are typing "hello"
Engine sends "h" delta
Framework receives "h" delta applies it to currentTextEditingValue.
Engine sends "e" delta
Framework receives "e" delta and applies it to currentTextEditingValue.
.
.
.
.
Engine sends "o" delta
Framework receives "o" delta and applies it to currentTextEditingValue.
User changes selection
Engine sends equality delta to framework
Framework receives equality delta and does nothing (or updates their current selection/composing region with the new one).

Writing this makes me think that we should also be sending the new selection/composing region with this delta. So when a developer gets a delta from updateEditingValueWithDelta they also get the updated selection/composing region they can apply to the TextEditingValue/whatever structure they have representing a TextEditingValue. Any thoughts on this?

justinmc · 2021-08-18T23:54:34Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

      Log.e(TAG, "editing state should not be changed in a listener callback");
    }

+    Log.e(


Weill replace be called when the engine receives a text change from the framework?

Yes replace is called when the engine receives a text change. It replaces the entire value with the one from the framework.

Is it useful to setDeltas in that case? Could that delta ever end up being sent back to the framework when it shouldn't be?

justinmc

Looks much cleaner with the TextEditingDelta class, though I left one comment about maybe refactoring it a bit.

I also responded to some comments outside of this review above accidentally, FYI.

justinmc · 2021-08-19T19:12:06Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+    return deltaEnd;
+  }
+
+  private void setDeltas(


Would it be cleaner to create a new TextEditingDelta instance rather than always mutating one instance? That would definitely be my recommendation in Dart though I'm less confident about these patterns in Java.

Then reasonDeltas could be a constructor.

justinmc · 2021-08-19T19:13:11Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+    deltaType = type;
+  }
+
+  public void reasonDeltas(


When you get to writing tests, it will be nice to be able to test all of this logic directly without needing to spin up a TextInputChannel and everything.

justinmc

Check out my comments but I think this is otherwise ready to open to broader review.

justinmc · 2021-08-30T18:01:22Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

      mImm.restartInput(view);
      mRestartInputPending = false;
+    } else {
+      mEditable.clearBatchDeltas();


Nit: Consider commenting here and other places where clearBatchDeltas is called to explain why it's being called there. At a high level it makes sense to clear it when the framework is sending an update, but it seems really nuanced to know when exactly to clear. If it's easily explainable then a comment might be good.

justinmc · 2021-08-30T18:04:52Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

+        // Make sure last delta has the most up to date composing region.
+        // This can happen when the IME is restarted because the framework has changed the
+        // composing region. We do not have access to the new composing region until after
+        // the restart has completed.


Is there a reason to special case the composing region change caused by IME? Shouldn't the information already be captured by the deltas recorded by the ListenableEditingState class?

For some reason when I capture it in ListenableEditingState the composing region is still (-1,-1). It is not until I arrive at ListenableEditingState that I'm able to grab the most up to date composing regions. I think this is somewhat consistent to how updateEditingState works, where we send the composing/selection regions we grab in didChangeEditingState.

I have observed that this is only the case when the composing region restarts. In other cases the selection/composing regions are up to date after the editable has been mutated with replace.

justinmc · 2021-08-30T18:06:22Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

+        }
+
+        textInputChannel.updateEditingStateWithDeltas(inputTarget.id, batchTextEditingDeltas);
+        mEditable.clearBatchDeltas();


This clearBatchDeltas makes sense!

justinmc · 2021-08-30T18:08:50Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

+
+        if (autofill.uniqueIdentifier.equals(currentAutofill.uniqueIdentifier)) {
+          // Autofilling the current client is the same as handling user input
+          // from the virtual keyboard. Setting the editable to newState and an


By "virtual keyboard" do you mean the usual Android on-screen keyboard? I think you should refer to it as the "soft keyboard" based on my understanding of Flutter's usual terminology, unless you've seen this way elsewhere. In the framework there is a PR referring to virtual keyboards as keyboards implemented in-app in Flutter itself (flutter/flutter#68988), so that made me notice this.

Eh I guess we do have virtualId here... I guess the naming is up to you, whatever you think is best.

The virtualId here is the id of the virtual view structure used for autofill.

justinmc · 2021-08-30T18:12:55Z

shell/platform/android/test/io/flutter/plugin/editing/TextEditingDeltaTest.java

+    assertEquals(0, delta.getNewComposingStart());
+    assertEquals(5, delta.getNewComposingEnd());
+  }
+}


Thanks for thoroughly testing the TextEditingDelta constructor!

LongCatIsLooong · 2021-08-30T21:58:49Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/TextInputChannel.java

        "TextInputClient.updateEditingStateWithTag", Arrays.asList(inputClientId, json));
  }

+  public void updateEditingStateWithDeltasWithTag(


This method is for autofill only (which generally just replaces all the current content of the text field). So it probably shouldn't be using deltas. Also it's unlikely that users would want to autofill a rich text field.

Makes sense, responded to this topic in another comment.

LongCatIsLooong · 2021-08-30T22:17:39Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

+        // This can happen when the IME is restarted because the framework has changed the
+        // composing region. We do not have access to the new composing region until after
+        // the restart has completed.
+        if (mEditable.getBatchTextEditingDeltas().size() - 1 >= 0) {


nit: collection.size() - 1 >= 0 => !collection.isEmpty() (not sure if there's an isNotEmpty() method).

Also the else if can just be else I think?

LongCatIsLooong · 2021-08-30T22:24:08Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

+          mEditable.popTextEditingDeltaFromList();
+          mEditable.addTextEditingDeltaToList(delta);
+        } else if (mEditable.getBatchTextEditingDeltas().size() == 0) {
+          // Make sure we always have at least one delta when triggering a framework update, in


What would be a case that there's no deltas recorded but the engine has a different text editing value and needs to update the framework?

I'm not sure I follow this example. If the engine has changed its text editing value then it has most likely gone through the replace method and generated a delta.

LongCatIsLooong · 2021-08-30T22:28:28Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

+        // Make sure last delta has the most up to date composing region.
+        // This can happen when the IME is restarted because the framework has changed the
+        // composing region. We do not have access to the new composing region until after
+        // the restart has completed.


Is there a reason to special case the composing region change caused by IME? Shouldn't the information already be captured by the deltas recorded by the ListenableEditingState class?

LongCatIsLooong · 2021-08-30T22:35:50Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

-    final HashMap<String, TextInputChannel.TextEditState> editingValues = new HashMap<>();
-    for (int i = 0; i < values.size(); i++) {
-      int virtualId = values.keyAt(i);
+    if (configuration.enableDeltaModel) {


We probably don't want to enable delta model for autofill?

I thought about this, I had included it just because I was trying to get every feature that works with the TextField on the engine side working with this delta model. I agree rich text editors will probably not be using autofill, do you think it is alright to not include this then? I was thinking this might be good to have for the future but I guess are plans for this are not concrete.

I think the thing autofill does is slightly different from any of the delta types specified: it replaces the entirety of the document, instead of the content in [0, currentLength). They're slightly different since we're dealing with async communications.

For example if the user autofills a password field and at the same time the framework changes the text to be longer, then we want the obscured field has the right characters in it, instead of just replacing the original range.

LongCatIsLooong · 2021-08-30T22:53:31Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

+  }
+
+  public void popTextEditingDeltaFromList() {
+    mBatchTextEditingDeltas.remove(mBatchTextEditingDeltas.size() - 1);


nit: do java lists have "pop()" or something similar?

I did not find an equivalent in the docs.

LongCatIsLooong · 2021-08-30T23:26:28Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+
+  private static final String TAG = "TextEditingDelta";
+
+  public TextEditingDelta(


I was expecting that we can just extract the tb{start, end}, start, end and tb from the replace implementation and send them verbatim to the framework, and the framework will have to implement the same replace method to apply it to a TextEditingValue. Would that not work?

That wouldn't work in some cases. Say the case where we are typing "world".

Current state: "worl|"
soft keyboard input: "d"
replace method call: replace(0,4, world, 0, 5)

or for a deletion

Current state: "world|"
soft keyboard input: backspace
replace method call: replace(0,5, worl, 0, 4)

If typing at the end of the composing region the text in the composing region is replaced with the entire new composing text.

If we move the cursor inside the word currently being composed like below we get this call.
current state: "wo|ld"
soft keyboard input: "r"
replace method call: replace(2,2, r, 0,1)

Here we are actually given the insertion point and the character inserted, where when it is at the end of the composing region we must do some reasoning.

On iOS we have something slightly different as well.

Current state: "world|"
soft keyboard input: backspace
replaceRangeWithLocal (4,5) with empty string ranges (0,0)

another thing iOS does is when typing with a CJK keyboard it will exhibit behavior similar to Android composing region where the entire composing region is replaced on input.

Because these values can vary a bit by platform for the same operation I think it is important to unify them into one format. I'm unsure if a replacement of "world" with "worl" would have the same result as replacing "d" in "world" with an empty string regarding if ranges of "world" where bolded/styled. Would these ranges still have the information needed to contract/expand as expected?

Ah makes sense, thank you for the clarification! Do you think we'll be able to extract that info directly from the InputConnection interface (instead of replace)? It doesn't have to be implemented that way right now if it is (it's probably going to be a little complicated to implement since InputConnectionAdaptor and TextInputPlugin currently don't directly talk to each other). I'm just wondering if we can avoid the manual diffing process here.

Unfortunately from my research of the API I don't think so. Even the InputConnectionAdaptor does not provide these granular differences between editing states.

InputConnectionAdaptor.commitText and InputConnectionAdaptor.setComposingText both call their super class BaseInputConnection which calls replaceText() and routes that to SpannableStringBuilder.replace(). Only in some cases will the IME actually provide granular changes like in InputConnectionAdaptor.deleteSurroundingText which does call SpannableStringBuilder.delete() that is in the end also routed to replace(). In most cases for Android since the composing region is always active we are always replacing the entire composing region.

Maybe I missed something but this is my conclusion after my research.

I see. So setComposingText and commit* can delete/insert/replace, but they all only operate on a specific range.

I'd prefer a bit more documentation in this class. If this were the entrypoint for someone learning about this feature, the documentation here is a bit lacking to properly introduce what this does and find the rest of the code.

LongCatIsLooong · 2021-08-30T23:28:53Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

        };
  }

+  public ArrayList<TextEditingDelta> getBatchTextEditingDeltas() {


nit: maybe we should just have a "pop" method that returns the deltas and clear the batched deltas.

Every time you retrieve the deltas you should typically delete them from ListenableEditingState too I think?

Yeah that's correct this makes sense then.

nit: get implies idempotency. A different verb maybe?

I agree that I wouldn't expect the .clear() with the "get" name. Maybe... "flush"? Maybe not...

Also, should clearBatchDeltas be clearBatchTextEditingDeltas? Since this method name refers to TextEditingDeltas.

LongCatIsLooong · 2021-08-30T23:30:22Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

+
+        if (autofill.uniqueIdentifier.equals(currentAutofill.uniqueIdentifier)) {
+          // Autofilling the current client is the same as handling user input
+          // from the virtual keyboard. Setting the editable to newState and an


The virtualId here is the id of the virtual view structure used for autofill.

LongCatIsLooong · 2021-09-02T20:35:37Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+
+  private static final String TAG = "TextEditingDelta";
+
+  public TextEditingDelta(


Ah makes sense, thank you for the clarification! Do you think we'll be able to extract that info directly from the InputConnection interface (instead of replace)? It doesn't have to be implemented that way right now if it is (it's probably going to be a little complicated to implement since InputConnectionAdaptor and TextInputPlugin currently don't directly talk to each other). I'm just wondering if we can avoid the manual diffing process here.

LongCatIsLooong · 2021-09-02T20:38:41Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

        };
  }

+  public ArrayList<TextEditingDelta> getBatchTextEditingDeltas() {


Every time you retrieve the deltas you should typically delete them from ListenableEditingState too I think?

LongCatIsLooong · 2021-09-02T20:46:36Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

+        new TextEditingDelta(
+            toString(),
+            "",
+            "TextEditingDeltaType.equality",


nit: I think "equality" is a confusing name. "cursorUpdate"? Or "selectionOnly"?

Equality could also represent a composing region update. Maybe regionUpdate or selectionOrComposingRegionUpdate, nonTextUpdate?

LongCatIsLooong · 2021-09-02T22:01:00Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

-    final HashMap<String, TextInputChannel.TextEditState> editingValues = new HashMap<>();
-    for (int i = 0; i < values.size(); i++) {
-      int virtualId = values.keyAt(i);
+    if (configuration.enableDeltaModel) {


I think the thing autofill does is slightly different from any of the delta types specified: it replaces the entirety of the document, instead of the content in [0, currentLength). They're slightly different since we're dealing with async communications.

For example if the user autofills a password field and at the same time the framework changes the text to be longer, then we want the obscured field has the right characters in it, instead of just replacing the original range.

LongCatIsLooong · 2021-09-02T22:06:47Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+    setDeltas(oldText, deltaText, deltaType, deltaStart, deltaEnd);
+  }
+
+  public void setNewComposingStart(int newStart) {


Do you still need to expose these methods? Would it be possible to make this class immutable?

I think this is possible now.

LongCatIsLooong

The approach LGTM.

LongCatIsLooong · 2021-09-03T20:16:44Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+    newComposingStart = composingStart;
+    newComposingEnd = composingEnd;
+    final boolean isDeletionGreaterThanOne = end - (start + tbend) > 1;
+    final boolean isCalledFromDelete = tb == "" && tbstart == 0 && tbstart == tbend;


is the tb == "" check because of the delete implementation here?
https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/android/text/SpannableStringBuilder.java;l=230-237?q=spannablestringbuilder

Yeah that's where I got that from.

LongCatIsLooong · 2021-09-03T20:28:59Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+
+  private static final String TAG = "TextEditingDelta";
+
+  public TextEditingDelta(


I see. So setComposingText and commit* can delete/insert/replace, but they all only operate on a specific range.

LongCatIsLooong · 2021-09-07T16:46:57Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

        };
  }

+  public ArrayList<TextEditingDelta> getBatchTextEditingDeltas() {


nit: get implies idempotency. A different verb maybe?

LongCatIsLooong · 2021-09-07T16:50:29Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

+    super.setSpan(what, start, end, flags);
+    // Setting a span does not involve mutating the text value in the editing state. Here we create
+    // a non text update delta with any updated selection and composing regions.
+    mBatchTextEditingDeltas.add(


Is it possible for setSpan to be invoked from within the replace method? If it is then when that happens we'll see an extra nonTextUpdate for the change?

It is possible but it shouldn't affect the final result since they are nonTextUpdates. In the end the final TextEditingDelta in the batch will have the most up to date selection/composing regions. In theory if setSpan is called in replace we will want to capture that anyways.

The batch may look something like this if super.replace() calls setSpan.
[nonTextUpdate, nonTextUpdate, nonTextUpdate, delta]
In this case the last delta will still have the most up to date composing/selection regions. We capture the delta after super.replace() has been called so the actual delta will always be the last one applied.

Ideally there should be no overlap between consecutive deltas and there should be no redundant deltas. Should be fine for now.

LongCatIsLooong · 2021-09-07T17:25:45Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+    setDeltas(oldText, deltaText, deltaType, deltaStart, deltaEnd);
+  }
+
+  public CharSequence getOldText() {


Do you need these getters? The text input plugin only need to be able to build deltas and serialize them to JSON right? Are these for writing tests?

Yes there are now purely for tests.

Nit: Maybe write a comment or comments stating that these are for testing only.

I don't know java too well but do you need expose them as public if the test is in the same package?

Yeah I think we do. I just tried to switch them to private and it complained.

justinmc

Looks a lot more straightforward now that a lot of delta logic has been moved to the framework 👍

justinmc · 2021-09-07T21:44:30Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

        };
  }

+  public ArrayList<TextEditingDelta> getBatchTextEditingDeltas() {


I agree that I wouldn't expect the .clear() with the "get" name. Maybe... "flush"? Maybe not...

Also, should clearBatchDeltas be clearBatchTextEditingDeltas? Since this method name refers to TextEditingDeltas.

justinmc · 2021-09-07T21:46:59Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

      Log.e(TAG, "editing state should not be changed in a listener callback");
    }

+    Log.v(TAG, "replace(" + start + ", " + end + ", " + tb + ", " + tbstart + ", " + tbend + ")");


Did you intend to remove this Log?

Yeah I just cleaned up the logs.

justinmc · 2021-09-07T22:04:57Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+    setDeltas(oldText, deltaText, deltaType, deltaStart, deltaEnd);
+  }
+
+  public CharSequence getOldText() {


Nit: Maybe write a comment or comments stating that these are for testing only.

justinmc · 2021-09-07T22:05:30Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+
+  private void setDeltas(CharSequence oldTxt, CharSequence newTxt, int newStart, int newExtent) {
+    oldText = oldTxt;
+    deltaText = newTxt;


Why "Txt" instead of "Text"?

I did this avoid shadowing, but I can also just use this.

justinmc · 2021-09-07T22:06:47Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

  // latest TextEditState from the framework.
  @VisibleForTesting
  void setTextInputEditingState(View view, TextInputChannel.TextEditState state) {
+    Log.v(


Nit: Remove this Log?

Addressed in another comment.

justinmc · 2021-09-07T22:09:30Z

shell/platform/android/test/io/flutter/plugin/editing/TextEditingDeltaTest.java

+            newComposingEnd);
+
+    assertEquals(oldText, delta.getOldText());
+    assertEquals("hello", delta.getDeltaText());


Why the string literal instead of textAfterChange?

justinmc · 2021-09-07T22:21:19Z

shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java

+            false,
+            true,
+            true,
+            true, // Enable delta model.


Nit: What do you think of adding a test where this is false and asserting that there are no deltas created and no calls to updateEditingStateWithDeltas?

Added tests

Renzo-Olivares · 2021-09-08T16:25:46Z

Ready for re-review.

The main change is that the main logic for deltas has moved to the framework. There was a lot of common logic between the Android and iOS engine PR's and this helps centralize the logic.

justinmc

LGTM 👍

Caveat: My comment in the framework PR about the json format for batched deltas. Is we decide to change that, this PR will need a small change too.

justinmc · 2021-09-08T23:24:16Z

shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java

+            false,
+            true,
+            true,
+            false,


Nit: You have a comment "Enable delta model." on all the other tests where this is true, maybe add it here too.

justinmc · 2021-09-08T23:25:25Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/TextInputChannel.java

+    for (TextEditingDelta delta : batchDeltas) {
+      deltas.put(delta.toJSON());
+    }
+    state.put("batchDeltas", deltas);


I made a comment in the framework about how we send these batched deltas: flutter/flutter#88477 (comment)

This is all good now, we always send an array of batchDeltas.

GaryQian · 2021-09-09T05:28:24Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+      CharSequence oldEditable,
+      int start,
+      int end,
+      CharSequence tb,


write this out instead of an abbreviation

GaryQian · 2021-09-09T05:30:16Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+
+  public TextEditingDelta(
+      CharSequence oldEditable,
+      int start,


specify what kind of start/end. Since you have lots of similarly named vars, it is important to disambiguate whenever possible

GaryQian · 2021-09-09T05:37:49Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

+    // Setting a span does not involve mutating the text value in the editing state. Here we create
+    // a non text update delta with any updated selection and composing regions.
+    mBatchTextEditingDeltas.add(
+        new TextEditingDelta(


We should make a "nonTextUpdate" delta be its own constructor. It is not always obvious to pass "", -1, -1. A separate constructor for a separate case would prevent any accidental misuse or misconfiguration of this particular type of delta.

This makes sense, I do this already in the iOS PR.

LongCatIsLooong

LGTM module comments from other reviewers.

LongCatIsLooong · 2021-09-09T20:34:28Z

shell/platform/android/io/flutter/plugin/editing/ListenableEditingState.java

+    super.setSpan(what, start, end, flags);
+    // Setting a span does not involve mutating the text value in the editing state. Here we create
+    // a non text update delta with any updated selection and composing regions.
+    mBatchTextEditingDeltas.add(


Ideally there should be no overlap between consecutive deltas and there should be no redundant deltas. Should be fine for now.

LongCatIsLooong · 2021-09-09T20:36:35Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+    setDeltas(oldText, deltaText, deltaType, deltaStart, deltaEnd);
+  }
+
+  public CharSequence getOldText() {


I don't know java too well but do you need expose them as public if the test is in the same package?

GaryQian · 2021-09-14T03:02:13Z

shell/platform/android/io/flutter/plugin/editing/TextEditingDelta.java

+import org.json.JSONObject;
+
+/// A representation of the change that occured to an editing state, along with the resulting
+/// composing and selection regions.


Do we explicitly differentiate between text content changes and selection/composing region index only changes? However it is handled, it should be documented.

Never mind, that is under the umbrella of TextEditingDeltaNonTextUpdate

On the framework we do, but on the engine there is no differentiation. This differentiation is documented on the framework side, should it also have something here?

Edit: didn't see your comment before this, disregard.

…mework and when we skip updating the framework

…90152)

flutter-dashboard bot added the platform-android label Aug 18, 2021

google-cla bot added the cla: yes label Aug 18, 2021

This was referenced Aug 18, 2021

Framework can receive TextEditingDeltas from engine flutter/flutter#88477

Merged

Flutter's text input plugin should report deltas instead of full text blocks flutter/flutter#87972

Closed

justinmc reviewed Aug 18, 2021

View reviewed changes

justinmc reviewed Aug 19, 2021

View reviewed changes

Renzo-Olivares force-pushed the text_editing_deltas_android branch from 0447ae2 to 796a826 Compare August 29, 2021 02:55

Renzo-Olivares requested a review from justinmc August 30, 2021 16:13

justinmc reviewed Aug 30, 2021

View reviewed changes

Renzo-Olivares changed the title ~~[WIP] TextEditingDelta support for Android~~ TextEditingDelta support for Android Aug 30, 2021

Renzo-Olivares marked this pull request as ready for review August 30, 2021 19:47

Renzo-Olivares requested review from GaryQian and LongCatIsLooong August 30, 2021 20:33

LongCatIsLooong reviewed Aug 30, 2021

View reviewed changes

LongCatIsLooong reviewed Sep 2, 2021

View reviewed changes

LongCatIsLooong reviewed Sep 3, 2021

View reviewed changes

Renzo-Olivares force-pushed the text_editing_deltas_android branch from ed9bb5e to 29bcc62 Compare September 7, 2021 06:16

LongCatIsLooong reviewed Sep 7, 2021

View reviewed changes

Renzo-Olivares requested a review from justinmc September 7, 2021 20:58

justinmc reviewed Sep 7, 2021

View reviewed changes

Renzo-Olivares requested review from LongCatIsLooong and justinmc September 8, 2021 16:34

justinmc approved these changes Sep 8, 2021

View reviewed changes

GaryQian reviewed Sep 9, 2021

View reviewed changes

LongCatIsLooong approved these changes Sep 9, 2021

View reviewed changes

Renzo-Olivares requested a review from GaryQian September 10, 2021 17:18

GaryQian reviewed Sep 14, 2021

View reviewed changes

Renzo-Olivares added 24 commits September 15, 2021 12:40

formatting

c66555d

formatting

73c849e

Capture delta on setSpan and clear deltas when generated from the fra…

b54b8b1

…mework and when we skip updating the framework

fix formatting

9496733

update tests to reflect new clear batch delta calls

07f0c1d

Make deletion inline with platform

2326d7b

fix tests

510ca19

Address reviewer comments

14c5f12

formatting

12da161

fix tests

8d34321

fix tests

f858e9f

move delta inference to framework

e493f6e

update tests

7d2c858

formatting

d0659f0

fix tests

7bacbed

Formatting

88f1042

get -> extract

034f0e8

More tests and address reviewer comments

630b505

clean up logs

81232af

Address reviewer comments

4b25c8e

formatting

1cd7aa1

formatting

9d7b337

Remove testing constructor

72ab127

send as deltas

4a13b0a

Renzo-Olivares force-pushed the text_editing_deltas_android branch from f51c01c to 4a13b0a Compare September 15, 2021 19:41

Renzo-Olivares merged commit c148fe5 into flutter:master Sep 15, 2021

engine-flutter-autoroll mentioned this pull request Sep 15, 2021

Roll Engine from 6447a9451fc6 to c148fe5d3aa2 (1 revision) flutter/flutter#90152

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 15, 2021

c148fe5 TextEditingDelta support for Android (flutter/engine#28175)

fe9ead2

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Sep 16, 2021

c148fe5 TextEditingDelta support for Android (flutter/engine#28175) (#…

8de5600

…90152)

Renzo-Olivares mentioned this pull request Apr 24, 2023

[iOS] - Dictation deltas don't respect app-side content updates flutter/flutter#120724

Open


		private static final String TAG = "TextEditingDelta";

		public TextEditingDelta(

TextEditingDelta support for Android #28175

TextEditingDelta support for Android #28175

Uh oh!

Conversation

Renzo-Olivares commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Pre-launch Checklist

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares commented Aug 18, 2021 •

edited

Loading

justinmc left a comment •

edited

Loading