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 dotconfig property for c_cpp_properties.json #7845

Merged
merged 8 commits into from
Feb 15, 2022
Merged

Add dotconfig property for c_cpp_properties.json #7845

merged 8 commits into from
Feb 15, 2022

Conversation

microhobby
Copy link
Contributor

Some bare metal systems, RTOS and Linux use a generated .config file
with defines needed to build the system and switch features. This patch
add the dotconfig property that will store the path for read a
.config file and will concat the defines from file to the extension
defines.

Signed-off-by: Matheus Castello matheus@castello.eng.br

@microhobby
Copy link
Contributor Author

I thinking out loud: #7840

@sean-mcmanus
Copy link
Collaborator

@microhobby Is this ready for code review?

Does anyone know what "This branch is out-of-date with the base branch" means?

@microhobby
Copy link
Contributor Author

@sean-mcmanus I will rebase it with the base branch and remove the WIP from the title, so i think it's good for the review. Open to suggestions from maintainers.

Extension/c_cpp_properties.schema.json Outdated Show resolved Hide resolved
Extension/src/LanguageServer/configurations.ts Outdated Show resolved Hide resolved
@microhobby
Copy link
Contributor Author

@bobbrow @sean-mcmanus and @Colengms thanks for the review, I will address the comments and as soon as I send the changes commits I will remove the WIP

@microhobby microhobby changed the title WIP: Add dotconfig property for c_cpp_properties.json Add dotconfig property for c_cpp_properties.json Jul 30, 2021
Extension/c_cpp_properties.schema.json Outdated Show resolved Hide resolved
Extension/c_cpp_properties.schema.json Outdated Show resolved Hide resolved
Extension/c_cpp_properties.schema.json Outdated Show resolved Hide resolved
Extension/c_cpp_properties.schema.json Outdated Show resolved Hide resolved
@Colengms Colengms dismissed their stale review August 6, 2021 02:28

resetting my reviewed status

Extension/ui/settings.ts Outdated Show resolved Hide resolved
@sean-mcmanus
Copy link
Collaborator

@microhobby FYI, 1.6.0 is locked down for shipping next week, but this can go into 1.7.0.

@sean-mcmanus

This comment has been minimized.

Copy link
Collaborator

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

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

It seems like the package.json is missing the "default.dotConfig" setting and the package.nls.json is missing a description for that. I have a bunch of settings added in my branch at https://github.com/microsoft/vscode-cpptools/pull/7816/files if you want an example, but you don't need to use markdownDescription since that is only used in that branch and I can update it later.

@microhobby
Copy link
Contributor Author

It seems like the package.json is missing the "default.dotConfig" setting and the package.nls.json is missing a description for that. I have a bunch of settings added in my branch at https://github.com/microsoft/vscode-cpptools/pull/7816/files if you want an example, but you don't need to use markdownDescription since that is only used in that branch and I can update it later.

@sean-mcmanus thanks for the examples, I add a commit adding it to the package and package.nls.json. Let me know if this is ok.

@sean-mcmanus
Copy link
Collaborator

@Colengms @michelleangela @bobbrow Does someone else want to review this too?

Copy link
Contributor

@michelleangela michelleangela left a comment

Choose a reason for hiding this comment

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

This property will also need to be added in the .html file of the IntelliSense Configuration UI.

See https://github.com/microsoft/vscode-cpptools/blob/main/Extension/ui/settings.html and the UI element for "Compile commands" property for example of creating a field for one file path entry.

Place UI element for dotConfig after "Forced include" in the UI. Use title "Dot Config" for the new property.

Also add this new property to https://github.com/microsoft/vscode-cpptools/blob/main/Extension/src/LanguageServer/settingsPanel.ts so that the UI will be updated with values from c_cpp_properties.json

See examples for elementId.compileCommands in settingsPanel.ts.

compileCommands: "compileCommands",

case elementId.compileCommands:

@michelleangela
Copy link
Contributor

@microhobby
Here is how strings are localized in HTML.

To localize HTML content, the following 2 attributes are used on HTML nodes:

data-loc-id
Specifying a unique string key value using this attribute causes the content of the node to be localized associated with that key

data-loc-id-<attribute-name>
Specifying a unique string key value using this attribute causes the specified attribute of this node to be localized. For example, to localize the title attribute of a node, use: data-loc-id-title="some.title.id"

The following is an example of a span with localized content, containing a sub-node with a localized title attribute.

