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

Allow debug extensions to provide configurations to display in Select and Start Debugging #88230

Closed
connor4312 opened this issue Jan 7, 2020 · 48 comments
Assignees
Labels
api-finalization debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-testplan
Milestone

Comments

@connor4312
Copy link
Member

connor4312 commented Jan 7, 2020

Goals

In some scenarios, it's possible for the debug adapters to provide a quality debugging experience without additional user configuration. For example, the new js-debug can debug npm scripts, and languages where entrypoints are easily identifiable. Go, C#, Java... may offer to debug the program without extra configuration.

In a similar way to how build tasks are provided, we'd like the ability to provide default run tasks that show up under the Select and Start Debugging quick-pick.

image

image

Proposal

Launch configurations already have an optional presentation key, with the following properties (this interface is not exposed in vscode.d.ts):

export interface IConfigPresentation {
	hidden?: boolean;
	group?: string;
	order?: number;
}

This proposes adding a new property:

export interface IConfigPresentation {
+	locations: ('debug.start' | 'debug.selectandstart' | 'debugView')[];

When set, this configures the locations in the UI where the returned configuration is displayed. If not provided, the locations returned from DebugConfigurationProvider.provideDebugConfigurations default to ['debug.start']. provideDebugConfigurations would now be called when the select and start quick-pick is opened to retrieve configurations to display.

Questions

  • Why are we proposing this over the existing initial configuration/F5 experience?

    • It's hard (impossible?) to discover if you have any configured launch configurations, you don't get the quick pick again;
    • Start debugging is contextual to the active text editor file, where the these tasks are intended to be project-level.
  • Should we surface all existing provided configurations in select and start?

    • + this would require no change to existing adapters
    • - existing returned tasks are contextual to the active editor, while select and start currently only displays project-level configurations.
    • - the names of most existing tasks are simply "Node.js" or "Go", which don't provide explaination and would confuse users in select and start.
  • Should there be a separate method to return the pick-able configurations?

    • + in this proposal, we start calling provideDebugConfigurations more frequently, which may lead to side-effects or slowdowns; a separate method avoids that.
    • + this proposal is somewhat of an overload to the existing method to potentially return a new (non-contextual) type of task.
    • - this proposal has a smaller API and the property fits in well with the presentation properties
    • - the API here allows configurations to be surfaced in multiple locations easily, including "Run and Debug" if we want to allow that.
  • Should we also be able to provide default configurations for the "Run and Debug" dropdown in the debug view? added

    • + this is most discoverable for users who don't enter through the command palette
    • + allowing this as an option in locations would let consumers in launch.json take advantage of it, rather than hidden: true/false.
    • - in cases where there are many discovered tasks (e.g. lots of npm scripts) this could get noisy
    • - would we need to call provideDefaultLaunchConfigurations every time the debug view is opened? If files change, would we call that again?

cc @isidorn @roblourens @weinand

microsoft/vscode-js-debug#197

@connor4312 connor4312 added debug Debug viewlet, configurations, breakpoints, adapter issues api-proposal labels Jan 7, 2020
@connor4312 connor4312 added this to the January 2020 milestone Jan 7, 2020
@connor4312 connor4312 self-assigned this Jan 7, 2020
@roblourens
Copy link
Member

Are you saying that when the user presses F5 and sees this:

image

Then there would be a second quick pick with the list of possible launch configs? Or that they would show up in a dropdown in the viewlet like configured ones?

I could see a flow like we do for tasks, where you see the list of autodetected configs, but then it prompts you to "configure" that task by adding it to a tasks.json file.

@connor4312
Copy link
Member Author

connor4312 commented Jan 7, 2020

In Select and Start Debugging

image

There would be a new section with discovered configuration, something like:

image

I imagine selecting those would immediately run the chosen configuration. I think it would be good to allow the user some way to persist the config into their launch.json file, but I'm not sure the best way for that interaction to work. Perhaps discovered configs appear as intellisense autocompletion entries while editing launch.json?

@roblourens
Copy link
Member

roblourens commented Jan 7, 2020

Oh I didn't know that's what that menu was called, yeah it is similar to tasks then, where we show configured and detected tasks.

image

One annoying thing about this for tasks is that the detection slows down showing that menu. But I think the discovery that debuggers do is probably simpler than that for tasks.

@connor4312
Copy link
Member Author

I don't see an API for it, but it would be nice to have a way to update the items in a quick pick after instantiation. That'd allow us to show some 'loading' placeholder to avoid delaying the pick opening. But yes I think the debug discovery should generally be simple.

@isidorn
Copy link
Contributor

isidorn commented Jan 8, 2020

I think this makes sense overall to me.
If I understand everything correctly you want to provide dynamic launch configuraitons which are not written in launch.json. This sounds like a duplicate of #54212
Since that issue was driven by @weinand I will let him comment here

As for where we surface this in the UX we can sync on that once we figure out the API. The select and start is not so discoverable, so I would push more that we just show them everywhere when it makes sense.

@connor4312
Copy link
Member Author

I think that showing them by default in debug view's dropdown would be nice for discoverability. But I think it's important to be able to easily turn them off; once I have my project built and set up with custom launch configs, I may no longer need any of the auto-discovered tasks. Maybe the gear beside the dropdown turns into a menu with the options [x] Auto-discover configurations, Edit launch.json.

I also want to be careful to avoid cluttering the dropdown like build tasks can get.

I'll update the proposal to add debugView as a possible location.

@weinand
Copy link
Contributor

weinand commented Jan 9, 2020

If I understand this request correctly, I can break it down into one new VS Code feature and two independent API requests:

