TextEditingDelta Mac by justinmc · Pull Request #29036 · flutter/engine

justinmc · 2021-10-05T23:58:46Z

This PR will bring TextEditingDeltas, originally introduced by @Renzo-Olivares, to Mac. This is very similar to the iOS implementation (#28418).

Partial fix for flutter/flutter#87972

justinmc · 2021-10-12T01:06:36Z

shell/platform/darwin/macos/framework/Source/FlutterTextEditingDelta.h

@@ -0,0 +1,24 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


FlutterTextEditingDelta here is nearly identical to its iOS counterpart (file), but here we use Cocoa instead of UIKit, and Mac seems to be using ARC. Is there any worthwhile way to share the code between the two despite that?

Why the cocoa import then? Is it possible to replace that (and the UIKit import in the iOS implementation) with Foundation?

Maybe we can implement something similar to TextInputModel in the shell/platform/common directory, that can be used across all platforms?

The Cocoa import should have been AppKit. It looks like I can replace both with Foundation though 👍 . I'll attempt to create something reusable in shell/platform/common.

The reusable class works, I've modified this PR to use it for both Mac and iOS. I'll use it in subsequent PRs on other platforms as well.

GaryQian · 2021-10-13T21:43:01Z

shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm

 @property(nonatomic, nonnull) NSString* inputAction;

+/**
+ * Whether to enable that the engine sends text input updates to the framework


nit: this wording is a bit awkward.

Maybe something like: Whether to enable the engine sending text input updates to the framework as TextEditingDeltas rather than one TextEditingValue.

Good call, I modified that slightly:

* Whether to enable the sending of text input updates from the engine to the * framework as TextEditingDeltas rather than as one TextEditingValue.

LongCatIsLooong · 2021-10-18T20:38:42Z

shell/platform/darwin/macos/framework/Source/FlutterTextEditingDelta.h

@@ -0,0 +1,24 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


Why the cocoa import then? Is it possible to replace that (and the UIKit import in the iOS implementation) with Foundation?

LongCatIsLooong · 2021-10-18T20:46:11Z

shell/platform/darwin/macos/framework/Source/FlutterTextEditingDelta.h

@@ -0,0 +1,24 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


Maybe we can implement something similar to TextInputModel in the shell/platform/common directory, that can be used across all platforms?

LongCatIsLooong · 2021-10-18T22:37:10Z

shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm

    // engine since it sent this update, and needs to now be made to match the
    // engine's version of the state.
-    [self updateEditState];
+    if (!_enableDeltaModel) {


Is this not needed when the delta model is enabled?

Hmm this is Mac's weird "Ack". In the race condition case that the comment mentions, you're probably right that it needs to update the framework. I'll update it to send an "empty" delta in that case. The client can compare the oldText with their current text to detect the race condition.

LongCatIsLooong · 2021-10-18T22:44:35Z

shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm

  if (_activeModel == nullptr) {
    return;
  }
+  NSString* textBeforeChange = [NSString stringWithUTF8String:_activeModel->GetText().c_str()];


@cbracken is there a reason the UTF16 presentation of the string isn't exposed by TextInputModel?

Internally it stores everything in UTF-16, so there's no technical reason we couldn't. If I remember correctly, TextInputModel tries to expose only a UTF-8 representation but will gladly convert from UTF-16.

I suspect that I did that just to minimize API surface (and minimise potential for confusion/bugs) and because we didn't need it for Linux which is the first platform I wrote it for.

…dates

justinmc · 2021-10-26T17:44:36Z

This PR suddenly had an error show up in it that didn't exist before when running the example app. Maybe due to a change in the framework? I'll get it running again and then address the comments above.

justinmc · 2021-11-16T23:05:24Z

shell/platform/common/text_editing_delta.h

+
+#ifndef FLUTTER_SHELL_PLATFORM_COMMON_TEXT_EDITING_DELTA_H_
+#define FLUTTER_SHELL_PLATFORM_COMMON_TEXT_EDITING_DELTA_H_
+#define _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING


The use of codecvt_utf8_utf16 caused a deprecation warning on Windows that wanted me to use wstring, but I've silenced it since this code needs to work on multiple platforms.

Yep; we enable this in a few places. That said, instead of defining it here, please define this in shell/platform/common/BUILD.gn as we do here; that'll make it easier to rip this out later.

I've filed: flutter/flutter#93743

Isn't that definition you linked exactly where I'd need to define this for this file? text_editing_delta.h and cc are included in that block in this PR. Or am I misunderstanding? I still seemed to get a failure in the Windows tests caused by this.

Ah yeah -- looks like that is indeed the exact target. Will try patching this in to see if I can repro.

justinmc · 2021-11-16T23:05:33Z

Ready for re-review. I've updated the TextEditingDelta struct to store the strings as utf-16 while providing setters and constructors for both utf 8 and 16.

skia-gold · 2021-11-16T23:20:21Z

Gold has detected about 1 new digest(s) on patchset 28.
View them at https://flutter-engine-gold.skia.org/cl/github/29036

cbracken

I've just reviewed the C++ bits -- haven't looked yet looked at the macOS specific Obj-C side.

cbracken · 2021-11-17T00:01:05Z

shell/platform/common/text_editing_delta.h

+
+namespace flutter {
+
+struct TextEditingDelta {


Add doc comments to this struct and the methods.

Let me know if I've missed any that usually should have doc comments. I skipped some things that seemed obvious.

cbracken · 2021-11-17T00:06:23Z

shell/platform/common/text_editing_delta.h

+  std::string oldText() const { return utf16ToUtf8(oldText_); }
+  std::string deltaText() const { return utf16ToUtf8(deltaText_); }
+  int deltaStart() const { return deltaStart_; }
+  int deltaEnd() const { return deltaEnd_; }


Here and below, methods and functions should be named in UpperCamelCase().

Simple property getters/setters can be named foo_bar() and set_foo_bar().

Ref: https://google.github.io/styleguide/cppguide.html#Function_Names

cbracken · 2021-11-17T00:10:58Z

shell/platform/common/text_editing_delta.h

+ public:
+  TextEditingDelta(std::u16string textBeforeChange,
+                   TextRange range,
+                   std::u16string text);


It looks like TextEditingDeltas are copyable types. In this case, you should define:

A copy constructor: TextEditingDelta(const TextEditingDelta& other)

operator=: TextEditingDelta& operator=(const TextEditingDelta& other)

In this case, you should be able to declare them as default.

Ref: https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types

cbracken · 2021-11-17T00:11:29Z

shell/platform/common/text_editing_delta.h

+namespace flutter {
+
+struct TextEditingDelta {
+ public:


This line is not needed since struct members are public by default.

cbracken · 2021-11-17T00:13:25Z

shell/platform/common/text_editing_delta.h

+
+struct TextEditingDelta {
+ public:
+  TextEditingDelta(std::u16string textBeforeChange,


Here and below, parameters, fields, and variables should be formatted as text_before_change.

Ref: https://google.github.io/styleguide/cppguide.html#Variable_Names

cbracken · 2021-11-17T00:34:41Z

shell/platform/common/text_editing_delta.h

+  int deltaEnd_;
+
+  void setDeltas(std::u16string oldText,
+                 std::u16string newTxt,


If we do keep this method around, s/txt/text/ on this, plus switch to words_separated_by_underscores form for all of these.

cbracken · 2021-11-17T00:35:23Z

shell/platform/common/text_editing_delta.h

+    deltaEnd_ = newEnd;
+  }
+
+  std::string utf16ToUtf8(std::u16string string) const {


Similarly here, use const std::u16string& text.

cbracken · 2021-11-17T00:36:23Z

shell/platform/common/text_editing_delta.h

+    std::wstring_convert<std::codecvt_utf8_utf16<char16_t>, char16_t>
+        utf16_converter;
+    return utf16_converter.from_bytes(string);
+  }


Since these two functions don't reference any instance variables they should just be static functions in the C++; that said they're basically just two-liners, and I bet deleting these functions and just using wstring_convert inline in the two places it's used would actually be less lines of code 😉.

e.g. of where else we do this:

engine/shell/platform/common/text_input_model.cc

Lines 36 to 42 in bba38c0

void TextInputModel::SetText(const std::string& text) {

std::wstring_convert<std::codecvt_utf8_utf16<char16_t>, char16_t>

utf16_converter;

text_ = utf16_converter.from_bytes(text);

selection_ = TextRange(0);

composing_range_ = TextRange(0);

}

I ended up keeping these methods around because I needed to use them in the initializers, where I think it would be harder to directly use wstring_convert. Let me know there is a slicker 1 liner way to use them that would be better.

Nope -- no one-liner trick here unfortunately. SGTM!

cbracken · 2021-11-17T00:43:56Z

shell/platform/common/text_editing_delta_unittests.cc

+  ASSERT_EQ(delta.oldText(), oldText);
+  ASSERT_EQ(delta.deltaText(), "");
+  ASSERT_EQ(delta.deltaStart(), -1);
+  ASSERT_EQ(delta.deltaEnd(), -1);


Here and above: These could probably be EXPECT_EQ since these checks aren't interdependent on each other.

Ref: https://testing.googleblog.com/2008/07/tott-expect-vs-assert.html

cbracken · 2021-11-17T00:45:49Z

shell/platform/common/text_editing_delta_unittests.cc

+  // Here we are simulating inserting an "o" at the end of "hell".
+  std::string oldText = "hell";
+  std::string replacementText = "hello";
+  flutter::TextRange range = flutter::TextRange(0, 4);


flutter:: prefix isn't necessary here since you're already in the flutter namespace.

Just:

TextRange range(0, 4);

justinmc · 2021-11-17T19:40:35Z

@cbracken Ready for re-review. Thanks for all of the suggestions and references as I figure out C++ at Google.

I think I took all of your suggestions except for two that I left questions on above.

skia-gold · 2021-11-17T19:50:21Z

Gold has detected about 253 new digest(s) on patchset 29.
View them at https://flutter-engine-gold.skia.org/cl/github/29036

cbracken · 2021-11-17T20:43:29Z

shell/platform/common/text_editing_delta_unittests.cc

+TEST(TextEditingDeltaTest, TestTextEditingDeltaConstructor) {
+  // Here we are simulating inserting an "o" at the end of "hell".
+  std::string old_text = "hell";
+  std::string replacementText = "hello";


nit: replacement_text

cbracken · 2021-11-17T20:44:32Z

shell/platform/common/text_editing_delta.cc

+
+TextEditingDelta::~TextEditingDelta() = default;
+
+TextEditingDelta::TextEditingDelta(std::u16string text_before_change,


const std::u16string& text_before_change and a couple more places below.

cbracken · 2021-11-17T20:46:28Z

shell/platform/common/text_editing_delta.h

+
+  TextEditingDelta(const std::string& text);
+
+  virtual ~TextEditingDelta();


You could mark this default and kill off the one in the cc file.

cbracken · 2021-11-17T20:47:06Z

shell/platform/common/text_editing_delta_unittests.cc

+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//


Nit: this should be a blank line (kill the //)

cbracken

LGTM on the C++ side modulo a couple minor nits. Will take a look at the macOS implementation after lunch!

justinmc · 2021-11-17T21:18:17Z

Not sure why gold picked up 253 new gold images... I've merged in master and hopefully that fixes it.

cbracken · 2021-11-17T21:39:16Z

shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm

+                                                            : kTextAffinityDownstream;
+}
+
+- (NSRange)clampSelection:(NSRange*)range forText:(NSString*)text {


What about clampRange:(NSRange*)range forText:(NSString*)text in case we ever use this on e.g. mark text?

Actually it looks like I've removed all usage of this 🤷. I'll remove it altogether then.

cbracken

lgtm!

justinmc · 2021-11-18T00:59:15Z

@cbracken I'm not authorized to merge this PR apparently, can you do it?

cbracken · 2021-11-18T01:15:15Z

Ah -- the issue is that you're trying to merge against master. We switched the default branch to main last night.

justinmc · 2021-11-18T16:43:56Z

Ah there we go, thanks!

justinmc added the affects: text input label Oct 5, 2021

justinmc self-assigned this Oct 5, 2021

flutter-dashboard bot added the platform-macos label Oct 5, 2021

google-cla bot added the cla: yes label Oct 5, 2021

justinmc mentioned this pull request Oct 6, 2021

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

Closed

justinmc force-pushed the deltas-mac branch 2 times, most recently from 3d917e6 to 98c0155 Compare October 7, 2021 22:54

justinmc commented Oct 12, 2021

View reviewed changes

justinmc force-pushed the deltas-mac branch from 55c6f2b to 1bee70b Compare October 12, 2021 01:08

justinmc marked this pull request as ready for review October 12, 2021 01:50

justinmc requested review from GaryQian and LongCatIsLooong October 12, 2021 01:50

justinmc changed the title ~~(WIP) TextEditingDelta Mac~~ TextEditingDelta Mac Oct 12, 2021

GaryQian reviewed Oct 13, 2021

View reviewed changes

LongCatIsLooong reviewed Oct 18, 2021

View reviewed changes

cbracken mentioned this pull request Oct 18, 2021

TextField with lots of text can hang app flutter/flutter#90063

Open

justinmc added 13 commits October 25, 2021 13:52

POC working by porting general ideas from iOS deltas PR

260aeb7

Test for TextEditingDelta, and licenses fix

9e68135

WIP first TextEditingPlugin test for deltas

5db0cbb

WIP Working test that framework=>engine updates dont trigger delta up…

f485baf

…dates

WIP working test for insertText deltas

7a683e7

WIP Working test for setMarkedText

9505852

WIP UnmarkText test and combine tests with duplicate logic

f8a704b

Tests working and cleaned up

2996d3d

More accurate naming given single argument

e3798a8

Clean up duplicate affinity calculation

9953c5d

Formatting

4828a7b

Clean up TODO and docs

343d1f4

Code review docs rewording

220d870

justinmc commented Nov 16, 2021

View reviewed changes

cbracken suggested changes Nov 17, 2021

View reviewed changes

C++ code review fixes

6c6fa83

cbracken reviewed Nov 17, 2021

View reviewed changes

fluttergithubbot force-pushed the master branch from aa7ab8d to 29ea28d Compare November 17, 2021 20:49

cbracken reviewed Nov 17, 2021

View reviewed changes

justinmc added 3 commits November 17, 2021 13:14

Code review round 2 for C++

11a928e

Merge branch 'master' into deltas-mac

c62faad

Formating

84dd649

cbracken reviewed Nov 17, 2021

View reviewed changes

cbracken approved these changes Nov 17, 2021

View reviewed changes

Remove unused methtod

d154f9c

cbracken added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. and removed waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. labels Nov 18, 2021

justinmc changed the base branch from master to main November 18, 2021 16:43

justinmc merged commit 29c5631 into flutter:main Nov 18, 2021

justinmc deleted the deltas-mac branch November 18, 2021 16:44

engine-flutter-autoroll mentioned this pull request Nov 19, 2021

Roll Engine from d3dcd41c9490 to 64b13b8eff25 (29 revisions) flutter/flutter#93941

Merged

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

29c5631 TextEditingDelta Mac (flutter/engine#29036)

08f935c

		@@ -0,0 +1,24 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

	void TextInputModel::SetText(const std::string& text) {
	std::wstring_convert<std::codecvt_utf8_utf16<char16_t>, char16_t>
	utf16_converter;
	text_ = utf16_converter.from_bytes(text);
	selection_ = TextRange(0);
	composing_range_ = TextRange(0);
	}


		TextEditingDelta::~TextEditingDelta() = default;

		TextEditingDelta::TextEditingDelta(std::u16string text_before_change,


		TextEditingDelta(const std::string& text);

		virtual ~TextEditingDelta();

Conversation

justinmc commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cbracken Oct 18, 2021 • 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!

justinmc commented Oct 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Nov 16, 2021 • 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 commented Nov 16, 2021

Uh oh!

skia-gold commented Nov 16, 2021

Uh oh!

cbracken 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!

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 commented Nov 17, 2021

Uh oh!

skia-gold commented Nov 17, 2021

justinmc commented Oct 5, 2021 •

edited

Loading

cbracken Oct 18, 2021 •

edited

Loading

cbracken Nov 16, 2021 •

edited

Loading

cbracken Nov 17, 2021 •

edited

Loading

cbracken Nov 17, 2021 •

edited

Loading