-
Notifications
You must be signed in to change notification settings - Fork 330
Add widget previewer tool window #8595
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
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.
Nice!
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 overall! Thanks for doing this Helin!
| final List<String> args = new ArrayList<>(); | ||
| args.add("start"); | ||
| args.add("--web-server"); | ||
| args.add("--machine"); |
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.
We should also pass --dtd-url=<url> here and, if the plugin hosts a DevTools instance, --devtools-server-address=<url>.
Does the Intellij plugin provide the DTD Editor service?
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 okay, both should be available. How does the the widget preview interact with DevTools?
IntelliJ is intended to serve as an Editor, at least if we're thinking of the same thing (https://github.com/dart-lang/sdk/blob/bc7e4d9fb583c9edeb90393f65989767e865e56f/pkg/dtd_impl/dtd_common_services_editor.md?plain=1#L4), but looking at the code now I'm not sure if this was implemented.
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.
How does the the widget preview interact with DevTools?
For the current beta, it doesn't directly interact with DevTools and passing this flag just prevents Flutter Tools from serving another DevTools instance. In the near future, this DevTools instance will be embedded in the widget previewer in an iframe which displays the widget inspector.
IntelliJ is intended to serve as an Editor, at least if we're thinking of the same thing (https://github.com/dart-lang/sdk/blob/bc7e4d9fb583c9edeb90393f65989767e865e56f/pkg/dtd_impl/dtd_common_services_editor.md?plain=1#L4), but looking at the code now I'm not sure if this was implemented.
Ah, it'd be great if this was implemented. The widget previewer can provide additional functionality if this service is available, including filtering previews based on the currently selected file and jumping to source from file URIs in stack traces displayed for previews which throw exceptions.
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.
@bkonyi could you file an issue to this repo about implementing the editor services and what functionality will be available for each service that's relevant, maybe with examples from VS Code if you know of them?
|
|
||
| @Override | ||
| public @NotNull String getBrowserUrl() { | ||
| return url + "?theme=" + (isBackgroundBright ? "light" : "dark"); |
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.
Do we support background and foreground colors for theming DevTools? If so, we should also pass those values here.
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.
We only use the params backgroundColor (hex code) and theme ('light' or 'dark') from the IJ plugin. I don't know if DevTools has other params available that we're not using though. @elliette do you know?
|
Thanks for the reviews! I'm going to land this for now for the dev build over the weekend, but I'll address the outstanding comments from Ben next week. |
Follow up to #8595 At the moment I don't see consequences to having or not having the DTD connection, but I expect that in the future, the widget preview will respond to theme change updates over DTD. (and potentially other things?) Based on the conversation in #8595 (comment), the DevTools URI also does not have a visible consequence, but it should prevent flutter tools from starting its own DevTools instance when widget-preview is run.
Changes include:
Things that may change further:
Addresses #8587