  • New feature: a new locations property in a launch config's presentation section determines where in the UI VS Code will show the launch config. This feature is independent from the extension API and applies to launch configurations in general: for every launch config the locations attribute can be used to determine where it appears in the UI.
  • API for contributing launch configurations dynamically (somewhat related to Debug Provider to return launch configurations 'in-memory' to the debugger #54212 as pointed out by @isidorn). But as already discussed in Debug Provider to return launch configurations 'in-memory' to the debugger #54212, I think it is questionable whether an "in-memory-only debug configuration" is really that useful. In Debug Provider to return launch configurations 'in-memory' to the debugger #54212 we concluded that regular VS Code "commands" are a more useful abstraction. That means that we could use existing extension API for contributing commands to the debug UI. And the provideDebugConfigurations hook should only be used to contribute the "source" of launch configs to launch.json but not for a more dynamic use in the UI where the source is not visible (and editable) to the end user. The npm: run start command shown above in the screenshot is a good example for this approach: instead of wrapping a npm script into an internal (in-memory) "launch config", we could just contribute a "run task" command to the debug QuickPick.
  • API request: reflect the new presentation attribute of debug configurations properly in the extension API (including the new locations attribute). Here is my take on that:
      export interface DebugConfiguration {
          // ...
          presentation?: DebugConfigPresentation;
      } 
    
      export interface DebugConfigPresentation {
          hidden?: boolean;
          group?: string;
          order?: number;
          locations?: DebugUILocation[];
      }
    
      export type DebugUILocation = 'debug.start' | 'debug.selectandstart' | 'debugView';

IMO the first and the last items make complete sense but the second item needs more discussion. I've removed the "api-proposal" for now and added a "feature-request" label.

@weinand weinand added feature-request Request for new features or functionality and removed api-proposal labels Jan 9, 2020
@connor4312
Copy link
Member Author

  • The downside of contributing the command would be that we can't expand them dynamically (e.g. a single action for npm scripts). But that would solve the bloat issue, the concerns about adding additional calls to provideDebugConfigurations, and performance questions when opening select and start and run and debug, so I like that approach 🙂
  • Adding locations in the API would be less useful with the command approach since adapters only emit it in a meaningful way from the existing provideDebugConfigurations where display intent probably is not known. I can't think of when I'd want to use it off the top of my head, but someone might find cases for it.

@weinand
Copy link
Contributor

weinand commented Mar 27, 2020

After revisiting this API feature request and investigating how to improve "single file debugging" I got some new insights:

In bullet item two of my comment above I had argued that the value of "in-memory-only debug configuration" is questionable because there is no need for editing them. Instead we could just allow to add regular commands to the debug configuration drop-down menu (and other places).

But now after having reviewed what "tasks" is doing and what other feature requests (e.g. #92269) are asking for, I'm leaning more towards the following "homogenous" approach (similar to "tasks"):

  • there is only a single type of debug configuration: the JSON-based one a user sees in the launch.json. The downside of this is that regular commands need to be wrapped as debug configurations but the advantage is, that there is an editable representation for them (see below).
  • a debug configuration has optional presentation hints for controlling where it is shown in the UI.
  • expanded debug extension API for creating debug configuration in the following situations:
  • all debug configurations can be launched with the startDebugging API (API exists).
  • all debug configurations can be easily added as JSON to a launch.json and become available for further customization (today we only have UX for the initial (empty) launch.json case; additional debug configs can be added via snippets),

Since we are not introducing new types of debug configurations, the VS Code implementation does not require big changes.

Work needed:

  • calling into debug extensions for acquiring the dynamic launch configs,
  • supporting the presentation hints,
  • optional: designing UX for adding dynamic debug configurations to launch.json

Proposed API:

/**
 * Debug UI locations.
 */
export type DebugUILocation = 'debug.start' | 'debug.selectandstart' | 'debugView';

/**
 * How a debug configuration is presented in the UI.
 */
export interface DebugConfigPresentation {
  hidden?: boolean;
  group?: string;
  order?: number;
  locations?: DebugUILocation[];
}

/**
 * Configuration for a debug session.
 */
export interface DebugConfiguration {
  // ...
  presentation?: DebugConfigPresentation;
}

export interface DebugConfigurationProvider {
  /**
   * Provides [debug configuration](#DebugConfiguration). If more than one debug configuration provider is
   * registered for the same type, debug configurations are concatenated in arbitrary order.
   *
   * @param token A cancellation token.
   * @return An array of [debug configurations](#DebugConfiguration).
   */
  provideDynamicDebugConfigurations?(token?: CancellationToken): ProviderResult<DebugConfiguration[]>;
}

@isidorn
Copy link
Contributor

isidorn commented Mar 27, 2020

This makes good sense to me and feels like a natural progression of the current api.

Some minor comments:

  1. Can you clarify a bit the DebugUILocation each value and how would that look in the UI - I confess I did not read the whole discussion before the last comment. debug.start I guess it is only in the drop down, but why would we have a configuration which is only available in the selectandstart? These two experiences need to be aligned imho. Also what does debugView mean
  2. Should the provideDynamicDebugConfigurations also get a folder: WorkspaceFolder | undefined as an argument. And in multi root workspaces we would call it for each folder.
  3. We would call this provideDynamicDebugConfigurations as soon as each debug extension gets activated. Does that make sense?
  4. We can always have a "Pin configuration" command / button which just writes it into launch.json
  5. provideDebugConfiguariotns seems like a good name for this, however since that one is already taken using the word dynamic makes sense, however something more cleaner would be even better. I do not have good ideas...

@awschristou
Copy link

Should the provideDynamicDebugConfigurations also get a folder: WorkspaceFolder | undefined as an argument. And in multi root workspaces we would call it for each folder.

It would be ideal if provideDynamicDebugConfigurations was called more often then during extension activation. If an implementation returned configurations based on workspace contents, it would be great for users to be able to update those contents, and have extensions offer up corresponding configurations without requiring an extension restart.

@weinand
Copy link
Contributor

weinand commented Mar 30, 2020

@isidorn @awschristou thanks for the feedback.

Here my replies to the bullets:

  1. We have different locations in the UI where we show debug configurations: the drop down menu in the debug viewlet, the "Select and Start Debugging" command, the Welcome View. With the locations attribute a provider can control where a debug configuration shows up (to avoid clutter in the UI). The problem with locations is that it semantically overlaps with the hidden presentation attribute, so I'm inclined to drop the locations attribute. @connor4312 could you please try to convince us why we need the locations attribute?
  2. provideDynamicDebugConfigurations is modelled after TaskProvider.provideTasks which does not have a folder argument. @dbaeumer and @alexr00 could you please shed some light on why TaskProvider.provideTasks does not need the folder?
  3. provideDynamicDebugConfigurations needs to be called whenever we need the list of "auto-discovered" debug configurations. This might be the case when opening the debug configuration drop down menu, or when running the "Select and Start Debugging" command. But we have to be careful not to run into the same scalability issues as tasks (performance and too many configs detected). See Allow debug extensions to provide configurations to display in Select and Start Debugging #88230 (comment).
  4. yes, a "Pin configuration" command/button would be a good UX for copying a "auto-discovered" debug configuration into the launch.json for further configuration.
  5. yes, provideDynamicDebugConfigurations is not the best name. What's about provideDetectedDebugConfigurations?

@weinand
Copy link
Contributor

weinand commented Apr 22, 2020

@isidorn I've created new issues #95835 and #95836 for the VS Code implementation work.

@weinand weinand removed the under-discussion Issue is under discussion for relevance, priority, approach label Apr 22, 2020
@isidorn
Copy link
Contributor

isidorn commented Apr 22, 2020

@weinand thanks.

@weinand
Copy link
Contributor

weinand commented Apr 22, 2020

@connor4312 you might want to try the new feature with js-debug (may be as part of testing this; see #95837).

@weinand
Copy link
Contributor

weinand commented Apr 26, 2020

In the API meeting it was suggested to rename the enum DebugConfigurationProviderScope to DebugConfigurationProviderTrigger (because "scope" is a rather unusual term in the VS Code extension API).

With this the final proposal becomes:

/**
 * A DebugConfigurationProviderTrigger specifies when the `provideDebugConfigurations` method of a `DebugConfigurationProvider` is triggered.
 * Currently there are two situations: to provide the initial debug configurations for a newly created launch.json or
 * to provide dynamically generated debug configurations when the user asks for them through the UI (e.g. via the "Select and Start Debugging" command).
 * A trigger is used when registering a `DebugConfigurationProvider` with #debug.registerDebugConfigurationProvider.
 */
export enum DebugConfigurationProviderTrigger {
	/**
	 *	`DebugConfigurationProvider.provideDebugConfigurations` is called to provide the initial debug configurations for a newly created launch.json.
	 */
	Initial = 1,
	/**
	 * `DebugConfigurationProvider.provideDebugConfigurations` is called to provide dynamically generated debug configurations when the user asks for them through the UI (e.g. via the "Select and Start Debugging" command).
	 */
	Dynamic = 2
}

export namespace debug {
	/**
	 * Register a [debug configuration provider](#DebugConfigurationProvider) for a specific debug type.
	 * The optional [trigger](#DebugConfigurationProviderTrigger) can be used to specify when the `provideDebugConfigurations` method of the provider is triggered.
	 * Currently two triggers are possible: with the value `Initial` (or if no trigger argument is given) the `provideDebugConfigurations` method is used to provide the initial debug configurations to be copied into a newly created launch.json.
	 * With the trigger `Dynamic` the `provideDebugConfigurations` method is used to dynamically determine debug configurations to be presented to the user (in addition to the static configurations from the launch.json).
	 * Please note that the `trigger` argument only applies to the `provideDebugConfigurations` method: so the `resolveDebugConfiguration` methods are not affected at all.
	 * Registering a single provider with resolve methods for different triggers, results in the same resolve methods called multiple times.
	 * More than one provider can be registered for the same type.
	 *
	 * @param type The debug type for which the provider is registered.
	 * @param provider The [debug configuration provider](#DebugConfigurationProvider) to register.
	 * @param trigger The [trigger](#DebugConfigurationProviderTrigger) for which the 'provideDebugConfiguration' method of the provider is registered.
	 * @return A [disposable](#Disposable) that unregisters this provider when being disposed.
	 */
	export function registerDebugConfigurationProvider(debugType: string, provider: DebugConfigurationProvider, trigger?: DebugConfigurationProviderTrigger): Disposable;
}

@jrieken @connor4312 @isidorn I've optimistically released the change (hoping that it will be accepted in the API meeting)

@jrieken
Copy link
Member

jrieken commented Apr 27, 2020

fyi - we haven't yet used Trigger by itself but as TriggerKind, e.g SignatureHelpTriggerKind and CompletionTriggerKind

@weinand
Copy link
Contributor

weinand commented Apr 27, 2020

@jrieken thanks, renamed DebugConfigurationProviderTrigger to DebugConfigurationProviderTriggerKind

@connor4312 @isidorn FYI

@weinand
Copy link
Contributor

weinand commented Apr 27, 2020

The test of this API is covered by test item #95837

@jrieken
Copy link
Member

jrieken commented Apr 28, 2020

Re-open to keep this item around for API finalization

@weinand
Copy link
Contributor

weinand commented May 5, 2020

The API submitted for finalisation is this:

/**
 * A DebugConfigurationProviderTriggerKind specifies when the `provideDebugConfigurations` method of a `DebugConfigurationProvider` is triggered.
 * Currently there are two situations: to provide the initial debug configurations for a newly created launch.json or
 * to provide dynamically generated debug configurations when the user asks for them through the UI (e.g. via the "Select and Start Debugging" command).
 * A trigger kind is used when registering a `DebugConfigurationProvider` with #debug.registerDebugConfigurationProvider.
 */
export enum DebugConfigurationProviderTriggerKind {
  /**
   * `DebugConfigurationProvider.provideDebugConfigurations` is called to provide the initial debug configurations for a newly created launch.json.
   */
  Initial = 1,
  /**
   * `DebugConfigurationProvider.provideDebugConfigurations` is called to provide dynamically generated debug configurations when the user asks for them through the UI (e.g. via the "Select and Start Debugging" command).
   */
  Dynamic = 2
}

export namespace debug {
  /**
   * Register a [debug configuration provider](#DebugConfigurationProvider) for a specific debug type.
   * The optional [triggerKind](#DebugConfigurationProviderTriggerKind) can be used to specify when the `provideDebugConfigurations` method of the provider is triggered.
   * Currently two trigger kinds are possible: with the value `Initial` (or if no trigger kind argument is given) the `provideDebugConfigurations` method is used to provide the initial debug configurations to be copied into a newly created launch.json.
   * With the trigger kind `Dynamic` the `provideDebugConfigurations` method is used to dynamically determine debug configurations to be presented to the user (in addition to the static configurations from the launch.json).
   * Please note that the `triggerKind` argument only applies to the `provideDebugConfigurations` method: so the `resolveDebugConfiguration` methods are not affected at all.
   * Registering a single provider with resolve methods for different trigger kinds, results in the same resolve methods called multiple times.
   * More than one provider can be registered for the same type.
   *
   * @param type The debug type for which the provider is registered.
   * @param provider The [debug configuration provider](#DebugConfigurationProvider) to register.
   * @param triggerKind The [trigger](#DebugConfigurationProviderTrigger) for which the 'provideDebugConfiguration' method of the provider is registered. If `triggerKind` is missing, the value `DebugConfigurationProviderTriggerKind.Initial` is assumed.
   * @return A [disposable](#Disposable) that unregisters this provider when being disposed.
   */
  export function registerDebugConfigurationProvider(debugType: string, provider: DebugConfigurationProvider, triggerKind?: DebugConfigurationProviderTriggerKind): Disposable;
}

@awschristou
Copy link

awschristou commented May 5, 2020

@weinand would it be reasonable to indicate in the comments what the behavior is when triggerKind is not provided to registerDebugConfigurationProvider?

What I meant to ask was if the default behavior could be added to the @param triggerKind line too

@weinand weinand closed this as completed in 7b34305 May 5, 2020
@weinand
Copy link
Contributor

weinand commented May 5, 2020

Moved the proposed API to vscode.d.ts.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-finalization debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-testplan
Projects
None yet
Development

No branches or pull requests

7 participants