Reland "iOS spell-checker ObjC #32941" #34356

cyanglaz · 2022-06-28T19:19:58Z

Things added to the original PR:

Make UITextChecker lazily intialized.
Convert the method channel result format to be the same as Android so the framework code could be shared.
Move method channel out of the SpellCheckerPlugin so the pattern matches other plugins in FlutterEngine.

Part of flutter/flutter#34688

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

cyanglaz · 2022-06-28T19:23:18Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

+    FlutterSpellCheckPlugin* spellCheckPlugin = _spellCheckPlugin.get();
+    [_spellCheckChannel.get()
+        setMethodCallHandler:^(FlutterMethodCall* call, FlutterResult result) {
+          [spellCheckPlugin handleMethodCall:call result:result];
+        }];


This is the 3rd difference between the original PR, I moved the method channel setup out of FlutterSpellCheckPlugin

cyanglaz · 2022-06-28T19:26:03Z

shell/platform/darwin/ios/framework/Source/FlutterSpellCheckPlugin.mm

+- (NSString*)toFormattedString {
+  return FormatSpellCheckResult(_misspelledRange.location,
+                                _misspelledRange.location + _misspelledRange.length - 1,
+                                [_suggestions copy]);
+}


This is the 2nd different between the original PR. Instead of formatting the result as a dictionary in my original PR: https://github.com/flutter/engine/pull/32941/files#diff-1c2372162cba7041e14181d7d080cbb0c66eeb44e9cd75379ab8969646e3087bR154-R159

I formatted as a String that matches the android format: https://github.com/flutter/engine/blob/main/shell/platform/android/io/flutter/plugin/editing/SpellCheckPlugin.java#L108-L110
Note that there is a typo in the android documentation, it is suppose to be \n not /n

cyanglaz · 2022-06-28T19:26:47Z

shell/platform/darwin/ios/framework/Source/FlutterSpellCheckPlugin.mm

+    // UITextChecker is an expensive object to initiate, see:
+    // https://github.com/flutter/flutter/issues/104454. Lazily initialate the UITextChecker object
+    // until at first method channel call. We avoid using lazy getter for testing.
+    _textChecker = [[UITextChecker alloc] init];


1st difference between the original PR, the UITextChecker object is initialized during the first method channel call.

The laziness is typically implemented inside a property getter:

- textChecker { if (!_textChecker) _textChecker = ... return _textChecker }

Yes, but it would be hard to test. For example, if I want to test the textChecker is nil at initialization, i couldn't because as soon as i call the getter, it is initialized.
My comment explained why we avoided a lazy getter.

ah sorry i miss-read that part.

cyanglaz · 2022-06-28T19:27:41Z

Adding @hellohuanlin as the reviewer as the reviewer of the original PR @jmagman is OOO.

skia-gold · 2022-06-28T19:39:36Z

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

hellohuanlin

main question is about whether to reinvent an in-house serialization format

hellohuanlin · 2022-06-28T23:21:38Z

shell/platform/darwin/ios/framework/Source/FlutterSpellCheckPlugin.mm

