Skip to content
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

Add "Install Debug Extension(s)" Action #103891

Merged
merged 13 commits into from
Aug 13, 2020
Merged

Add "Install Debug Extension(s)" Action #103891

merged 13 commits into from
Aug 13, 2020

Conversation

bamurtaugh
Copy link
Member

The error message that appears when an extension is missing that contributes a certain debug type may be confusing or unclear to newer users (i.e. Configured debug type coreclr is not supported. may not make it evident that the C# extension is missing in .NET development).

I've added an Install Debug Extension(s) button that opens the Extensions view and filters by tag:debuggers @sort:installs:

image

image

A couple of outstanding questions:

  • What precise wording would we like for the button? What would make the most sense for users? When I ran into this error, if I saw a button that indicated I could go install extensions to fix it, I would've been interested and inclined to click it.
  • Should this button come before or after the button to Open launch.json?
  • If you see any implementation improvements, I'd be happy to iterate on the code as this is my first time contributing to the core VS Code repo.

This PR fixes #103719

@bamurtaugh bamurtaugh requested a review from isidorn August 4, 2020 02:27
@isidorn
Copy link
Contributor

isidorn commented Aug 4, 2020

@bamurtaugh thanks a lot for creating this PR and congrats on your first contribution to VS Code core 🎉
I like the work and here's some answers to your questions:

  • I would call the button "Install coreclr Extension". And it would still do the same thing. Definetely no (s) since we do not do that in our UI. I was also thinking that we could filter the debug extensions in the extension view by 'coreclr' keyword but unfortunetly this does not find the C# extension.
  • Open Launch.json should be the first action and the one selected by default, since I think that is the most likely action a user takes. Though this order also works for me
  • for the implementation improvements I will comment directly in the code

Thanks!

src/vs/workbench/contrib/debug/browser/debugService.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/debugService.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/debugService.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/debugService.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/debugService.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/debugService.ts Outdated Show resolved Hide resolved
@isidorn isidorn added this to the August 2020 milestone Aug 5, 2020
@isidorn
Copy link
Contributor

isidorn commented Aug 10, 2020

Let me know when this is ready for review - no hurries.
Until then assigning to On-Deck.

@isidorn isidorn modified the milestones: August 2020, On Deck Aug 10, 2020
@bamurtaugh
Copy link
Member Author

@isidorn Sounds great, thank you! Hoping to get some time to work on it more this week.

@bamurtaugh bamurtaugh requested a review from isidorn August 11, 2020 01:07
@bamurtaugh
Copy link
Member Author

@isidorn Thank you again for all of the help and feedback! I worked to incorporate your suggestions. Please feel free to review again whenever you get a chance and let me know what you think.

await this.showError(message);
const actionList: IAction[] = [];

actionList.push(new Action(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Action looks good!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@@ -118,6 +120,7 @@ export class DebugService implements IDebugService {

this.viewModel = new ViewModel(contextKeyService);
this.taskRunner = this.instantiationService.createInstance(DebugTaskRunner);
this.commandService = this.instantiationService.createInstance(CommandService);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way you are creating a new instance of the commandsService, which we do not want. We use dependency injetion across our codebase so that different objects can get ahold of services.
So you shuold just change the debug service consuctor arguments to take an additional argument, something like
@ICommandService private readonly commandService: ICommandService

The @ is a special vscode annotation which means inject me with that service.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the insights here- I added @ICommandService private readonly commandService: ICommandService as an argument to the constructor. Let me know what you think.

@isidorn
Copy link
Contributor

isidorn commented Aug 11, 2020

@bamurtaugh nice work. I have reviewed again and there is just one more thing which I commented in the code.

@bamurtaugh
Copy link
Member Author

@isidorn Thank you for reviewing again! I made some adjustments based on the last comment.

@isidorn
Copy link
Contributor

isidorn commented Aug 13, 2020

Looks good, merging in. Thanks for this PR 👏 ☀️

@isidorn isidorn merged commit 2e2f69d into microsoft:master Aug 13, 2020
@isidorn isidorn modified the milestones: On Deck, August 2020 Aug 13, 2020
@bamurtaugh
Copy link
Member Author

Thank you for all your help, @isidorn! This has been a great learning experience, and it's really neat to have it merged in 😄

@github-actions github-actions bot locked and limited conversation to collaborators Sep 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request for Debug to handle missing extension from configuration
3 participants