<span data-loc-id="switch.to.json">Switch to the <a href="command:C_Cpp.ConfigurationEditJSON" title="Edit configurations in JSON file" data-loc-id-title="edit.configurations.in.json">c_cpp_properties.json</a> file by clicking on the file link or using the command:</span>

Child nodes are not included when content of a parent node is localized. In the outer nodes of the above example, the following string will be extracted for localization:

"Switch to the {0} file by clicking on the file link or using the command:"

@microhobby
Copy link
Contributor Author

@michelleangela thanks for the review, I will work on it.

@sean-mcmanus
Copy link
Collaborator

FYI, we've changed our settings strings to prefer using markdownDescription, with a hint string -- see #8129 for examples, so for this change you'd use backticks around .config and replace the "" with ``.

@microhobby
Copy link
Contributor Author

@sean-mcmanus @michelleangela sorry for the delay, I had a few shifts in priorities these past few months, but I'm still on it. I will rebase it and address the proposed changes. Thanks.

@microhobby
Copy link
Contributor Author

@sean-mcmanus @michelleangela first of all sorry for the delay, I rebased it and addressed the proposed changes.
@michelleangela let me know if the html UI changes looks good to you, or if I missed something.

@sean-mcmanus
Copy link
Collaborator

@microhobby FYI, we stopped publishing downloadable binaries so you may need to copy over updated ones (see #8691 and #8788).

@sean-mcmanus
Copy link
Collaborator

It says "This branch is out-of-date with the base branch" -- can that be updated?

@microhobby
Copy link
Contributor Author

It says "This branch is out-of-date with the base branch" -- can that be updated?

@sean-mcmanus done!

@michelleangela
Copy link
Contributor

@microhobby
I tested the Configuration UI changes. The input field works and updates the JSON file and vice versa. It is only missing the error message on the UI when the path file is invalid. The UI element for the error message is already there in the `settings.html' file but the string for the error message is missing.

Note that the error message for invalid paths on JSON file works.

Example of expected error on UI:
image

Where to get error message for the UI and updating errors on UI:

  1. UI error messages are created in function private getErrorsForConfigUI(configIndex: number) (https://github.com/microsoft/vscode-cpptools/blob/main/Extension/src/LanguageServer/configurations.ts#L1365)
  2. In getErrorsForConfigUI add something like errors.dotConfig = this.validatePath(config.dotConfig, false); similar to errors.compileCommands = this.validatePath(config.compileCommands, false);
  3. Another string property will need to be added to interface ConfigurationErrors for dotConfig errors.
  4. Errors on the UI are updated via the private updateErrors(errors: any) function in https://github.com/microsoft/vscode-cpptools/blob/main/Extension/ui/settings.ts. See examples this.showErrorWithInfo(elementId.compileCommandsInvalid, errors.compileCommands); and compileCommandsInvalid: "compileCommandsInvalid", for adding another error message to update.

@microhobby
Copy link
Contributor Author

microhobby commented Feb 12, 2022

@michelleangela thank you very much for the valuable tips, I push a new commit, tested here and now the error message show as expected.

@michelleangela michelleangela added this to the 1.9.1 (insiders2) milestone Feb 14, 2022
@michelleangela
Copy link
Contributor

@microhobby Could you update your branch again so it is up-to-date with the base branch? Then I'll re-run the checks and check-in the PR.

Kconfig system generate a .config file with all the defines, CONFIG
options selected, to build a project. Examples of projects that use
Kconfig system is Linux Kernel and NuttX RTOS. This patch add the
`dotConfig` property that will store the path for read a .config file
and will concat the defines from file to the extension defines.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
Validate if the path exists and if the path is a valida file. For these
add a diagnostic warning to show a squiggle during the file edition.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
Signed-off-by: Matheus Castello <matheus@castello.eng.br>
Signed-off-by: Matheus Castello <matheus@castello.eng.br>
Signed-off-by: Matheus Castello <matheus@castello.eng.br>
Signed-off-by: Matheus Castello <matheus@castello.eng.br>
Signed-off-by: Matheus Castello <matheus@castello.eng.br>
Signed-off-by: Matheus Castello <matheus@castello.eng.br>
@microhobby
Copy link
Contributor Author

@michelleangela I rebased it with the main

@michelleangela michelleangela merged commit 67eea35 into microsoft:main Feb 15, 2022
@michelleangela
Copy link
Contributor

@microhobby
Your changes to add dotConfig property has been checked-in. Thank you for your contribution to the extension!

@github-actions github-actions bot locked and limited conversation to collaborators Apr 2, 2022
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.

5 participants