+
+// Spell check results will be encoded as a string representing the span of that result, with
+// the format "start_index.end_index.suggestion_1\nsuggestion_2\nsuggestion_3".
+static NSString* FormatSpellCheckResult(NSUInteger startIndex,


Is this the same format as the android?

Why are we using a custom serialization? can we just use more standardized ones like JSON or CSV?

Yeah it is matching android, im not sure the detailed reason behind not using JSON.
cc @camsim99

The reason behind this was to avoid unnecessary overhead as brought up in my design doc. I think I discussed with @cyanglaz doing some testing to see if this would significantly slow things down and I would be open to that since that's the main concern!

@camsim99 Are you worrying that JSON serialization/parsing is significantly slower (or taking more space) than the proposed custom format?

I think JSON is a better approach, it shouldn't be slower.
For example in java: https://stackoverflow.com/questions/12155800/how-to-convert-hashmap-to-json-object-in-java

I thought there was a discussion about this format when @camsim99 did the Android engine PR, but I couldn't find it (code in the PR). I think @GaryQian was involved if I remember right?

From my memory the concern with JSON was the repetition of keys for every suggestion, which for a long document there could be many.

I think this is what I was thinking of from the design doc: https://docs.google.com/document/d/1AVI2dwpBYtvf7TnDyfLYuE8VLwoMj3iaRLUs3qMjxWY/edit?disco=AAAATKs1zrg

If memory is a concern, it seems to me that we should limit the misspelled word that the API returns.

If we write our own formatter and parser, we also have to consider the performance of them, which creates an additional maintenance burden.

Also, I was a little confused myself last week. JSON serialization is already supported by default on MethodChannel, so we don't really have to do an extra serialization in this PR if we choose to use JSON. I will revert that code.

I talked to Gary a little bit and we agreed that using JSON seems to be the way to go for now. If it turns out to be slow, we can revisit using a custom format.

Great! Sounds like a solid plan!

hellohuanlin · 2022-06-28T23:22:58Z

shell/platform/darwin/ios/framework/Source/FlutterSpellCheckPlugin.mm

+
+// Get all the misspelled words and suggestions in the entire String.
+//
+// The result will be formatted as am NSArray.


hellohuanlin · 2022-06-28T23:27:16Z

shell/platform/darwin/ios/framework/Source/FlutterSpellCheckPlugin.mm

+    // UITextChecker is an expensive object to initiate, see:
+    // https://github.com/flutter/flutter/issues/104454. Lazily initialate the UITextChecker object
+    // until at first method channel call. We avoid using lazy getter for testing.
+    _textChecker = [[UITextChecker alloc] init];


The laziness is typically implemented inside a property getter:

- textChecker { if (!_textChecker) _textChecker = ... return _textChecker }

skia-gold · 2022-06-29T20:39:36Z

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

flutter-dashboard · 2022-06-29T21:03:44Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #34356 at sha 0b5e6ce

cyanglaz · 2022-06-29T21:42:57Z

shell/platform/darwin/ios/framework/Source/SwiftFile.swift

@@ -0,0 +1 @@
+import Foundation


accedentialy checked this in, will remove

cyanglaz · 2022-07-06T16:06:31Z

@hellohuanlin Updated the format to JSON, ptal

cc @camsim99

skia-gold · 2022-07-06T16:27:32Z

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

Revert "Revert "iOS spell-checker ObjC" (flutter#33570)" This reverts commit b1fafb9. method channel move out of plugin tests cleanup fix fix typo remove swift file change format to JSON nil error format revert format code

cyanglaz · 2022-07-11T17:25:21Z

Friendly ping @hellohuanlin
We can delay the discussion of the result format. I can always come back to add a new PR to update the format if we end up using the "startIndex.endIndex.suggestions" format.

I'm not sure about the skia-gold check. There is no bot message saying the gold is fixed but the gold error comment didn't show up after re-basing so I assume it was an upstream issue and re-base fixed it?

cc @godofredoc Do you think we can improve the skia-gold bot so that it is clearer that the skia-gold issue is fixed after a commit?

flutter-dashboard bot added the platform-ios label Jun 28, 2022

cyanglaz commented Jun 28, 2022

View reviewed changes

cyanglaz requested a review from hellohuanlin June 28, 2022 19:27

cyanglaz marked this pull request as ready for review June 28, 2022 19:27

hellohuanlin reviewed Jun 28, 2022

View reviewed changes

flutter-dashboard bot added the will affect goldens label Jun 29, 2022

cyanglaz force-pushed the reland_ios_check_check branch from 0b5e6ce to c1c1f2c Compare June 29, 2022 21:09

cyanglaz commented Jun 29, 2022

View reviewed changes

cyanglaz force-pushed the reland_ios_check_check branch from c2d5961 to f2511d8 Compare July 6, 2022 16:45

draft

8847696

Revert "Revert "iOS spell-checker ObjC" (flutter#33570)" This reverts commit b1fafb9. method channel move out of plugin tests cleanup fix fix typo remove swift file change format to JSON nil error format revert format code

cyanglaz force-pushed the reland_ios_check_check branch from 65d6824 to 8847696 Compare July 7, 2022 17:18

hellohuanlin approved these changes Jul 11, 2022

View reviewed changes

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 11, 2022

auto-submit bot merged commit 45d0ae7 into flutter:main Jul 11, 2022

engine-flutter-autoroll mentioned this pull request Jul 12, 2022

Roll Flutter Engine from abe7b2ccc30d to 45d0ae7e7875 (1 revision) flutter/flutter#107436

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2022

45d0ae7 Reland "iOS spell-checker ObjC #32941" (flutter/engine#34356)

77b4b76

engine-flutter-autoroll mentioned this pull request Jul 12, 2022

Roll Flutter Engine from abe7b2ccc30d to 7fa9e5ad45b7 (2 revisions) flutter/flutter#107439

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2022

45d0ae7 Reland "iOS spell-checker ObjC #32941" (flutter/engine#34356)

aba5507

camsim99 mentioned this pull request Jul 13, 2022

Undo custom serialization of spell check results on Android #34647

Merged

8 tasks

Reland "iOS spell-checker ObjC #32941" #34356

Reland "iOS spell-checker ObjC #32941" #34356

Uh oh!

Conversation

cyanglaz commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

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!

cyanglaz commented Jun 28, 2022

Uh oh!

skia-gold commented Jun 28, 2022

Uh oh!

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

skia-gold commented Jun 29, 2022

Uh oh!

flutter-dashboard bot commented Jun 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Jul 6, 2022

Uh oh!

skia-gold commented Jul 6, 2022

Uh oh!

cyanglaz commented Jul 11, 2022

Uh oh!

Uh oh!

cyanglaz commented Jun 28, 2022 •

edited

Loading