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

Make cache editing more robust against corner cases #1826

Merged
merged 5 commits into from
Jul 19, 2021

Conversation

aemseemann
Copy link
Contributor

The cache editor UI exhibits erroneous behavior in several corner cases:

  • If the value of the a cache variable occurs somewhere in its name (like ON at the end of SOME_OPTI*ON*), that part of the name is replaced with the new value (resulting in SOME_OPTIFALSE in the previous example).
  • In case a variable name ends with the name of another cache variable (e.g. ABC_SOME_OPTION and SOME_OPTION), the editor may modify the prefixed entry when the user changed the unprefixed variable (depending on order of variables within CMakeCache.txt).
  • The extension can read cache variables names stored in quoted form in CMakeCache.txt (necessary when the variable name contains a colon), but it cannot change them via the editor UI, because it always looks for the unquoted form in CMakeCache.txt.
  • The cache editor UI displays ...-STRINGS entries as editable cache variables, but these are just properties of other variables and therefore should be excluded.

Furthermore, there is no proper escaping of variable names and values when building the HTML markup for the editor webview, which can mess up the GUI.

This PR attempts to fix all of the above issues and also uses the -STRINGS properties to add dropdowns to the editor UI. If you prefer, I can factor out this part into another PR.
I'm not completely happy with it anyway, because with the currently used <datalist> approach, the dropdown list is filtered by the text in the input field and thus all choices except for the currently selected value are usually hidden. To see the full list of choices, one has to clear the text input first. Unfortunately, I was unable to find a way to realize a plain old combo box with HTML/CSS, but was also reluctant to pull in a 3rd party Javascript library for that seemingly simple task. If you have any suggestions, I'd be happy to try it.

@ghost
Copy link

ghost commented May 2, 2021

CLA assistant check
All CLA requirements met.

@andreeis
Copy link
Contributor

andreeis commented May 3, 2021

@aemseemann, thank you for this contribution. We will review and test once we are done with the CMake Tools 1.7.2 patch release. Very good findings, thank you.

@andreeis
Copy link
Contributor

andreeis commented May 3, 2021

@aemseemann, do you think this PR could address the symptoms described in #1591?

@aemseemann
Copy link
Contributor Author

@aemseemann, do you think this PR could address the symptoms described in #1591?

Cannot rule it out, but it is quite unlikely. I'll comment over there.

@bobbrow bobbrow added this to the 1.8.0 milestone May 4, 2021
@bobbrow bobbrow added this to Pull Requests in 1.8.0 May 24, 2021
@andreeis
Copy link
Contributor

@aemseemann, I reviewed and tested these scenarios and I am approving this PR. I committed some linter fixes in your branch. I will also merge it as soon as the tests complete.
I want to let you know that for the third bullet with quotes, I did see how before the PR the cache editor was not able to update a quoted variable and after the PR it does correctly. But even with the PR, once that cache variable is saved, it loses its quotes. I can investigate and make an additional fix. Let me know if you don't think this is a concern or if you'd like to follow up with an additional small fix. I will merge this PR without that to avoid more integration work and since all works fine.

@andreeis
Copy link
Contributor

@aemseemann, you need to sign the Contributor License Agreement in order for us to merge this PR.

@aemseemann
Copy link
Contributor Author

Thanks for reviewing the changes and sorry for abandoning this PR for several weeks. I didn't find the time to familiarize myself with the CLA and the general concept behind it.

Regarding the quoting issue: The behavior should be fine as it is now. I guess you tried it with a variable name that does not require quoting. In this case, cmake removes the quotes the next time it reconfigures the project (which is triggered by the cache editor). If you try it with a variable name containing a literal colon (:) or starting with //, cmake will keep the entry quoted (see also https://gitlab.kitware.com/cmake/cmake/-/blob/master/Source/cmCacheManager.cxx#L374)

@andreeis andreeis merged commit e64ccc8 into microsoft:develop Jul 19, 2021
@elahehrashedi elahehrashedi moved this from Pull Requests to Done in 1.8.0 Jul 22, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
1.8.0
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants