fix voice control delete line command does not delete line correctly by chunhtai · Pull Request #24831 · flutter-team-archive/engine

chunhtai · 2021-03-05T23:22:03Z

The UITextInputStringTokenizer does not parse the line correctly, This pr fixes it.

fixes flutter/flutter#77274

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.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

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

gaaclarke

Mostly looks good, just 2 comments.

gaaclarke · 2021-03-09T01:18:21Z

+#pragma mark - FlutterTokenizer
+
+@implementation FlutterTokenizer {
+  FlutterTextInputView* _textInputView;


Please use a property so we can know if you intend this to be a strong or a weak reference. Right now this is being treated as a weak reference. I suspect you want a strong reference.

This should be a weak reference, FlutterTextInputView owns the FlutterTokenizer. I will update

gaaclarke · 2021-03-09T21:30:34Z

+
+@interface FlutterTokenizer ()
+
+@property(nonatomic, assign) FlutterTextInputView* textInputView;


s/assign/weak for more modern objc and forward compatibility with ARC

I tried to use weak, but it complains this file is MRC and can't use weak

gaaclarke · 2021-03-09T21:35:49Z

+  return self;
+}
+
+- (UITextRange*)rangeEnclosingPosition:(UITextPosition*)position


If you just pass in the FlutterTextInputView as an argument to this method you wouldn't have to keep an instance variable around. The instance variable is a bit problematic because if the class is used differently it can cause a crash.

this is an override. https://developer.apple.com/documentation/uikit/uitextinputtokenizer/1614464-rangeenclosingposition?language=objc

I can't change the function signature

I figured it is quite hard to know whether this is an override or private method for a reviewer. is there a best practice way to annotate an override method?

You almost never use inheritance in objc. I didn't see this was inheriting UITextInputStringTokenizer. Is there a reason it has to inherit from UITextInputStringTokenizer?

The documentation says this for subclassing UITextInputString:

When you subclass UITextInputStringTokenizer, override all UITextInputTokenizer methods, calling the superclass implementation (super) when method parameters are not affected by layout. For example, the subclass needs a custom implementation of all methods for line granularity. For the left direction, it needs to decide whether left corresponds at a given position to forward or backward, and then call super passing in the storage direction (UITextStorageDirection).

Sounds like it might be a problem if you aren't calling super.

edit: ~~You should probably be implementing UITextInputTokenizer then delegating to an instance of UITextInputStringTokenizer when you need to.~~

calling the superclass implementation (super) when method parameters are not affected by layout.

it does not seems to be a hard requirement? a little background on this change.

If you have text = "how are you"
and you call

[UITextInputStringTokenizer rangeEnclosingPosition:<at the end> withGranularity:line inDirection:forward]

it returns NSRange(location =8, length =3) which corresponds to "you". It is same result of Granularity word

The result is completely unusable. That is why i have to complete rewrite the line logic.

Yea, maybe you are right. I have a hard time understanding if that warning in the documentation is actually applicable. We do have tests I guess.

gaaclarke · 2021-03-09T21:36:39Z

+           @"The FlutterTokenizer can only be used in a FlutterTextInputView");
+  self = [super initWithTextInput:textInput];
+  if (self) {
+    _textInputView = (FlutterTextInputView*)textInput;


Why cast here? Why not just pass in a FlutterTextInputView? If you want to keep the cast you should have an assert that it a valid cast (isKindOf:).

yes i have assert it in the beginning, line 392.

This is an override of https://developer.apple.com/documentation/uikit/uitextinputstringtokenizer/1614469-initwithtextinput?language=objc

I thought i saw it somewhere, thanks.

gaaclarke · 2021-03-09T22:39:48Z


+- (FlutterTextRange*)getLineRangeFromTokenizer:(id<UITextInputTokenizer>)tokenizer
+                                       atIndex:(NSInteger)index {
+  return (FlutterTextRange*)[tokenizer


Probably should add an assert here too if you are going to cast.

…rrectly (flutter-team-archive/engine#24831)

…lutter-team-archive#24831)

flutter-dashboard Bot added the platform-ios label Mar 5, 2021

googlebot added the cla: yes label Mar 5, 2021

chunhtai requested a review from gaaclarke March 5, 2021 23:23

gaaclarke reviewed Mar 9, 2021

View reviewed changes

chunhtai added 3 commits March 9, 2021 12:06

fix voice control delete line command does not delete line correctly

53a0527

fix typo

6cbb4d3

addressing comment

2feb124

chunhtai force-pushed the issues/77274 branch from 506a694 to 2feb124 Compare March 9, 2021 20:58

gaaclarke reviewed Mar 9, 2021

View reviewed changes

gaaclarke approved these changes Mar 9, 2021

View reviewed changes

addressing comment

2f8b5fe

chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 10, 2021

fluttergithubbot merged commit a560c26 into flutter-team-archive:master Mar 10, 2021

engine-flutter-autoroll mentioned this pull request Mar 10, 2021

Roll Engine from afc72ee1d79c to 4fd6bb1cdf9c (3 revisions) flutter/flutter#77857

Merged

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

a560c26 fix voice control delete line command does not delete line co…

a6b1691

…rrectly (flutter-team-archive/engine#24831)

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

fix voice control delete line command does not delete line correctly (f…

3f5bd3c

…lutter-team-archive#24831)

chriscraws pushed a commit to chriscraws/engine that referenced this pull request Mar 23, 2021

fix voice control delete line command does not delete line correctly (f…

d9cb2f1

…lutter-team-archive#24831)


		@interface FlutterTokenizer ()

		@property(nonatomic, assign) FlutterTextInputView* textInputView;

Uh oh!

Conversation

chunhtai commented Mar 5, 2021

Pre-launch Checklist

Uh oh!

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

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!

chunhtai Mar 9, 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!

gaaclarke Mar 9, 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!

chunhtai Mar 9, 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!

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

chunhtai Mar 9, 2021 •

edited

Loading

gaaclarke Mar 9, 2021 •

edited

Loading

chunhtai Mar 9, 2021 •

edited

Loading