-
Notifications
You must be signed in to change notification settings - Fork 336
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
Initial structure for offline memory screen. #5965
Conversation
@@ -290,6 +291,17 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin { | |||
), | |||
); | |||
}, | |||
memoryAnalysisScreenId: (_, __, args, ____) { |
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 thought we decided on having one screen, memory
, and conditionally switching out the screen body widget based on the app connection state? That way we don't have to have a separate 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.
Our standard navigation to a screen only works if there is live connection. We do not have mechanism to navigate to the same route with connection and without it. If I pass ScreenMetaData.memory.id
as route on button click at landing page, navigation does not happen.
So, with current mechanism we need different route to connect to the screen offline.
I suggest to review/refactor this separately, in a right way, when we implement offline pattern.
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.
Our standard navigation to a screen only works if there is live connection.
Not necessarily. We already can navigate to the app size screen and any standalone screens without an app connection. I'd take another look at the stack trace - my guess is that the live connection requirement is coming from the memory screen code itself.
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.
Fine to refactor later though. Please hide this route behind the FeatureFlags.memoryAnalysis
flag. We don't want a user to be able to navigate to a screen that is not shipped.
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.
The code is copy-pasted from what we do for app size one block before:
appSizeScreenId: (_, __, args, ____) {
final embed = isEmbedded(args);
return DevToolsScaffold.withChild(
key: const Key('appsize'),
embed: embed,
child: MultiProvider(
providers: _providedControllers(),
child: const AppSizeBody(),
),
);
},
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.
The code is copy-pasted from what we do for app size one block before:
I'm not following. How does this relate to navigation? Navigation to the app size screen (which does not require an app connection) happens here: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/framework/landing_screen.dart#L351
I am saying that if you were unable to navigate to the memory screen without having an app connection, then that likely is due to code from the memory screen that assumes we have a connected app - not code from the navigation framework, which has no understanding of app connected state.
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.
Discussed offline. Fine to leave as is for now - we will likely refactor how both the app size and memory screens are accessed for static mode here soon with the evolution of static tooling support in DevTools (#5990)
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.
please hide behind the feature flag, then lgtm
Co-authored-by: Kenzie Davisson <[email protected]>
Lint errors are fixed here: #5966