Suppress 'Open launch.json' command on error dialog if DA provided a command (#124020) #130754
Conversation
| const label = error.urlLabel ? error.urlLabel : nls.localize('moreInfo', "More Info"); | ||
| const uri = URI.parse(url); | ||
| // Use a suffixed id if uri invokes a command, so default 'Open launch.json' command is suppressed on dialog | ||
| const actionId = uri.scheme === 'command' ? 'debug.moreInfo.command' : 'debug.moreInfo'; |
There was a problem hiding this comment.
Please use this scheme const for command
https://github.com/microsoft/vscode/blob/main/src/vs/base/common/network.ts#L49
| const configureAction = new Action(DEBUG_CONFIGURE_COMMAND_ID, DEBUG_CONFIGURE_LABEL, undefined, true, () => this.commandService.executeCommand(DEBUG_CONFIGURE_COMMAND_ID)); | ||
| const actions = [...errorActions, configureAction]; | ||
| // Don't append the standard command if id of any provided action indicates it is a command | ||
| const actions = errorActions.filter((action) => action.id.endsWith('.command')).length > 0 ? errorActions : [...errorActions, configureAction]; |
There was a problem hiding this comment.
Do we have to check here if it endsWith. That feels a bit hacky?
Maybe just if it contains the word command?
Or even better can we use the ICommandRegistry to check if the getCommand exists for that id. That way we will know if the command is good or not?
There was a problem hiding this comment.
This is a convention I devised so handleErrorResponse in rawDebugSession.ts can indicate that the Action it supplied (currently only one) invokes a command. Here in showError I don't think there's any way I can discover this directly from the Action.
Another idea would be for handleErrorResponse to set the id of the Action to 'command:' followed by the id of the command (i.e. uri.authority), then make this check startsWith('command:'). It could even use match to extract the command id, then validate using ICommandRegistry. In that case, what's best to do if it fails validation? Omit the action from the messagebox? Or add it but disabled? Or don't suppress the DEBUG_CONFIGURE_COMMAND_ID?
What do you think?
There was a problem hiding this comment.
I think the current approach makes sense, and that your second proposal is very similar but just might complicate the implementation a bit. Due to that I suggest to go with the current approach.
|
@gjsjohnmurray thanks a lot for this PR. I have commented directly in the code, let me know once you tackle my comments! Thanks! |
|
Thank you very much, this looks good. Merging in 👏 ☀️ |
This PR fixes #124020