fix `SearchAnchor` disposing `SearchController` while it is still used by PurplePolyhedron · Pull Request #155219 · flutter/flutter

PurplePolyhedron · 2024-09-15T08:22:23Z

fixes #155180
New behaviour: SearchAnchor now closes the menu when itself is disposed while the menu is still open. This is the behaviour of MenuAnchor/OverlayPortal.

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

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

packages/flutter/lib/src/material/search_anchor.dart

PurplePolyhedron · 2024-09-16T10:48:40Z

I moved the disposal from _SearchViewRoute to _ViewContentState and added more comments.

nate-thegrate

I'm really impressed with how the search anchor has been updated to elegantly handle any combination of the search view route & search anchor being created/disposed.

My main concern is the concept of "spaghetti code": adding a few extra members isn't a huge deal but does make the API slightly more difficult to understand & maintain.

My current inclination is to wait a few days to see if anyone has ideas for improving readability; if not, I'd be happy to approve this 🙂

packages/flutter/lib/src/material/search_anchor.dart

packages/flutter/test/material/search_anchor_test.dart

packages/flutter/lib/src/material/search_anchor.dart

packages/flutter/test/material/search_anchor_test.dart

nate-thegrate

Thanks for the updates!

I think I figured out a way to use 1 boolean flag instead of 2:

class SearchController {
  // If set to true, the SearchAnchor or ViewContent should
  // dispose of the controller when it's no longer in use.
  bool _internal = false;
}

class _SearchAnchorState {
  SearchController get _searchController {
    return widget.searchController
        ?? (_internalSearchController ??= SearchController().._internal = true);
  }

  void dispose() {
    widget.searchController?._detach(this);
    _internalSearchController?._detach(this);

    final _SearchViewRoute? route = _route;
    if (route != null && route.isActive) {
      route.navigator!.removeRoute(route);
    } else {
      _internalSearchController?.dispose();
    }
    super.dispose();
  }
}


class _ViewContentState {
  void dispose() {
    if (_controller._internal && !_controller.isAttached) {
      _controller.dispose();
    }
  }
}

Let me know what you think!

PurplePolyhedron · 2024-09-18T13:06:04Z

Thanks for the updates!

I think I figured out a way to use 1 boolean flag instead of 2:

class SearchController {
  // If set to true, the SearchAnchor or ViewContent should
  // dispose of the controller when it's no longer in use.
  bool _internal = false;
}

class _SearchAnchorState {
  SearchController get _searchController {
    return widget.searchController
        ?? (_internalSearchController ??= SearchController().._internal = true);
  }

  void dispose() {
    widget.searchController?._detach(this);
    _internalSearchController?._detach(this);

    final _SearchViewRoute? route = _route;
    if (route != null && route.isActive) {
      route.navigator!.removeRoute(route);
    } else {
      _internalSearchController?.dispose();
    }
    super.dispose();
  }
}


class _ViewContentState {
  void dispose() {
    if (_controller._internal && !_controller.isAttached) {
      _controller.dispose();
    }
  }
}

Let me know what you think!

Thanks for the suggestion, I tried it but it crashes using the example from the issue. My bad for not covering this in the test cases. I've now added a new test for that.
I think the crash is due to the route is considered not isActive during the exit animation.

However it did inspired me to come up with a new idea.

QuncCccccc

LGTM! Thanks a lot for the fix!

QuncCccccc · 2024-09-24T23:28:40Z

packages/flutter/test/material/search_anchor_test.dart

    });
  });
