[TextInput] minor fixes by LongCatIsLooong · Pull Request #87973 · flutter/flutter

LongCatIsLooong · 2021-08-10T01:44:53Z

Make the invalid TextSelection a singleton
Equality check does not account for text affinity unless it's
collapsed and valid (which makes more sense according to its documentation).
Fix EditableText.scrollController's lifecycle.
Fix EditableTextState animation controller never disposed #83885. Dispose animation controllers

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

List which issues are fixed by this PR. You must list at least one issue.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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.

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

- Make the invalid `TextSelection` a singleton - Equality check does not account for text affinity unless it's collapsed and valid (which makes more sense according to its documentation). - Fix `EditableText.scrollController`'s lifecycle. - Dispose animation controllers

LongCatIsLooong · 2021-08-10T01:51:55Z

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

+    return other.baseOffset == baseOffset
        && other.extentOffset == extentOffset
-        && other.affinity == affinity
+        && (!isCollapsed || other.affinity == affinity)


https://master-api.flutter.dev/flutter/services/TextSelection/affinity.html:

If the text range is collapsed and has more than one visual location (e.g., occurs at a line break), which of the two locations to use when painting the caret.

So it appears we don't have separate text affinities for each endpoint (I wonder why?) and the affinity is only meaningful when the selection is collapsed.

I just noticed this same thing! I'm worried it may be an oversight. I'm pretty sure we have code that does check the affinity when it's not collapsed, too. I'm going to create an issue for this.

…or-fixes

justinmc

LGTM 👍

Lots of good improvements here, thanks for taking the time to do this! I think the only thing that needs to change is the Scorll typo.

justinmc · 2021-08-13T01:20:37Z

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

+    return other.baseOffset == baseOffset
        && other.extentOffset == extentOffset
-        && other.affinity == affinity
+        && (!isCollapsed || other.affinity == affinity)


I just noticed this same thing! I'm worried it may be an oversight. I'm pretty sure we have code that does check the affinity when it's not collapsed, too. I'm going to create an issue for this.

justinmc · 2021-08-13T01:45:03Z

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

+    return other.baseOffset == baseOffset
        && other.extentOffset == extentOffset
-        && other.affinity == affinity
+        && (!isCollapsed || other.affinity == affinity)


justinmc · 2021-08-13T01:47:07Z

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


  @override
  String toString() {
-    return '${objectRuntimeType(this, 'TextSelection')}(baseOffset: $baseOffset, extentOffset: $extentOffset, affinity: $affinity, isDirectional: $isDirectional)';


👍 Yours is easier to read in my opinion.

justinmc · 2021-08-13T01:53:45Z

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

+    if (other is! TextSelection)
+      return false;
+    if (!isValid) {
+      return !other.isValid;


I like this, I think it helps alleviate the confusion of -1,-1 selection.

justinmc · 2021-08-13T04:49:25Z

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

  TextInputConnection? _textInputConnection;
  TextSelectionOverlay? _selectionOverlay;

+  ScrollController? _internalScorllController;


Typo: Scorll => Scroll

justinmc · 2021-08-13T04:50:09Z

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

  static const Duration _floatingCursorResetTime = Duration(milliseconds: 125);

-  late AnimationController _floatingCursorResetController;
+  late final AnimationController _floatingCursorResetController = AnimationController(


Is this late unnecessary now since it's immediately assigned?

Yeah since the animation controller needs to reference this it has to be lazy.

justinmc · 2021-08-13T04:59:15Z

packages/flutter/test/widgets/editable_text_test.dart

+    expect(scrollController2.attached, isFalse);
+
+    // Change scrollController to controller 2.
+    await tester.pumpWidget(


Making sure I understand what's happening in this test. By pumping a new app, the old EditableText has its dispose called and the new one gets instantiated. didUpdateWidget is not called. Is that right?

No it's just replacing scrollController1 with scrollController2 so didUpdateWidget is called. Currently we're not properly removing/adding listeners when the scroll controller changes

…or-fixes

LongCatIsLooong requested review from GaryQian and justinmc August 10, 2021 01:44

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 10, 2021

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

LongCatIsLooong force-pushed the editable-text-minor-fixes branch from e2d30bc to edcf3a8 Compare August 10, 2021 01:47

LongCatIsLooong force-pushed the editable-text-minor-fixes branch from edcf3a8 to 4696d1e Compare August 10, 2021 01:48

LongCatIsLooong commented Aug 10, 2021

View reviewed changes

fix tests

26baedc

LongCatIsLooong force-pushed the editable-text-minor-fixes branch from 78d4058 to 26baedc Compare August 10, 2021 04:42

LongCatIsLooong mentioned this pull request Aug 12, 2021

[EditableText] call onSelectionChanged only when there are actual selection/cause changes #87971

Merged

8 tasks

Merge remote-tracking branch 'upstream/master' into editable-text-min…

5084d29

…or-fixes

justinmc approved these changes Aug 13, 2021

View reviewed changes

LongCatIsLooong added 2 commits August 14, 2021 15:53

Merge remote-tracking branch 'upstream/master' into editable-text-min…

cdf6d12

…or-fixes

typo

7fb1b6b

LongCatIsLooong force-pushed the editable-text-minor-fixes branch from adb5189 to 7fb1b6b Compare August 16, 2021 16:41

Merge remote-tracking branch 'upstream/master' into editable-text-min…

754ca2b

…or-fixes

LongCatIsLooong added the waiting for tree to go green label Aug 16, 2021

fluttergithubbot merged commit c49eba6 into flutter:master Aug 16, 2021

LongCatIsLooong deleted the editable-text-minor-fixes branch August 16, 2021 19:12

blasten pushed a commit to blasten/flutter that referenced this pull request Aug 19, 2021

[TextInput] minor fixes (flutter#87973)

615cf27

Conversation

LongCatIsLooong commented Aug 10, 2021

Pre-launch Checklist

Uh oh!

LongCatIsLooong Aug 10, 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 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LongCatIsLooong Aug 10, 2021 •

edited

Loading