-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Make Route dispatching memory events. [prod-leak-fix] #133721
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| if (kFlutterMemoryAllocationsEnabled) { | ||
| MemoryAllocations.instance.dispatchObjectCreated( | ||
| library: 'package:flutter/widgets.dart', | ||
| className: '$Route<$T>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice this earlier, why would you need to provide a className if you already provide the object itself? Should the ObjectCreated uses the object.runtimeType if it needs the class name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runtimeType is different from type that owns instrumentation, because runtime type can be descender (like MaterialPageRoute). Does it help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for explanation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: why is Route<$T> a clearer name than MaterialPageRoute? I would have thought the more specific class name would be clearer for someone debugging.
| /// used instead. | ||
| Route({ RouteSettings? settings }) : _settings = settings ?? const RouteSettings(); | ||
| Route({ RouteSettings? settings }) : _settings = settings ?? const RouteSettings() { | ||
| if (kFlutterMemoryAllocationsEnabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I have not brought up this earlier. Should these logic be wrapped in an assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is proven that this code is tree-shaked when kFlutterMemoryAllocationsEnabled is false, because it is a compile time constant.
So, it is ok to go without assert.
Does this answer your question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thanks for explanation
| const TestPage page = TestPage(name: 'page'); | ||
| final Route<dynamic> route = page.createRoute(nav.currentContext!); | ||
| // ignore: invalid_use_of_protected_member | ||
| route.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do
nav.currentState!.push(MaterialPageRoute(builder: (_) => const PlaceHolder())); // This should create a route
await tester.pumpAndSettle();
nav.currentState!.pop();
await tester.pumpAndSettle(); // this should dispose the route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nav.currentState is null on first line
Did i miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you will need this first before push
await tester.pumpWidget(
MaterialApp(
navigatorKey: nav,
home: const Scaffold(
body: Text('home'),
)
)
);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied. Thank you!
| MemoryAllocations.instance.addListener(listener); | ||
|
|
||
| await createAndDisposeRoute(); | ||
| expect(events, hasLength(2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting, I would expect 3 events. One for MaterialPageRoute created from home page of MaterialApp. Two for creating and disposing the TestPage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah nvm, the home page will create MaterialPageRoute which will be filter out in the listener
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes!
chunhtai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| ) | ||
| ); | ||
|
|
||
| nav.currentState!.push(MaterialPageRoute<void>(builder: (_) => const Placeholder())); // This should create a route |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| nav.currentState!.push(MaterialPageRoute<void>(builder: (_) => const Placeholder())); // This should create a route | |
| nav.currentState!.push(MaterialPageRoute<void>(builder: (_) => const Placeholder())); // This should create a route. |
| await tester.pumpAndSettle(); | ||
|
|
||
| nav.currentState!.pop(); | ||
| await tester.pumpAndSettle(); // this should dispose the route. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| await tester.pumpAndSettle(); // this should dispose the route. | |
| await tester.pumpAndSettle(); // This should dispose the route. |
No description provided.