+
+  testWidgets('SearchAnchor does not dispose external SeachController', (WidgetTester tester) async {


Suggested change

testWidgets('SearchAnchor does not dispose external SeachController', (WidgetTester tester) async {

testWidgets('SearchAnchor does not dispose external SearchController', (WidgetTester tester) async {

…till used (flutter/flutter#155219)

Roll Flutter from 4ca51a1 to 538e742 (40 revisions) flutter/flutter@4ca51a1...538e742 2024-09-25 [email protected] Marks Linux build_aar_module_test to be unflaky (flutter/flutter#155349) 2024-09-25 [email protected] Roll Packages from 4926c0f to 7da2374 (3 revisions) (flutter/flutter#155701) 2024-09-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Normalize TabBarTheme (#155476)" (flutter/flutter#155698) 2024-09-25 [email protected] Normalize TabBarTheme (flutter/flutter#155476) 2024-09-25 [email protected] increase both linux & windows tool integration test shards (flutter/flutter#155631) 2024-09-25 [email protected] Roll Flutter Engine from c7cd559e483b to d6d5fdba6ae1 (1 revision) (flutter/flutter#155693) 2024-09-25 [email protected] Add WidgetStateBorderSide example and tests for it. (flutter/flutter#155559) 2024-09-25 [email protected] Roll Flutter Engine from b9dd7a39dd58 to c7cd559e483b (1 revision) (flutter/flutter#155686) 2024-09-25 [email protected] Roll Flutter Engine from 87c1667dfd1e to b9dd7a39dd58 (1 revision) (flutter/flutter#155684) 2024-09-25 [email protected] Roll Flutter Engine from 05211f9d2267 to 87c1667dfd1e (1 revision) (flutter/flutter#155681) 2024-09-25 [email protected] Roll Flutter Engine from 8d1eb7410b49 to 05211f9d2267 (1 revision) (flutter/flutter#155672) 2024-09-25 [email protected] Roll Flutter Engine from ad3dd0df0fe7 to 8d1eb7410b49 (2 revisions) (flutter/flutter#155662) 2024-09-25 [email protected] Roll Flutter Engine from 746ce6124844 to ad3dd0df0fe7 (2 revisions) (flutter/flutter#155653) 2024-09-25 [email protected] Add PrivacyInfo.xcprivacy to macOS plugin template (flutter/flutter#155570) 2024-09-25 [email protected] Roll Flutter Engine from 559f2ff31c74 to 746ce6124844 (14 revisions) (flutter/flutter#155648) 2024-09-25 [email protected] fix `SearchAnchor` disposing `SearchController` while it is still used (flutter/flutter#155219) 2024-09-25 [email protected] Roll pub packages (flutter/flutter#155640) 2024-09-24 [email protected] Preserve transform when using *Gradient:withOpacity (flutter/flutter#154908) 2024-09-24 [email protected] fixed keyboardDismissBehavior on scroll without a drag (flutter/flutter#154675) 2024-09-24 [email protected] Roll Flutter Engine from 7cd3d0b1bb2e to 559f2ff31c74 (3 revisions) (flutter/flutter#155629) 2024-09-24 [email protected] Roll Flutter Engine from 2a13c3a27e1f to 7cd3d0b1bb2e (4 revisions) (flutter/flutter#155625) 2024-09-24 [email protected] Misc docs cleanup and fixes (flutter/flutter#155501) 2024-09-24 [email protected] Roll Flutter Engine from dc44f95b7027 to 2a13c3a27e1f (1 revision) (flutter/flutter#155619) 2024-09-24 [email protected] Roll Flutter Engine from 2745b8797025 to dc44f95b7027 (1 revision) (flutter/flutter#155616) 2024-09-24 [email protected] Roll Flutter Engine from 8a54cc56d4b9 to 2745b8797025 (2 revisions) (flutter/flutter#155610) 2024-09-24 [email protected] Roll Flutter Engine from c07812775255 to 8a54cc56d4b9 (2 revisions) (flutter/flutter#155607) 2024-09-24 [email protected] Roll Packages from 9de72be to 4926c0f (4 revisions) (flutter/flutter#155605) 2024-09-24 [email protected] Fix some broken links in DAP readme (flutter/flutter#155600) 2024-09-24 [email protected] Roll Flutter Engine from 22e4f015cc99 to c07812775255 (2 revisions) (flutter/flutter#155599) 2024-09-24 [email protected] [flutter_tools] Fix encoded stderr in "dart.log" from debug adapter to client (flutter/flutter#155249) 2024-09-24 [email protected] Roll Flutter Engine from 309468cfd1bb to 22e4f015cc99 (2 revisions) (flutter/flutter#155591) 2024-09-24 [email protected] [native assets] Roll dependencies (flutter/flutter#155432) 2024-09-24 [email protected] Roll Flutter Engine from 4013dc28a48b to 309468cfd1bb (2 revisions) (flutter/flutter#155588) 2024-09-24 [email protected] Roll Flutter Engine from 8a5af19a43f3 to 4013dc28a48b (1 revision) (flutter/flutter#155585) 2024-09-24 [email protected] Roll Flutter Engine from 95c5a0940ad9 to 8a5af19a43f3 (10 revisions) (flutter/flutter#155583) 2024-09-24 [email protected] Add `WidgetStateProperty` example and tests for it. (flutter/flutter#155315) 2024-09-24 [email protected] Redo flutter engine flutter autoroll bd3d1990 485b 419c 8c55 b27e3eeb15ed 1727117767 (flutter/flutter#155579) 2024-09-23 [email protected] Roll Flutter Engine from 61f0a3fbabbe to 9bb0ece79ae2 (2 revisions) (flutter/flutter#155549) 2024-09-23 [email protected] Assert macOS framework artifact contains xcprivacy manifest (flutter/flutter#155189) 2024-09-23 [email protected] Roll Packages from f54fe93 to 9de72be (1 revision) (flutter/flutter#155540) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. ...

flutter#155219) fixes flutter#155180 New behaviour: SearchAnchor now closes the menu when itself is disposed while the menu is still open. This is the behaviour of `MenuAnchor`/`OverlayPortal`.

…till used (flutter/flutter#155219)

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Sep 15, 2024

PurplePolyhedron marked this pull request as ready for review September 16, 2024 06:11

navaronbracke reviewed Sep 16, 2024

View reviewed changes

packages/flutter/lib/src/material/search_anchor.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/material/search_anchor.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/material/search_anchor.dart Outdated Show resolved Hide resolved

PurplePolyhedron added 12 commits September 16, 2024 21:22

Update search_anchor_test.dart

d6b61c5

Update search_anchor.dart

cdffd8f

Update menu_anchor.dart

cb30e0d

revert Update menu_anchor.dart

6f15e12

fix search_anchor.dart error

8b39e1b

Update search_anchor_test.dart

8622214

Update search_anchor_test.dart

b018ce6

Update search_anchor_test.dart

4b59559

Update search_anchor_test.dart

3e445f5

move disposal to ViewContent and update doc

10f7d2e

Update search_anchor.dart doc

bdcb9b6

Update search_anchor.dart doc

4c5f19a

PurplePolyhedron force-pushed the search_anchor_dispose branch from 431ef0a to 4c5f19a Compare September 16, 2024 11:22

nate-thegrate reviewed Sep 16, 2024

View reviewed changes

PurplePolyhedron added 3 commits September 17, 2024 16:50

Update search_anchor_test.dart

7f143c6

Update search_anchor.dart

e45cc99

Update search_anchor.dart

226e28e

nate-thegrate reviewed Sep 17, 2024

View reviewed changes

PurplePolyhedron added 6 commits September 18, 2024 23:08

Update search_anchor.dart

70bba6a

Update search_anchor_test.dart

6a2cd13

search_anchor.dart format

1192390

Update search_anchor_test.dart

d12c58d

Update search_anchor_test.dart

096a88a

Update search_anchor.dart

f4b71e0

PurplePolyhedron added 8 commits September 25, 2024 01:11

Update search_anchor.dart

2e33498

Update search_anchor_test.dart

a9d914a

Update search_anchor_test format

5e8d863

Update search_anchor_test format

62cab33

Update search_anchor_test.dart format

150c6ca

Update search_anchor_test.dart format

4e0f0b3

Merge branch 'master' into search_anchor_dispose

7cbac61

Merge branch 'master' into search_anchor_dispose

d205e8e

QuncCccccc approved these changes Sep 24, 2024

View reviewed changes

Update search_anchor_test.dart

e34910b

nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2024

auto-submit bot merged commit 7677aac into flutter:master Sep 25, 2024

engine-flutter-autoroll mentioned this pull request Sep 25, 2024

Roll Flutter from 4ca51a1de597 to 4d1f086a649f (33 revisions) flutter/packages#7695

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 25, 2024

7677aac fix SearchAnchor disposing SearchController while it is s…

c6c3b97

…till used (flutter/flutter#155219)

engine-flutter-autoroll mentioned this pull request Sep 25, 2024

Roll Flutter from 4ca51a1de597 to ce24dd6a76d2 (34 revisions) flutter/packages#7697

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 25, 2024

7677aac fix SearchAnchor disposing SearchController while it is s…

f913019

…till used (flutter/flutter#155219)

engine-flutter-autoroll mentioned this pull request Sep 25, 2024

Roll Flutter from 4ca51a1de597 to 048e38976159 (35 revisions) flutter/packages#7698

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 25, 2024

7677aac fix SearchAnchor disposing SearchController while it is s…

276c1b2

…till used (flutter/flutter#155219)

engine-flutter-autoroll mentioned this pull request Sep 25, 2024

Roll Flutter from 4ca51a1de597 to f310625bc6df (37 revisions) flutter/packages#7700

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 25, 2024

7677aac fix SearchAnchor disposing SearchController while it is s…

5156516

…till used (flutter/flutter#155219)

engine-flutter-autoroll mentioned this pull request Sep 25, 2024

Roll Flutter from 4ca51a1de597 to 1c9607fc9c98 (38 revisions) flutter/packages#7702

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 25, 2024

7677aac fix SearchAnchor disposing SearchController while it is s…

89d03e7

…till used (flutter/flutter#155219)

engine-flutter-autoroll mentioned this pull request Sep 25, 2024

Roll Flutter from 4ca51a1de597 to 538e7424d018 (40 revisions) flutter/packages#7705

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 25, 2024

7677aac fix SearchAnchor disposing SearchController while it is s…

9bc4c09

…till used (flutter/flutter#155219)

Piinks mentioned this pull request Sep 27, 2024

Flutter Framework Team: Notable Commits #121415

Closed

Mairramer mentioned this pull request Oct 15, 2024

SearchAnchor doesn't pop the search view if it's removed from the widget tree #154051

Closed

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

7677aac fix SearchAnchor disposing SearchController while it is s…

334d8ad

…till used (flutter/flutter#155219)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024

7677aac fix SearchAnchor disposing SearchController while it is s…

e783616

…till used (flutter/flutter#155219)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix `SearchAnchor` disposing `SearchController` while it is still used#155219

fix `SearchAnchor` disposing `SearchController` while it is still used#155219
auto-submit[bot] merged 34 commits intoflutter:masterfrom
PurplePolyhedron:search_anchor_dispose

PurplePolyhedron commented Sep 15, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PurplePolyhedron commented Sep 16, 2024

Uh oh!

nate-thegrate left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nate-thegrate left a comment

Uh oh!

PurplePolyhedron commented Sep 18, 2024 •

edited

Loading

Uh oh!

QuncCccccc left a comment

Uh oh!

QuncCccccc Sep 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	testWidgets('SearchAnchor does not dispose external SeachController', (WidgetTester tester) async {
	testWidgets('SearchAnchor does not dispose external SearchController', (WidgetTester tester) async {

Conversation

PurplePolyhedron commented Sep 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PurplePolyhedron commented Sep 16, 2024

Uh oh!

nate-thegrate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nate-thegrate left a comment

Choose a reason for hiding this comment

Uh oh!

PurplePolyhedron commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

QuncCccccc Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PurplePolyhedron commented Sep 15, 2024 •

edited

Loading

PurplePolyhedron commented Sep 18, 2024 •

edited

Loading