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.

Sign up for GitHub

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
Merged

Make cache editing more robust against corner cases #1826

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
abcc2b0
Make cache editing more robust against corner cases
May 2, 2021
d22bb2d
Merge branch 'develop' into cache-editor-robustness
andreeis Jul 13, 2021
108f087
Fix linter errors.
andreeis Jul 13, 2021
c0fb782
Merge branch 'develop' into cache-editor-robustness
andreeis Jul 13, 2021
999215d
Merge branch 'develop' into cache-editor-robustness
andreeis Jul 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ export interface CacheEntryProperties {
value: any;
/** Whether this entry is ADVANCED, meaning it hidden from the user. */
advanced: boolean;
/** List of allowed values, as specified by STRINGS property */
choices: string[];
}

/**
Expand Down
96 changes: 70 additions & 26 deletions src/cache-view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ const localize: nls.LocalizeFunc = nls.loadMessageBundle();
export interface IOption {
key: string; // same as CMake cache variable key names
type: string; // "Bool" for boolean and "String" for anything else for now
helpString: string;
choices: string[];
value: string; // value from the cache file or changed in the UI
dirty: boolean; // if the variable was edited in the UI
}
Expand Down Expand Up @@ -225,7 +227,7 @@ export class ConfigurationWebview {
// Static cache entries are set automatically by CMake, overriding any value set by the user in this view.
// Not useful to show these entries in the list.
if (entry.type !== api.CacheEntryType.Static) {
options.push({ key: entry.key, type: (entry.type === api.CacheEntryType.Bool) ? "Bool" : "String", value: entry.value, dirty: false });
options.push({ key: entry.key, helpString: entry.helpString, choices: entry.choices, type: (entry.type === api.CacheEntryType.Bool) ? "Bool" : "String", value: entry.value, dirty: false });
}
}

Expand Down Expand Up @@ -289,6 +291,10 @@ export class ConfigurationWebview {
border: 1px solid var(--vscode-settings-textInputBorder);
}

.invalid-selection {
background-color: #4e2621;
}

.vscode-light .input-disabled {
background-color:rgba(255, 255, 255, 0.4);
color: rgb(138, 138, 138);
Expand Down Expand Up @@ -421,16 +427,30 @@ export class ConfigurationWebview {
</style>
<script>
const vscode = acquireVsCodeApi();
function toggleKey(id) {
const label = document.getElementById('LABEL_' + id);
label.textContent = label.textContent === 'ON' ? 'OFF' : 'ON';
const checkbox = document.getElementById(id);
vscode.postMessage({key: id, type: "Bool", value: checkbox.checked});
function updateCheckboxState(checkbox) {
checkbox.labels.forEach(label => label.textContent = checkbox.checked ? 'ON' : 'OFF');
}
function toggleKey(checkbox) {
updateCheckboxState(checkbox);
vscode.postMessage({key: checkbox.id, type: "Bool", value: checkbox.checked});
document.getElementById('not-saved').classList.remove('invisible');
}
function edit(id) {
const editbox = document.getElementById(id);
vscode.postMessage({key: id, type: "String", value: editbox.value});
function validateInput(editbox) {
const list = editbox.list;
if (list) {
let found = false;
for (const opt of list.options) {
if (opt.value === editbox.value) {
found = true;
break;
}
}
editbox.classList.toggle('invalid-selection', !found);
}
}
function edit(editbox) {
validateInput(editbox);
vscode.postMessage({key: editbox.id, type: "String", value: editbox.value});
document.getElementById('not-saved').classList.remove('invisible');
}
function save() {
Expand All @@ -447,6 +467,17 @@ export class ConfigurationWebview {
}
}
}

window.onload = function() {
document.querySelectorAll('.cmake-input-bool').forEach(checkbox => {
updateCheckboxState(checkbox);
checkbox.onclick = () => toggleKey(checkbox);
});
document.querySelectorAll('.cmake-input-text').forEach(editbox => {
validateInput(editbox)
editbox.oninput = () => edit(editbox);
});
}
</script>
</head>
<body>
Expand All @@ -468,26 +499,39 @@ export class ConfigurationWebview {

// compile a list of table rows that contain the key and value pairs
const tableRows = this._options.map(option => {

// HTML attributes may not contain literal double quotes or ambiguous ampersands
const escapeAttribute = (text: string) => text.replace(/&/g, "&amp;").replace(/"/g, "&quot;");
// Escape HTML special characters that may not occur literally in any text
const escapeHtml = (text: string) =>
escapeAttribute(text)
.replace(/</g, "&lt;")
.replace(/>/g, "&gt;")
.replace(/'/g, "&#039;")
.replace(/ /g, "&nbsp;"); // we are usually dealing with single line entities - avoid unintential line breaks

const id = escapeAttribute(option.key);
let editControls = '';

if (option.type === "Bool") {
return `<tr class="content-tr">
<td></td>
<td>${option.key}</td>
<td>
<input class="cmake-input-bool" id="${option.key}" onclick="toggleKey('${option.key}')"
type="checkbox" ${util.isTruthy(option.value) ? 'checked' : ''}>
<label id="LABEL_${option.key}" for="${option.key}">${util.isTruthy(option.value) ? `ON` : `OFF`}</label>
</td>
</tr>`;
editControls = `<input class="cmake-input-bool" id="${id}" type="checkbox" ${util.isTruthy(option.value) ? 'checked' : ''}>
<label id="LABEL_${id}" for="${id}"/>`;
} else {
return `<tr class="content-tr">
<td></td>
<td>${option.key}</td>
<td>
<input id="${option.key}" value="${option.value}" style="width: 90%;"
type="text" oninput="edit('${option.key}')">
</td>
</tr>`;
const hasChoices = option.choices.length > 0;
if (hasChoices) {
editControls = `<datalist id="CHOICES_${id}">
${option.choices.map(ch => `<option value="${escapeAttribute(ch)}">`).join()}
</datalist>`;
}
editControls += `<input class="cmake-input-text" id="${id}" value="${escapeAttribute(option.value)}" style="width: 90%;"
type="text" ${hasChoices ? `list="CHOICES_${id}"` : ''}>`;
}

return `<tr class="content-tr">
<td></td>
<td title="${escapeAttribute(option.helpString)}">${escapeHtml(option.key)}</td>
<td>${editControls}</td>
</tr>`;
});

html = html.replace(key, tableRows.join(""));
Expand Down
90 changes: 67 additions & 23 deletions src/cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {fs} from './pr';
import rollbar from './rollbar';
import * as util from './util';
import * as nls from 'vscode-nls';
import { isBoolean } from 'util';
import {isBoolean, isString} from 'util';

nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })();
const localize: nls.LocalizeFunc = nls.loadMessageBundle();
Expand All @@ -17,14 +17,17 @@ const log = logging.createLogger('cache');

/**
* Implements access to CMake cache entries. See `api.CacheEntry` for more
* information. This type is immutable.
* information.
*/
export class Entry implements api.CacheEntry {
private readonly _type: api.CacheEntryType = api.CacheEntryType.Uninitialized;
private readonly _docs: string = '';
private readonly _key: string = '';
private readonly _value: any = null;
private readonly _advanced: boolean = false;
advanced: boolean = false;
choices: string[] = [];

serializedKey: string = '';

get type() {
return this._type;
Expand All @@ -46,10 +49,6 @@ export class Entry implements api.CacheEntry {
return this.value as T;
}

get advanced() {
return this._advanced;
}

/**
* Create a new Cache Entry instance. Doesn't modify any files. You probably
* want to get these from the `CMakeCache` object instead.
Expand All @@ -61,14 +60,15 @@ export class Entry implements api.CacheEntry {
*/
constructor(key: string, value: string, type: api.CacheEntryType, docs: string, advanced: boolean) {
this._key = key;
this.serializedKey = key; // may be overwritten later with quoted version of `key`
this._type = type;
if (type === api.CacheEntryType.Bool) {
this._value = util.isTruthy(value);
} else {
this._value = value;
}
this._docs = docs;
this._advanced = advanced;
this.advanced = advanced;
}
}

Expand Down Expand Up @@ -153,6 +153,8 @@ export class CMakeCache {

const entries = new Map<string, Entry>();
let docs_acc = '';
const advancedNames: string[] = [];
const choices: Map<string, string[]> = new Map();
for (const line of lines) {
if (line.startsWith('//')) {
docs_acc += /^\/\/(.*)/.exec(line)![1] + ' ';
Expand All @@ -162,15 +164,21 @@ export class CMakeCache {
rollbar.error(localize('failed.to.read.line.from.cmake.cache.file', 'Failed to read a line from a CMake cache file {0}', line));
continue;
}
const [, , quoted_name, unquoted_name, typename, valuestr] = match;
const [, serializedName, quoted_name, unquoted_name, typename, valuestr] = match;
const name = quoted_name || unquoted_name;
if (!name || !typename) {
continue;
}
log.trace(localize('read.line.in.cache', 'Read line in cache with {0}={1}, {2}={3}, {4}={5}', 'name', name, 'typename', typename, 'valuestr', valuestr));
if (name.endsWith('-ADVANCED') && valuestr === '1') {
log.trace(localize('skipping.variable', 'Skipping {0} variable', '*-ADVANCED'));
// We skip the ADVANCED property variables. They're a little odd.
if (name.endsWith('-ADVANCED')) {
if (valuestr === '1') {
const entryName = name.substr(0, name.lastIndexOf('-'));
advancedNames.push(entryName);
}
} else if (name.endsWith('-MODIFIED')) {
// ignore irrelevant entry property
} else if (name.endsWith('-STRINGS')) {
choices.set(name.substr(0, name.lastIndexOf('-')), valuestr.split(';'));
} else {
const key = name;
const typemap = {
Expand All @@ -189,12 +197,33 @@ export class CMakeCache {
rollbar.error(localize('cache.entry.unknown', 'Cache entry \'{0}\' has unknown type: \'{1}\'', name, typename));
} else {
log.trace(localize('constructing.new.cache.entry', 'Constructing a new cache entry from the given line'));
entries.set(name, new Entry(key, valuestr, type, docs, false));
const entry = new Entry(key, valuestr, type, docs, false);
entry.serializedKey = serializedName;
entries.set(name, entry);
}
}
}
}

// Update `advanced` attribute
advancedNames.forEach(name => {
const entry = entries.get(name);
if (entry) {
entry.advanced = true;
} else {
log.warning(localize('nonexisting.advanced.entry', 'Nonexisting cache entry \'{0}\' marked as advanced', name));
}
});
// update `choices`
choices.forEach((list, name) => {
const entry = entries.get(name);
if (entry) {
entry.choices = list;
} else {
log.warning(localize('ignore.strings.for.nonexisting.entry', 'Ignoring `STRINGS` property for nonexisting cache entry \'{0}\'', name));
}
});

log.trace(localize('parsed.cache.entries', 'Parsed {0} cache entries', entries.size));
return entries;
}
Expand All @@ -206,17 +235,32 @@ export class CMakeCache {
* @param value Boolean value
*/
private replace(content: string, key: string, value: string): string {
const re = key + ':[^=]+=(.+)';
const found = content.match(re);

if (found && found.length >= 2) {
const line = found[0];
const currentVal = found[1];
const newValueLine = line.replace(currentVal, isBoolean(value) ? (value ? "TRUE" : "FALSE") : value);
return content.replace(line, newValueLine);
} else {
return content;

const entry = this._entries.get(key);
if (entry !== undefined) {
// cmake variable name may contain characters with special meanings in regex
const escapedKey = entry.serializedKey.replace(/[^A-Za-z0-9_]/g, '\\$&');
const re = RegExp(`^${escapedKey}(:[^=]+=)(.+)`, 'm');
const found = content.match(re);

if (found && found.length >= 3) {
const line = found[0];
const type = found[1];

// FIXME: How can `value` be boolean desipte being marked as string in the signature?

if (isString(value)) {
const newlineIndex = value.search(/[\r\n]/);
if (newlineIndex >= 0) {
value = value.substring(0, newlineIndex);
log.warning(localize('cache.value.truncation.warning', 'Newline(s) found in cache entry \'{0}\'. Value has been truncated to \'{1}\'', key, value));
}
}
const newValueLine = entry.serializedKey + type + (isBoolean(value) ? (value ? "TRUE" : "FALSE") : value);
return content.replace(line, newValueLine);
}
}
return content;
}

/**
Expand Down