-
Notifications
You must be signed in to change notification settings - Fork 327
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
Show extension screens in the VS code sidebar DevTools dropdown #6709
Show extension screens in the VS code sidebar DevTools dropdown #6709
Conversation
Cool! I'm a little less sold on my reversal of alignment/icons in the menu with a sub-menu though.. it looks a little odd, but I don't have any suggestions to improve it 🙃
I think we should be able to get this - but it might not be guaranteed to always be there (for example, you can create a launch config that just runs a loose (we need to handle it missing anyway for older versions of VS Code that aren't sending it) |
I've made some Dart-Code changes (Dart-Code/Dart-Code@84e75c0) to provide a I've also pushed the change in a Dart-Code pre-release ( Note: I have not done any testing for what happens if you call |
See #6709 Dart-Code will pass a project root* for debug sessions: ![image](https://github.com/flutter/devtools/assets/1078012/7f01b54c-bbbc-408f-8074-4a3af0f99338) \* if it's able to compute one - which it always should for the cases we care about, but there are some edge cases that mean the field might be missing.
I think the handling of required fields might be incomplete.. When I miss only
But Anyway, the reason for my error was likely that my project was out of date. Using the e2e example with some rigging of the config, I got further: And it triggered a new browser window when clicked: I think that error is because the way I rigged my config was only for the sidebar and doesn't apply here. I'll do some more digging later though. It's promising that it opened in an external window rather than failed, because it means even if we fix it to open embedded, we don't need any capabilities for this, the current behaviour is better than disabling it (assuming we can fix the above). |
for the issue with |
Seems like when I'm running
This happens even if I set
I'm wondering if I have some bad versions of things being picked along the day (such as the build above maybe just called "dart" that might be taking stable from |
Ok, I got this working - I think the dependency_override in DDS for devtools_shared is what was required (and a bad version of dart/flutter caused the other failures when I tried this above). Opening an extension does work - though currently in an external window: ext.mp4We can look at supporting embedded, but I think this is good enough for now. One thing to note though (and this may or may not be expected), the extensions did not show up in the list when running in the sidebar (they did show up if I opened DevTools in an external window where it had a VM Service connection). To test opening the page above I had to hard-code the extension in the nav: |
Great! can we remove
How can I reproduce this locally with a locally running DevTools server and locally running DevTools app in the sidebar? |
Oops, good catch - fixed!
This might change slightly depending on the outcome of discussion on Discord, but for now:
|
@DanTup where is the best place to test this functionality? I didn't see where the sidebar is tested in the devtools/ repo. Do we have testing in the Dart-Code plugin? |
We don't have many tests for this yet. When we first added it it seemed like there was going to be more design input and I was waiting to try and minimize the changes needed to tests. I'm not sure what happened about that though and I got busy with other things 😔 Testing the actual contents of the sidebar (eg. things inside the webview) from Dart-Code is tricky (and would probably make catching issues a slow round-trip) so I think it's better to have those for functionality here. The mock VS Code environment has a kind of mock version of the VS Code API that would probably help with these tests. |
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, LGTM :)
@DanTup I added tests, can you please review? thanks |
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.
Looks great, thanks!
Hey @DanTup, I was experimenting with how we might do this and thought I would push up a draft. This uses a fake list of extensions from the vs code sidebar stager app. We may need to tag team this feature.
I think we will need to make a request to the DevTools server directly to get the list of available extensions for a given debug session: https://github.com/flutter/devtools/blob/master/packages/devtools_shared/lib/src/server/server_api.dart/#L214. We will need to pass the root path for the given debug session to this DevTools server request. Can we get that from the [VsCodeDebugSession] object? If we can add the data there, that would be much simpler than having to spin up another VM service connection just to detect the root path of the main isolate.