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 support for a new command: cpptools.setActiveConfigName #4893

Merged
merged 4 commits into from Jan 30, 2020
Merged

add support for a new command: cpptools.setActiveConfigName #4893

merged 4 commits into from Jan 30, 2020

Conversation

aleksey-sergey
Copy link
Contributor

relates to #4870

New command allows other extensions to control activeConfigName.
Can be used like in a code snippet below:

async function func() {
  await vscode.commands.executeCommand("cpptools.setActiveConfigName", "config_A");
}

I can also update the documentation but need help to figure out an appropriate place for this.

Command allows other extensions to control activeConfigName
@msftclas
Copy link

msftclas commented Jan 24, 2020

CLA assistant check
All CLA requirements met.

Extension/src/LanguageServer/client.ts Outdated Show resolved Hide resolved
@Colengms
Copy link
Collaborator

I can also update the documentation but need help to figure out an appropriate place for this.

Could you add the command to package.json, in the commands section? There is a variable replacement pattern you'll see used there (i.e. %c_cpp.command.configurationSelect.title%). Those refer to entries in package.nls.json. Could you add a new entry there? Only an English entry is needed. Other language files will be automatically generated by our Localization team.

That would be for the title. Our docs are here, but I'm not sure if we have verbose doc for commands. (@sean-mcmanus Do we have doc for commands?)

@sean-mcmanus
Copy link
Collaborator

I can also update the documentation but need help to figure out an appropriate place for this.

Could you add the command to package.json, in the commands section? There is a variable replacement pattern you'll see used there (i.e. %c_cpp.command.configurationSelect.title%). Those refer to entries in package.nls.json. Could you add a new entry there? Only an English entry is needed. Other language files will be automatically generated by our Localization team.
That would be for the title. Our docs are here, but I'm not sure if we have verbose doc for commands. (@sean-mcmanus Do we have doc for commands?)

We never added any docs for the "cpptools.activeConfigName" command either. Maybe we could just add an issue to track that.

@aleksey-sergey
Copy link
Contributor Author

I can also update the documentation but need help to figure out an appropriate place for this.

Could you add the command to package.json, in the commands section? There is a variable replacement pattern you'll see used there (i.e. %c_cpp.command.configurationSelect.title%). Those refer to entries in package.nls.json. Could you add a new entry there? Only an English entry is needed. Other language files will be automatically generated by our Localization team.

That would be for the title. Our docs are here, but I'm not sure if we have verbose doc for commands. (@sean-mcmanus Do we have doc for commands?)

@Colengms , I encounted these problems while trying to implement your suggestion:

  • c_cpp.command.configurationSelect.title already exists and is used to choose a configuration via UI dropdown. Adding another 'configuration set/select' command might confuse users of cpptools extension.
  • I tried to add c_cpp.command.configurationSelectByName.title... Unluckily didn't find a way to inform vscode that command requires user to provide an argument. As result, attempt to execute
    new command fails with "The requested configuration not found: undefined" message.

After some thinking can suggest these alternatives:

  • Accept current implementation. cpptools.activeConfigName and cpptools.setActiveConfigName stay outside of commands in package.json. Documentation will be created via a separate issue as suggested by @sean-mcmanus

  • instead of introducing new command, we might try to modify behavior of already existing onSelectConfiguration(...). Function might accept an optional parameter (e.g. configurationName) and show the UI dropdown only if parameter is empty.
    Cons: existing onSelectConfiguration(...) doesn't return a Promise; refactoring is necessary to make things working properly. Implementation would differ much from other UI-related onSomething handlers.

  • make cpptools.activeConfigName and cpptools.setActiveConfigName part of vscode-cpptools-api . Since the goal of PR is to allow other extensions to control currentConfigName, public API might be a good candidate.

@sean-mcmanus
Copy link
Collaborator

I can also update the documentation but need help to figure out an appropriate place for this.

Could you add the command to package.json, in the commands section? There is a variable replacement pattern you'll see used there (i.e. %c_cpp.command.configurationSelect.title%). Those refer to entries in package.nls.json. Could you add a new entry there? Only an English entry is needed. Other language files will be automatically generated by our Localization team.
That would be for the title. Our docs are here, but I'm not sure if we have verbose doc for commands. (@sean-mcmanus Do we have doc for commands?)

I don't think package.json needs to be changed to add the command there -- those are just for commands that appear in the command prompt or UI.

@sean-mcmanus sean-mcmanus merged commit 2aeb48d into microsoft:master Jan 30, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 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.

None yet

5 participants