fix SearchAnchor disposing SearchController while it is still used
#155219
Conversation
|
I moved the disposal from |
431ef0a
to
4c5f19a
Compare
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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. However it did inspired me to come up with a new idea. |
There was a problem hiding this comment.
This looks fantastic—I like this current setup a lot more than anything suggested previously!
I believe only one more change is needed:
@override
void didUpdateWidget(SearchAnchor oldWidget) {
super.didUpdateWidget(oldWidget);
if (oldWidget.searchController != widget.searchController) {
oldWidget.searchController?._detach(this);
_searchController._attach(this);
}
}Based on that method, it looks like there's an edge case where the SearchAnchor starts using the internal search controller, but later switches over to an external one—in that case, the internal controller would need to be disposed of.
| _route?._dismiss( | ||
| disposeController: widget.searchController == null, | ||
| ); |
There was a problem hiding this comment.
| _route?._dismiss( | |
| disposeController: widget.searchController == null, | |
| ); | |
| final bool usingExternalController = widget.searchController != null; | |
| _route?._dismiss(disposeController: !usingExternalController); | |
| if (usingExternalController) { | |
| _internalSearchController?.dispose(); | |
| } |
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
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.