Refactor TextSelectionOverlay by chunhtai · Pull Request #98153 · flutter/flutter

chunhtai · 2022-02-09T21:55:27Z

Refactoring TextSelectionOverlay by pulling out dependencies on renderEditable.

This PR creates a base class SelectionOverlay that set up overlay without depending on renderEditable and makes the original TextSelectionOverlay to wrap around SelectionOverlay.

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 this 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.

chunhtai

Tests will be added soon

chunhtai · 2022-02-09T21:56:54Z

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

  ///
  /// The [context] must not be null and must have an [Overlay] as an ancestor.
  TextSelectionOverlay({
    required TextEditingValue value,


The changes remove the getter for these parameter because it now directly pass it in SelectionOverlay, not sure whether any customer depends on it

It's worth it to make this change even if so. The migration shouldn't be too bad.

chunhtai · 2022-02-09T21:58:26Z

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

+    _effectiveEndHandleVisibility.dispose();
+  }
+
+  double _getStartGlyphHeight() {


these helper functions are directly copied from the old code

chunhtai · 2022-02-09T22:01:05Z

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

+  /// Creates an object that manages overlay entries for selection handles.
+  ///
+  /// The [context] must not be null and must have an [Overlay] as an ancestor.
+  SelectionOverlay({


Ideally selectionEndPoints and toolbarLocation should be removed once @justinmc refactor the TextSelectionControls.buildToolbar to not depends one these metrics

That sounds good based on my work so far 👍 . Still trying to get you a WIP PR for that...

Renzo-Olivares · 2022-02-10T23:21:29Z

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

+  void updateForScroll() => _updateSelectionOverlay();
+
+  /// Whether the handles are currently visible.
+  bool get handlesAreVisible => _selectionOverlay._handles != null && handlesVisible;


Should _selectionOverlay._handles and _selectionOverlay._toolbar have public getters? I thought a leading underscore signified private.

they could, but both handlesAreVisible and toolbarIsVisible feel unnecessary based on how they were currently used. I would probably remove it if we were to start over, so I decide to hide them in SelectionOverlay unless someone ask for it in the future.

Renzo-Olivares · 2022-02-11T00:03:41Z

packages/flutter/test/widgets/text_selection_test.dart

+              child: const Text('toolbar'),
+            ),
+          ],
+        )


Nit: trailing commas

Renzo-Olivares

  _    ___ _____ __  __ 
 | |  / __|_   _|  \/  |
 | |_| (_ | | | | |\/| |
 |____\___| |_| |_|  |_|

justinmc

LGTM 👍

The TextSelectionOverlay / SelectionOverlay split makes sense, I think it's a good solution to this problem.

Looks like it shouldn't interfere much with my "context menu anywhere" work either.

justinmc · 2022-02-11T18:16:18Z

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

+    renderObject.selectionStartInViewport.addListener(_updateHandleVisibilities);
+    renderObject.selectionEndInViewport.addListener(_updateHandleVisibilities);
+    _updateHandleVisibilities();
+    _selectionOverlay = SelectionOverlay(


Nit: Just out of curiosity, could this be done in an initializer?

No, the _handleSelectionStartHandleDragStart and other methods need to be static methods in order to be used in initializer, but they can't be static.

Got it, I figured there was a reason 👍

justinmc · 2022-02-11T18:20:34Z

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

+  /// Creates an object that manages overlay entries for selection handles.
+  ///
+  /// The [context] must not be null and must have an [Overlay] as an ancestor.
+  SelectionOverlay({


That sounds good based on my work so far 👍 . Still trying to get you a WIP PR for that...

justinmc · 2022-02-11T18:22:48Z

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

-  }
 }

+


Nit: Unnecessary newline here.

justinmc · 2022-02-11T18:26:03Z

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

  ///
  /// The [context] must not be null and must have an [Overlay] as an ancestor.
  TextSelectionOverlay({
    required TextEditingValue value,


It's worth it to make this change even if so. The migration shouldn't be too bad.

fluttergithubbot · 2022-02-11T20:05:20Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Mac framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.

* Refactor TextSelectionOverlay * fix test * remove print * fix more tests * update * added tests * fix comments * fmt * fix test * addressing comment * remove dispose * remove new line

Refactor TextSelectionOverlay

a07cd36

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Feb 9, 2022

fix test

43c089b

chunhtai commented Feb 9, 2022

View reviewed changes

chunhtai added 6 commits February 9, 2022 15:23

remove print

b7b4af4

fix more tests

c4fb1f2

update

55d14b2

added tests

8a99fcf

fix comments

febc981

fmt

a795ecd

chunhtai requested review from Renzo-Olivares and justinmc February 10, 2022 19:42

fix test

0f19acb

Renzo-Olivares reviewed Feb 10, 2022

View reviewed changes

Renzo-Olivares reviewed Feb 11, 2022

View reviewed changes

Renzo-Olivares approved these changes Feb 11, 2022

View reviewed changes

chunhtai added 2 commits February 10, 2022 17:02

addressing comment

b8e2b87

remove dispose

960ea1f

justinmc approved these changes Feb 11, 2022

View reviewed changes

remove new line

99915ca

chunhtai added the waiting for tree to go green label Feb 11, 2022

fluttergithubbot removed the waiting for tree to go green label Feb 11, 2022

chunhtai added the waiting for tree to go green label Feb 11, 2022

chunhtai merged commit d486aa7 into flutter:master Feb 11, 2022

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Flutter from 8cd540f449a7 to d486aa704005 (4 revisions) flutter/packages#789

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2022

d486aa7 Refactor TextSelectionOverlay (flutter/flutter#98153)

86e47b1

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Flutter from 8cd540f449a7 to 61ac285608cd (5 revisions) flutter/packages#790

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2022

d486aa7 Refactor TextSelectionOverlay (flutter/flutter#98153)

2657eaf

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Flutter from 8cd540f449a7 to 61ac285608cd (5 revisions) flutter/plugins#4810

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 11, 2022

d486aa7 Refactor TextSelectionOverlay (flutter/flutter#98153)

83ce4ca

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Flutter from 8cd540f449a7 to eac1a548ed9b (6 revisions) flutter/packages#791

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2022

d486aa7 Refactor TextSelectionOverlay (flutter/flutter#98153)

a7ab51f

koji-1009 mentioned this pull request Mar 18, 2022

Depending on packages which use TextSelectionOverlay.fadeDuration results in build failure #100343

Closed

Conversation

chunhtai commented Feb 9, 2022

Pre-launch Checklist

Uh oh!

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

Renzo-Olivares Feb 10, 2022 • 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!

Renzo-Olivares Feb 11, 2022 • 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!

Renzo-Olivares left a comment • 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 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!

fluttergithubbot commented Feb 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Renzo-Olivares Feb 10, 2022 •

edited

Loading

Renzo-Olivares Feb 11, 2022 •

edited

Loading

Renzo-Olivares left a comment •

edited

Loading