Framework can receive TextEditingDeltas from engine #88477

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

Description

This change implements a new channel for the TextInputClient.

TextInputClient.updateEditingValueWithDeltas(List<TextEditingDelta> deltas) will be used to receive changes made to the editing value from the engine.

This delta model is opt-inable through a flag in TextInputConfiguration called enableDeltaModel. This will tell the engine to use updateEditingValueWithDeltas instead of updateEditingValue.

Demo part 1	Demo part 2

final TextEditingController _controller = TextEditingController();

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            TextField(
              autofocus: true,
              controller: _controller,
              maxLines: null,
            ),
          ],
        ),
      ),
    );
  }

See #87972 (comment) for more details

Engine PR: flutter/engine#28175

Related Issues

Partially fixes #87972

Tests

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], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

justinmc

One quick comment, but actually I'm leaving all of my important comments to the engine PR, no need to reiterate here.

justinmc · 2021-08-18T23:55:47Z

packages/flutter/lib/src/services/text_input.dart

+  factory TextEditingDelta.fromJSON(Map<String, dynamic> encoded) {
+    TextEditingDeltaType? deltaType = null;
+
+    switch (encoded['delta'] as String) {


Maybe this could be a Map?

justinmc

I couldn't help myself leaving a bunch of nits even though I know this is a WIP PR, sorry, feel free to ignore for now. But also some thoughts about how to get this ready for merge and stuff.

justinmc · 2021-08-23T17:50:28Z

packages/flutter/lib/src/services/text_input.dart

@@ -671,6 +678,87 @@ class RawFloatingCursorPoint {
  final FloatingCursorDragState state;
 }

+/// A [TextEditingDeltaType.insertion] singifies there has been a single/sequence


Nit: I think usually you would have a description of the enum overall up here, and then split the enum values into different lines and document them down below. Maybe TextAffinity as an example.

justinmc · 2021-08-23T17:52:43Z

packages/flutter/lib/src/services/text_input.dart

+  /// The old text state before the delta has occured.
+  final String oldText;
+
+  /// The raw delta value.


I'd still be confused what this is. Maybe explain what it is in the case of an insertion and replacement (and for other types it's empty if that's true)?

packages/flutter/lib/src/services/text_input.dart

justinmc · 2021-08-23T17:53:40Z

packages/flutter/lib/src/services/text_input.dart

+  /// The new [TextRange] as a result of the delta.
+  final TextRange deltaRange;
+
+  final TextSelection selection;


Docs here and for composing below. Maybe you can use a macro to take them from TextEditingValue.

justinmc · 2021-08-23T17:54:08Z

packages/flutter/lib/src/services/text_input.dart

+  /// See [TextEditingDeltaType] for more information.
+  final TextEditingDeltaType deltaType;
+
+  /// The new [TextRange] as a result of the delta.


I'd also still be confused about this. Maybe explain by-type again.

justinmc · 2021-08-23T17:55:23Z

packages/flutter/lib/src/services/text_input.dart

+      deltaRange: TextRange(
+        start: encoded['deltaStart'] as int? ?? -1,
+        end: encoded['deltaEnd'] as int? ?? -1,


@LongCatIsLooong I know we've been trying to move away from using -1 to represent invalid. Is that something we can start to do here, or should we just follow the existing pattern for now?

No unfortunately. That's one large breaking change.

justinmc · 2021-08-23T17:57:08Z

packages/flutter/lib/src/services/text_input.dart

+  });
+
+  /// The old text state before the delta has occured.
+  final String oldText;


I know we were talking about the ability to save space by only sending the deltas and not the whole document each time (at some point in the future). However, if we're still sending the entire oldText, we won't have any savings. Could this theoretically be a hash or something in the future?

We might actually be able to remove oldText all together what do you think? The framework technically has the oldText already so I don't think we need to send it.

It feels a little bit weird to send a delta without any indication of what it applies to, but I guess it makes sense to remove it for now since we currently haven't seen a need for it. Keep an eye out for bugs with the engine and framework getting out of sync though. If that seems possible then maybe we can add a hash in the future.

Update: Seems like we're keeping it in order to make sure the delta applies to the current text, for now, but we could investigate removing it for space savings later.

justinmc · 2021-08-23T18:00:27Z

packages/flutter/lib/src/widgets/editable_text.dart

@@ -55,6 +55,49 @@ const Duration _kCursorBlinkWaitForStart = Duration(milliseconds: 150);
 // is shown in an obscured text field.
 const int _kObscureShowLatestCharCursorTicks = 3;

+class DeltaTextEditingController extends TextEditingController {
+  TextEditingValue applyDeltas(List<TextEditingDelta> deltas) {


This method should be thoroughly tested (when everything is finalized).

justinmc · 2021-08-23T18:02:24Z

packages/flutter/lib/src/widgets/editable_text.dart

@@ -2625,6 +2674,7 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
        currentEditingValue: currentTextEditingValue,
      ),
      enableIMEPersonalizedLearning: widget.enableIMEPersonalizedLearning,
+      enableDeltaModel: true, //Proof of concept.


When this is finalized, maybe we could move DeltaTextEditingController out of the framework as an example, but add to the framework the updateEditingValueWithDeltas and enableDeltaModel parameters to all of our text editing classes? Or what were you thinking?

Initially I thought we should obscure enableDeltaModel and keep it limited to TextInputConfiguration, but this would lock out developers that want to use a TextField or EditableText as a base for their rich text editors. However, I think to fully support those developers we will need to finish implementing deltas for framework text editing actions/shortcuts etc as well. So for now I think we should keep it limited to TextInputConfiguration until we are able to implement the framework side of the deltas. This gives developers who are aiming to build more complex editors to choose whether to subscribe to the delta model or the plain text model.

DeltaTextEditingController should definitely move out of the framework until we decide how we want to incorporate TextEditingDeltas into our own editable text widgets as the source of truth.

Discussed offline: For now keep it limited to TextInputConfiguration. If/when we want to open it up to EditableText in the future, maybe we could add enableDeltaModel and updateEditingValueWithDeltas params.

Are we proposing to put enableDeltaModel in production now?

justinmc · 2021-08-27T18:21:10Z

packages/flutter/lib/src/widgets/editable_text.dart

+  DeltaTextEditingController({ String? text })
+      : super.fromValue(text == null ? TextEditingValue.empty : TextEditingValue(text: text));
+
+  TextEditingValue applyDeltas(List<TextEditingDelta> deltas) {


What if you add an apply method to TextEditingDelta that takes a TextEditingValue and returns a new TextEditingValue with the delta applied? Then in each subclass of TextEditingDelta you can put the specific logic there rather than using a switch here.

Also, maybe you could get rid of deltaType altogether. If you really do need to check the type, you could do delta is TextEditingDeltaInsertion or whatever.

I think we should keep deltaType to avoid having the developer use dynamic type checking through is. The is check is discourage in our style guide.

Also we talked offline about the apply method and the conclusion is that it may tie TextEditingDelta and TextEditingValue together too closely... Interested in what others think though.

justinmc

I also think this is ready to open to review. The architecture here seems pretty minimal in a good way to me. I'm interested in what everyone else thinks of creating the enableDeltaModel flag for updateEditingValueWithDeltas, which then just calls through to updateEditingValue.

justinmc · 2021-08-30T18:35:14Z

packages/flutter/lib/src/services/text_input.dart

@@ -607,6 +609,11 @@ class TextInputConfiguration {
  /// {@endtemplate}
  final bool enableIMEPersonalizedLearning;

+  /// Whether to enable that the engine sends text input updates to the
+  /// framework as [TextEditingDelta]'s or as one [TextEditingValue].
+  /// Defaults to false. Cannot be null.


Nit: Should be an empty line above this one:

/// /// Defaults to false. Cannot be null.

Also, is this the place where we should thoroughly explain what this is for? You could mention updateEditingValueWithDeltas and updateEditingValue, and maybe mention that updateEditingValue doesn't work when this is true (is that right?).

Long term it would also be cool to have an example of a simple rich text editor or something in the docs somewhere. Or maybe your demo of just showing the current change.

justinmc · 2021-08-30T18:39:47Z

packages/flutter/lib/src/services/text_input.dart

+  equality,
+
+  /// If ever created this delta should be thrown away.
+  none


Comma here. Are we sure that none is needed now?

It isn't. I had it in there because TextEditingDelta was not abstract but we can make it abstract.

justinmc · 2021-08-30T18:40:13Z

packages/flutter/lib/src/services/text_input.dart

+
+  /// Replaces a range of text in the original string with the text given in the
+  /// replacement string.
+  String replace(String originalText, String replacementText, int start, int end) {


Could this be static?

Is there a reason it's made a public method (and in a mixin)? Would it be possible to just leave it as a private global function?

+1, I can't see why this should be a mixin.

justinmc · 2021-08-30T18:40:36Z

packages/flutter/lib/src/services/text_input.dart

+/// This class should not be used directly, and should be extended for different
+/// types of deltas.


Should it be abstract then?

Yeah this makes sense. Can't remember why I didn't make it abstract to begin with.

Nit: probably no need to mention that it shouldn't be used directly now that it's not possible to use it directly.

justinmc · 2021-08-30T18:47:11Z

packages/flutter/lib/src/services/text_input.dart

+  /// The old text state before the delta has occured.
+  final String oldText;
+
+  /// This value will slightly vary based on the [TextEditingDeltaType].


I would make this the second line and add a new first line that summarizes this more explicitly, maybe like "Any String needed to clarify the delta, depending on the type." I don't know, trying to make this fit our style of having a brief summary as the first sentence: https://dart.dev/guides/language/effective-dart/documentation#do-start-doc-comments-with-a-single-sentence-summary

Same for delatRange below.

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

packages/flutter/lib/src/services/text_input.dart

+  TextEditingValue apply(TextEditingValue value) {
+    return apply(value);


I'm confused about where this apply comes from but maybe I just missed something... Is this a method that a subclass must implement itself?

Yeah each subclass implements this method themselves.

By making the class abstract it makes this a little more clear that each subclass should implement it itself.

justinmc · 2021-08-30T18:56:44Z

packages/flutter/lib/src/services/text_input.dart

+        // an editing state are accumulated before being sent to the framework.
+        // This is called batch editing on Android, and on other platforms the
+        // TextInputPlugin does not adhere to this type of editing.
+        if (encoded['batchDeltas'] != null) {


I'm interested in what other reviewers think of this. In some cases multiple deltas are sent in one updateEditingValueWithDeltas, and in others just one is sent. Is this the cleanest way to do that?

Yeah I have the same question. Since framework is not consuming the "isBatched" information, would it be OK if we always send a list of deltas?

@Renzo-Olivares Was there ever any more discussion about this?

I am ok with always sending a list, even if only one is included. The complexity of a boolean check here is more confusing than just a static batch model.

On a different thought, the json probably needs to be a map anyways. We need to added a global timestamp to the message in the future to help with synchronization: #89394.

So let's always send it as a list, where encoded would always look something like this?

{ deltas: [ ... ], // One or many, always an array. timestamp: ..., // Whenever we add this. }

{ deltas: [ ... ], // One or many, always an array. timestamp: ..., // Whenever we add this. }

and for platforms without batch edits

{ oldText:, deltaText:, deltaStart:, deltaEnd:, selectionStart:, selectionEnd:, composingStart:, composingEnd:, timestamp: ..., // Whenever we add this. }

I thought the always sending as a list was in relation to sending the deltas as a list to updateEditingValueWithDeltas. Is it regarding the encoded json format?

Yeah sorry I digressed. I was suggesting the json should be a map with a list of deltas.

a map so we can add a fields in the future.

a list of deltas so we don't have to write separately logic for handling batched/non-batched updates .

justinmc · 2021-08-30T18:57:27Z

packages/flutter/lib/src/widgets/editable_text.dart

@@ -2625,6 +2674,7 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
        currentEditingValue: currentTextEditingValue,
      ),
      enableIMEPersonalizedLearning: widget.enableIMEPersonalizedLearning,
+      enableDeltaModel: true, //Proof of concept.


Are we proposing to put enableDeltaModel in production now?

justinmc · 2021-08-30T19:00:16Z

packages/flutter/lib/src/widgets/editable_text.dart

+    for (final TextEditingDelta delta in textEditingDeltas) {
+      value = delta.apply(value);
+    }
+    updateEditingValue(value);


Smart, just call through to updateEditingValue... For a rich text editor author, could they still disable this?

A rich text editor author at this low level I think would just implement updateEditingValueWithDeltas themselves.

justinmc · 2021-08-30T19:02:33Z

packages/flutter/test/widgets/editable_text_test.dart

+      expect(state.currentTextEditingValue.composing, equalityDelta.composing);
+    });
+  });
+


Do we have any integration tests that test text editing? If so, maybe as an easy additional test for this, we could run them a second time with enableDeltaModel set to true, and make sure that text editing still works as normal. Not sure if we have any though.

GaryQian · 2021-08-30T21:18:23Z

packages/flutter/lib/src/services/text_input.dart

+  /// A replacement can occur in cases such as auto-correct, suggestions, and
+  /// when a selection is replaced by a single character.
+  /// {@endtemplate}
+  replacement,


Isn't insertion equivalent to a replacement on a collapsed range? Adding a hardcoded type like this for something that is semantically equivalent to existing API may result in redundancy, though this does make a bit more sense to laypeople

Same goes for deletion, is it not equivalent to a replacement with an emptystring? Do we have a way to disambiguate between an emptystring replacement vs a deletion, collapsed replacement vs insertion?

I'd like to offer a couple thoughts on this. One of them is specific, the other is more generic to the overall approach.

something that is semantically equivalent to existing API

I think this statement is a bit inaccurate. The changes are "technically equivalent", they are not "semantically equivalent". A user who "inserts" content is not a user who is "replacing" content. From a user/domain perspective, these are distinct intentions, typically utilized for different use-cases. There could be any number of app-level reactions that care about the semantic distinction.

Second, I'd like to repeat a point that I've made elsewhere in this project, but it's on direct display in this particular question/recommendation. This proposed delta API, along with this recommendation to combine event types, are both predicated on the idea of a "least common denominator" approach to fundamental text input. It's a reduction of nuanced platform behaviors to what the Flutter team deems worthy of Flutter apps. Consider that the details that are made available at this bottleneck point within the framework are the only details that any app developer can ever gain access to (without maintaining a permanent fork of the engine).

Historically, Flutter has claimed to oppose the "least common denominator" problem that tends to arise in cross-platform toolkits, but this problem began as a result of an artificially limited API, and we seem to be continuing that trend with this delta PR. Flutter suggests that we embrace the yak shave, but this project is still lacking a deep audit of text-based APIs on the 6 major platforms. How do we know what we're leaving out? Flutter believes in aggressive composition, but rather than port each platforms text APIs and then compose them into a unified API, this approach creates a unified API monolith, from which app developers cannot escape (without maintaining a permanent fork of the engine).

I recommend retaining absolutely all text-related information within the fundamental text APIs, both with regard to this specific area, as well as the overall project, in general. This recommendation is based on the fact that Flutter is a UI toolkit, and text interaction is an absolutely fundamental, universal UI interaction. This is the core of what Flutter exists to do. This is not an area to "simplify" platform APIs. App developers need to know what the platform is doing with text.

I agree we should definitely avoid a least common denominator approach, which is definitely not the intention here. The engine PRs have changed a little bit. Let us know if there is any information from the platform that is being thrown away.

GaryQian · 2021-08-30T21:31:15Z

packages/flutter/lib/src/services/text_input.dart

+  ///
+  /// This value will slightly vary based on the [TextEditingDeltaType].
+  ///
+  /// For a [TextEditingDeltaType.insertion] this will be the character/s being


Bulleted list? Might be more clear.

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

packages/flutter/lib/src/services/text_input.dart

+        // an editing state are accumulated before being sent to the framework.
+        // This is called batch editing on Android, and on other platforms the
+        // TextInputPlugin does not adhere to this type of editing.
+        if (encoded['batchDeltas'] != null) {


Yeah I have the same question. Since framework is not consuming the "isBatched" information, would it be OK if we always send a list of deltas?

LongCatIsLooong · 2021-09-03T20:36:43Z

packages/flutter/lib/src/services/text_input.dart

+  /// handles. There are no changes to the text, but there are updates to the selection
+  /// and potentially the composing region as well.
+  /// {@endtemplate}
+  equality,


"cursorUpdate" maybe? "equality" sounds like a very strong word to me.

LongCatIsLooong · 2021-09-03T20:39:46Z

packages/flutter/lib/src/services/text_input.dart

+
+  /// Replaces a range of text in the original string with the text given in the
+  /// replacement string.
+  String replace(String originalText, String replacementText, int start, int end) {


Is there a reason it's made a public method (and in a mixin)? Would it be possible to just leave it as a private global function?

LongCatIsLooong · 2021-09-03T20:42:48Z

packages/flutter/lib/src/services/text_input.dart

+      case 'TextEditingDeltaType.equality':
+        return TextEditingDeltaEquality.fromJSON(encoded);
+      default:
+        return TextEditingDeltaEquality.fromJSON(encoded);


nit: maybe assert(false) here if the default clause should not be reachable.

LongCatIsLooong · 2021-09-03T20:50:13Z

packages/flutter/lib/src/services/text_input.dart

+  /// For a [TextEditingDeltaType.replacement] this will be the range of
+  /// characters that are being replaced.
+  ///
+  /// For a [TextEditingDeltaType.equality] this will be a collapsed range


If a property does not make sense in a subtype then it may make sense to remove it from the common interface. Same for deltaText.

LongCatIsLooong · 2021-09-03T20:57:32Z

packages/flutter/lib/src/services/text_input.dart

+  /// {@macro flutter.services.TextEditingDelta.apply}
+  @override
+  TextEditingValue apply(TextEditingValue value) {
+    String newText = value.text;


Should this be using oldText instead of value.text? It may crash if value.text is, say, empty.

I have added a check to verify if oldText == value.text. If this condition is not true then we return the original TextEditingValue.

LongCatIsLooong · 2021-09-03T20:58:42Z

packages/flutter/lib/src/services/text_input.dart

+  /// {@macro flutter.services.TextEditingDelta.apply}
+  @override
+  TextEditingValue apply(TextEditingValue value) {
+    String newText = value.text;


Same as the previous comment. The base string should probably be oldText? Also how should conflicts be resolved if value.text is not oldText?

justinmc

Some comments to address but I like this with the fromJSON logic here in the framework instead of the engine!

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

packages/flutter/lib/src/services/text_input.dart

+
+  /// {@template flutter.services.TextEditingDeltaReplacement}
+  /// The delta is replacing a range of characters with a new sequence of text.
+  /// The range that is being replaced can either grow or shrink based on the


An empty line should be between these two lines, and below.

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

packages/flutter/lib/src/services/text_input.dart

+  /// The delta is not modifying the text. There are potentially selection and
+  /// composing region updates in the delta that still need to be applied to your
+  /// text model.
+  /// A situation where this delta would be created is when dragging the selection


Empty line above this line too.

justinmc · 2021-09-07T22:36:14Z

packages/flutter/lib/src/services/text_input.dart

+  /// A replacement can occur in cases such as auto-correct, suggestions, and
+  /// when a selection is replaced by a single character.
+  /// {@endtemplate}
+  replacement,


I agree we should definitely avoid a least common denominator approach, which is definitely not the intention here. The engine PRs have changed a little bit. Let us know if there is any information from the platform that is being thrown away.

justinmc · 2021-09-07T22:38:15Z

packages/flutter/lib/src/services/text_input.dart

+
+  /// Replaces a range of text in the original string with the text given in the
+  /// replacement string.
+  String replace(String originalText, String replacementText, int start, int end) {


+1, I can't see why this should be a mixin.

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

packages/flutter/lib/src/services/text_input.dart

+/// This class should not be used directly, and should be extended for different
+/// types of deltas.


Nit: probably no need to mention that it shouldn't be used directly now that it's not possible to use it directly.

justinmc · 2021-09-07T22:42:08Z

packages/flutter/lib/src/services/text_input.dart

+      );
+    }
+
+    assert(false);


So this should never be reached, but if it is reached in prod, then use a TextEditingDeltaNonTextUpdate? Maybe explain this in the message of the assert.

justinmc · 2021-09-07T22:43:12Z

packages/flutter/lib/src/services/text_input.dart

+  });
+
+  /// The old text state before the delta has occured.
+  final String oldText;


Update: Seems like we're keeping it in order to make sure the delta applies to the current text, for now, but we could investigate removing it for space savings later.

justinmc · 2021-09-07T22:46:52Z

packages/flutter/lib/src/services/text_input.dart

+  ///
+  /// See [TextEditingDeltaType] for more information.
+  /// {@endtemplate}
+  TextEditingDeltaType get deltaType;


What should users do with deltaType if they implement their own TextEditingDelta subclass?

This was discussed offline but @LongCatIsLooong suggested converting any new types to one of the basic types we have introduced replacement/deletion/insertion/nonTextUpdate. If a developer does want to implement their own TextEditingDelta subclass I think that would be the appropriate course of action if they want to avoid any framework changes.

Though how a developer would plug this new delta type into the current pipeline has not been addressed. Currently that is all handled by the TextInput._handleTextInputInvocation and all the developer will ever receive from updateEditingValueWithDeltas is the final list of TextEditingDeltas. In theory they could process this list of TextEditingDeltas even further to include their own custom TextEditingDelta types, but there is no way to include their new delta type in the processing before this point.

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

packages/flutter/lib/src/services/text_input.dart

+      deltaText: deltaText,
+      deltaRange: deltaRange,
+      selection: selection,
+      composing:composing,


Space here after the colon (and in other subclasses like this).

justinmc · 2021-09-07T22:51:33Z

packages/flutter/test/widgets/editable_text_test.dart

+      expect(controller.selection, nonTextUpdateDelta.selection);
+      expect(state.currentTextEditingValue.composing, nonTextUpdateDelta.composing);
+    });
+  });


I think you should add some unit tests to services/text_input_test.dart that test TextEditingDelta.fromJSON directly since there is a lot of logic in there. Just make a bunch of calls to it with various kinds of json data and expect it to create the correct TextEditingDelta.

That sounds like a good idea to me. I'll still leave these tests in editable_test_text.dart as they test the updateEditingValueWithDelta function.

… TextInputClient.updateEditingStateWithDelta

…del and delta model, add proof of concept DeltaTextEditingController

justinmc

The only thing I see left is the batchDeltas array thing I commented on.

justinmc · 2021-09-10T23:43:34Z

packages/flutter/lib/src/services/text_input.dart

+        // an editing state are accumulated before being sent to the framework.
+        // This is called batch editing on Android, and on other platforms the
+        // TextInputPlugin does not adhere to this type of editing.
+        if (encoded['batchDeltas'] != null) {


So let's always send it as a list, where encoded would always look something like this?

{ deltas: [ ... ], // One or many, always an array. timestamp: ..., // Whenever we add this. }

justinmc · 2021-09-10T23:44:23Z

packages/flutter/lib/src/widgets/editable_text.dart

+  void updateEditingValueWithDeltas(List<TextEditingDelta> textEditingDeltas) {
+    TextEditingValue value = _value;
+    for (final TextEditingDelta delta in textEditingDeltas) {
+      print('Delta class type: ' + delta.runtimeType.toString());


Prints to remove before this is final.

LongCatIsLooong · 2021-09-10T23:29:24Z

packages/flutter/lib/src/services/text_editing_delta.dart

+  /// The offset in the [oldText] where the insertion begins.
+  final int insertionOffset;
+
+  /// {@macro flutter.services.TextEditingDelta.apply}


nit: the documentation is inherited so you don't have to use the macro here.

LongCatIsLooong · 2021-09-10T23:33:43Z

packages/flutter/lib/src/services/text_input.dart

+  /// Requests that this client update its editing state by applying the delta
+  /// received from the engine.
+  ///
+  /// The [TextEditingDelta] is treated as a change that will be applied to the client's


How is this different from the first paragraph?

Oh emphasis on "change" to explain what "delta" means?

Updated the docs.

LongCatIsLooong · 2021-09-13T17:04:32Z

packages/flutter/lib/src/services/text_editing_delta.dart

+    // nature of the connection between the framework and platform text input plugins.
+    try {
+      if (oldText != value.text) {
+        throw FlutterError('The editing state is out of sync between the framework and the engine.');


I think you can just call FlutterError.reportError here? Also the oldText != value.text should be in an assert probably.

I'm still not completely convinced that this should be an error. Conflicts can happen (especially when the developer has additional input formatters/mutating onChanged callbacks), and the error will probably get spammed in these scenarios. Also it is typically not a programmer error thus not actionable (and a little bit confusing).

I'm ok with just a comment here then. We can revisit this if it ever seems to be a problem for developers.

I agree that this may lead to a lot of log spam. I though this log would be useful, but if the developer cannot act on it there doesn't seem to be a good reason to have it. Maybe just the comment will suffice?

If a developer wanted to act on this asynchronous failure then they would have to extend their desired TextEditingDelta implementation, and capture the error themselves in the apply method and have logic to handle it. For example say they wanted to see if this happens with an insertion. They would extend TextEditingDeltaInsertion and override apply. They would convert any TextEditingDeltaInsertion to their new TextEditingDeltaNewInsertion and apply those to their editing value instead of the original ones.

LongCatIsLooong · 2021-09-13T17:53:11Z

packages/flutter/lib/src/services/text_input.dart

+        // an editing state are accumulated before being sent to the framework.
+        // This is called batch editing on Android, and on other platforms the
+        // TextInputPlugin does not adhere to this type of editing.
+        if (encoded['batchDeltas'] != null) {


Yeah sorry I digressed. I was suggesting the json should be a map with a list of deltas.

a map so we can add a fields in the future.

a list of deltas so we don't have to write separately logic for handling batched/non-batched updates .

GaryQian · 2021-09-14T07:20:26Z

packages/flutter/lib/src/services/text_editing_delta.dart

+  }
+}
+
+/// The delta is replacing a range of characters with a new sequence of text.


The phrasing of these descriptions currently assume previous knowledge of what a "delta" is.

This should be worded more generally, describing what it is and does without large assumptions of previous knowledge of the system

This applies to the other docs for the other classes as well.

Thanks for the feedback! I updated all the docs.

GaryQian · 2021-09-14T07:24:10Z

packages/flutter/lib/src/services/text_input.dart

+  /// framework as [TextEditingDelta]'s or as one [TextEditingValue].
+  ///
+  /// When this is enabled platform text input updates will
+  /// come through [TextInputClient.updateEditingValueWithDeltas].


May want to also mention that using this results in more granular explicit set of changes rather than batched together general updates to the editing value. We want to help developers find/decide if this feature is right for them.

GaryQian · 2021-09-14T07:27:43Z

packages/flutter/lib/src/services/text_editing_delta.dart

+    // A non text update delta occurs when the selection and/or composing region
+    // has been changed by the platform, and there have been no changes to the
+    // text value.
+    final String oldText = encoded['oldText'] as String;


Instead of parsing the type from the properties of the data, can we concretely determine this from the source in the embedding and pass this information up? This parsing seems to work, but I imagine it could be vulnerable to edge cases in the case of wild and crazy deltas. It would be better to be absolutely certain when possible, and fallback on this algo only when the source cannot distinguish for certain.

Part of this algorithm is using the method definitions from the platforms embedding to concretely determine if something an insertion or a deletion.

From android Editable documentation for their replace() method. Editable has insert/append/delete as well but they are all convenience methods that call replace().

* Replaces the specified range (<code>st…en</code>) of text in this * Editable with a copy of the slice <code>start…end</code> from * <code>source</code>. The destination slice may be empty, in which case * the operation is an insertion, or the source slice may be empty, * in which case the operation is a deletion.

Similarly looking at iOS deleteBackwards method https://github.com/flutter/engine/blob/e8c44adc49d56e0d00a74dde5f79ddfe526fbd37/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm#L1468 that also uses an empty source string as a replacement for the deleted range. insertText similarly inserts some text at the current selection https://github.com/flutter/engine/blob/45dc2fee1304805f54d72563f49873e226e4cd95/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm#L1399. Both of these methods are convenience methods for replaceRange WithText.

The iOS and Android PR's before the inference logic was moved to the framework where very similar.
flutter/engine@2148347
flutter/engine@402280e
The only real new case iOS was handling is that when typing with CJK keyboard (simplified chinese pinyin for example) it actually first types the english characters first and then replaces them with the chinese characters. On Android the word is not inserted into the editing state until it has been fully composed. Keeping in mind there similarity I thought it would be better to centralize all the logic in the framework.

The web also behaves similarly but less information is given to us upfront since we delegate all of the text input control to the <input>/<textarea> element. In this case we must use the relevant API's like beforeinput and compositionupdate to capture the parameters we need. Since nothing is given to us upfront, we must normalize the parameters into a common format.

The only ambiguous part comes from dealing with cases of the composing region. Where we check if the original composing region has been changed, if it has then it is a replacement, if it hasn't then it is either an insertion/deletion. That is the main inference going on.

Ok, thanks for the breakdown, that SGTM

GaryQian · 2021-09-14T07:29:19Z

packages/flutter/lib/src/services/text_editing_delta.dart

+///  * [TextEditingDeltaNonTextUpdate], a delta representing an update to the
+///  selection and/or composing region.
+///  * [TextInputConfiguration], to opt-in your [TextInputClient] to receive
+///  [TextEditingDelta]'s you must set [TextInputConfiguration.enableDeltaModel]


In general, can we make sure we link to the [TextInputConfiguration.enableDeltaModel] more frequently? It may be confusing for developers to find the entry-point into the feature since it is not frequently referred to. We can simply note that the feature only works if this flag is enabled in a few more relevant places.

GaryQian · 2021-09-14T07:30:36Z

packages/flutter/lib/src/services/text_editing_delta.dart

+  }
+}
+
+/// The delta is not modifying the text. There are potentially selection and


Keep first paragraph a one-sentence overview. Details can go below.

GaryQian · 2021-09-14T07:31:59Z

packages/flutter/lib/src/services/text_input.dart

+  ///
+  /// The list of [TextEditingDelta]'s are treated as changes that will be applied
+  /// to the client's editing state. A change is any mutation to the raw text
+  /// value, or any updates to the selection and/or composing region.


Mention opt-in flag here, or people may be confused why this is never called

justinmc

LGTM 👍

justinmc · 2021-09-14T21:56:40Z

packages/flutter/lib/src/services/text_input.dart

+        // an editing state are accumulated before being sent to the framework.
+        // This is called batch editing on Android, and on other platforms the
+        // TextInputPlugin does not adhere to this type of editing.
+        for (final dynamic encodedDelta in encoded['batchDeltas']) {


Looks good always using an array of batchDeltas now 👍

Actually not to overly complicate things, but should we just call this deltas now?

I agree will also remove the comment above this regarding batch editing.

GaryQian

LGTM! :)

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Aug 18, 2021

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

This was referenced Aug 18, 2021

TextEditingDelta support for Android flutter/engine#28175

Merged

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

Closed

justinmc reviewed Aug 18, 2021

View reviewed changes

justinmc reviewed Aug 23, 2021

View reviewed changes

justinmc reviewed Aug 27, 2021

View reviewed changes

Renzo-Olivares force-pushed the text_deltas branch from 4f54cc8 to 51853a7 Compare August 29, 2021 03:00

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] Framework can receive TextEditingDeltas from engine~~ Framework can receive TextEditingDeltas from engine 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:36

GaryQian reviewed Aug 30, 2021

View reviewed changes

Renzo-Olivares mentioned this pull request Sep 2, 2021

TextEditingDelta support for iOS flutter/engine#28418

Merged

8 tasks

LongCatIsLooong reviewed Sep 3, 2021

View reviewed changes

Renzo-Olivares force-pushed the text_deltas branch 2 times, most recently from 3b5538f to 57406ea Compare September 7, 2021 04:20

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

justinmc reviewed Sep 7, 2021

View reviewed changes

Renzo-Olivares added 7 commits September 7, 2021 18:59

Add TextDelta to TextEditingValue

6896c6e

docs updates

535c58b

move TextEditingDeltas out of TextEditingValue and create new channel…

b1c537e

… TextInputClient.updateEditingStateWithDelta

updates for deltaManager

15cb4f4

remove deltaManager

f65518e

add flag to TextInputConfiguration for choosing between plain text mo…

1575424

…del and delta model, add proof of concept DeltaTextEditingController

Add support for autofill with delta model

2c09999

Renzo-Olivares added 2 commits September 10, 2021 16:20

Address reviewer comments in relation to deltaType

283ca65

Report an error if the editing state is out of sync

60a718e

justinmc reviewed Sep 10, 2021

View reviewed changes

Remove prints

ec31582

LongCatIsLooong reviewed Sep 13, 2021

View reviewed changes

Renzo-Olivares added 4 commits September 13, 2021 11:09

Every platform now sends a list of deltas

65e8130

Update docs

967c44c

Remove FlutterError from TextEditingDelta

d4b3e9c

Documentation is inherited

1546d7d

Renzo-Olivares requested review from justinmc and LongCatIsLooong September 13, 2021 21:47

GaryQian reviewed Sep 14, 2021

View reviewed changes

Renzo-Olivares added 3 commits September 14, 2021 10:49

update docs

ec45d92

Remove logs

46f30c2

Merge branch 'master' of github.com:flutter/flutter into text_deltas

a594423

Renzo-Olivares requested a review from GaryQian September 14, 2021 20:41

Update documentation

79b883a

justinmc approved these changes Sep 14, 2021

View reviewed changes

Renzo-Olivares added 2 commits September 15, 2021 11:02

Use deltas instead of batchDeltas

4e7b4c8

Update documentation

5b6183f

GaryQian approved these changes Sep 15, 2021

View reviewed changes

LongCatIsLooong approved these changes Sep 15, 2021

View reviewed changes

Renzo-Olivares added the waiting for tree to go green label Sep 16, 2021

fluttergithubbot merged commit bf66cc2 into flutter:master Sep 16, 2021

justinmc mentioned this pull request Sep 23, 2021

TextField with lots of text can hang app #90063

Open

		/// This class should not be used directly, and should be extended for different
		/// types of deltas.

		TextEditingValue apply(TextEditingValue value) {
		return apply(value);

Framework can receive TextEditingDeltas from engine #88477

Framework can receive TextEditingDeltas from engine #88477

Conversation

Renzo-Olivares commented Aug 18, 2021 • edited

Description

Related Issues

Tests

Pre-launch Checklist

justinmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Renzo-Olivares Aug 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Renzo-Olivares Sep 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc left a comment

Choose a reason for hiding this comment

Renzo-Olivares commented Aug 18, 2021 •

edited

Renzo-Olivares Aug 23, 2021 •

edited

Renzo-Olivares Sep 10, 2021 •

edited