From ec6cdfd6ba28c16a04c30df2ffdd00b9632f34c6 Mon Sep 17 00:00:00 2001 From: BeniBenj Date: Mon, 24 Jun 2024 15:17:45 +0200 Subject: [PATCH 1/4] Allow multiple extensions to provide default values for object settings --- .../common/configurationRegistry.ts | 103 ++++++++-- .../test/common/configurationRegistry.test.ts | 32 ++- .../browser/workbench.contribution.ts | 2 +- .../settingsEditorSettingIndicators.ts | 50 ++++- .../preferences/browser/settingsTree.ts | 157 +++++++++------ .../preferences/browser/settingsTreeModels.ts | 24 ++- .../preferences/browser/settingsWidgets.ts | 190 ++++++++++++------ .../preferences/common/preferences.ts | 4 +- .../preferences/common/preferencesModels.ts | 4 +- 9 files changed, 413 insertions(+), 153 deletions(-) diff --git a/src/vs/platform/configuration/common/configurationRegistry.ts b/src/vs/platform/configuration/common/configurationRegistry.ts index ed8e56d50c5a9..4dd83106eeab2 100644 --- a/src/vs/platform/configuration/common/configurationRegistry.ts +++ b/src/vs/platform/configuration/common/configurationRegistry.ts @@ -233,21 +233,24 @@ export interface IConfigurationNode { restrictedProperties?: string[]; } +export type ConfigurationDefaultSource = IExtensionInfo | string; +export type ConfigurationDefaultValueSource = ConfigurationDefaultSource | Map; + export interface IConfigurationDefaults { overrides: IStringDictionary; - source?: IExtensionInfo | string; + source?: ConfigurationDefaultSource; } export type IRegisteredConfigurationPropertySchema = IConfigurationPropertySchema & { defaultDefaultValue?: any; source?: IExtensionInfo; // Source of the Property - defaultValueSource?: IExtensionInfo | string; // Source of the Default Value + defaultValueSource?: ConfigurationDefaultValueSource; // Source of the Default Value }; export type IConfigurationDefaultOverride = { readonly value: any; - readonly source?: IExtensionInfo | string; // Source of the default override - readonly valuesSources?: Map; // Source of each value in default language overrides + readonly source?: ConfigurationDefaultValueSource; // Source of the default override + readonly valuesSources?: Map; // Source of each value in default language overrides }; export const allSettings: { properties: IStringDictionary; patternProperties: IStringDictionary } = { properties: {}, patternProperties: {} }; @@ -351,13 +354,38 @@ class ConfigurationRegistry implements IConfigurationRegistry { if (OVERRIDE_PROPERTY_REGEX.test(key)) { const configurationDefaultOverride = this.configurationDefaultsOverrides.get(key); - const valuesSources = configurationDefaultOverride?.valuesSources ?? new Map(); - if (source) { - for (const configuration of Object.keys(overrides[key])) { - valuesSources.set(configuration, source); + const valuesSources = configurationDefaultOverride?.valuesSources ?? new Map(); + + const defaultValue = configurationDefaultOverride?.value || {}; + for (const configuration of Object.keys(overrides[key])) { + const overrideValue = overrides[key][configuration]; + + const isObjectSetting = types.isObject(overrideValue) && (types.isUndefined(defaultValue[configuration] || types.isObject(defaultValue[configuration]))); + if (isObjectSetting) { + // Objects are merged instead of overridden + defaultValue[configuration] = { ...(defaultValue[configuration] ?? {}), ...overrideValue }; + + // Track the source of each value in the object + if (source) { + let objectConfigurationSources = valuesSources.get(configuration) as Map | undefined; + if (!objectConfigurationSources) { + objectConfigurationSources = new Map(); + valuesSources.set(configuration, objectConfigurationSources); + } + + for (const objectKey in overrideValue) { + objectConfigurationSources.set(objectKey, source); + } + } + } else { + // Primitive values are overridden + defaultValue[configuration] = overrideValue; + if (source) { + valuesSources.set(configuration, source); + } } } - const defaultValue = { ...(configurationDefaultOverride?.value || {}), ...overrides[key] }; + this.configurationDefaultsOverrides.set(key, { source, value: defaultValue, valuesSources }); const plainKey = getLanguageTagSettingPlainKey(key); const property: IRegisteredConfigurationPropertySchema = { @@ -373,8 +401,26 @@ class ConfigurationRegistry implements IConfigurationRegistry { this.configurationProperties[key] = property; this.defaultLanguageConfigurationOverridesNode.properties![key] = property; } else { - this.configurationDefaultsOverrides.set(key, { value: overrides[key], source }); const property = this.configurationProperties[key]; + let defaultValue = overrides[key]; + let defaultValueSource: ConfigurationDefaultValueSource | undefined = source; + + // If the default value is an object, merge the objects and store the source of each keys + if (property.type === 'object' && types.isObject(overrides[key])) { + const objectDefaults = this.configurationDefaultsOverrides.get(key); + const existingDefaultValue = objectDefaults?.value ?? property.defaultDefaultValue ?? {}; + defaultValue = { ...existingDefaultValue, ...overrides[key] }; + + if (source) { + defaultValueSource = objectDefaults?.source as Map ?? new Map(); + for (const objectKey in overrides[key]) { + defaultValueSource.set(objectKey, source); + } + } + } + + this.configurationDefaultsOverrides.set(key, { value: defaultValue, source: defaultValueSource }); + if (property) { this.updatePropertyDefaultValue(key, property); this.updateSchema(key, property); @@ -397,14 +443,43 @@ class ConfigurationRegistry implements IConfigurationRegistry { for (const { overrides, source } of defaultConfigurations) { for (const key in overrides) { - const configurationDefaultsOverride = this.configurationDefaultsOverrides.get(key); const id = types.isString(source) ? source : source?.id; - const configurationDefaultsOverrideSourceId = types.isString(configurationDefaultsOverride?.source) ? configurationDefaultsOverride?.source : configurationDefaultsOverride?.source?.id; - if (id !== configurationDefaultsOverrideSourceId) { + + const configurationDefaultsOverride = this.configurationDefaultsOverrides.get(key); + if (!configurationDefaultsOverride || !configurationDefaultsOverride.source) { continue; } + + // If the default value is an object, remove the source of each key + if (configurationDefaultsOverride.source instanceof Map) { + + const keySources = configurationDefaultsOverride.source; + for (const objectKey in overrides[key]) { + const keySource = keySources.get(objectKey); + const keySourceId = types.isString(keySource) ? keySource : keySource?.id; + + if (keySourceId === id) { + keySources.delete(objectKey); + delete configurationDefaultsOverride.value[objectKey]; + } + } + + if (keySources.size === 0) { + this.configurationDefaultsOverrides.delete(key); + } + } + // Otherwise, remove the default value if the source matches + else { + const configurationDefaultsOverrideSourceId = types.isString(configurationDefaultsOverride.source) ? configurationDefaultsOverride.source : configurationDefaultsOverride.source.id; + if (id !== configurationDefaultsOverrideSourceId) { + continue; // Another source is overriding this default value + } + + this.configurationDefaultsOverrides.delete(key); + } + bucket.add(key); - this.configurationDefaultsOverrides.delete(key); + if (OVERRIDE_PROPERTY_REGEX.test(key)) { delete this.configurationProperties[key]; delete this.defaultLanguageConfigurationOverridesNode.properties![key]; diff --git a/src/vs/platform/configuration/test/common/configurationRegistry.test.ts b/src/vs/platform/configuration/test/common/configurationRegistry.test.ts index e2cf8972a2c33..24dc735cd11be 100644 --- a/src/vs/platform/configuration/test/common/configurationRegistry.test.ts +++ b/src/vs/platform/configuration/test/common/configurationRegistry.test.ts @@ -38,7 +38,7 @@ suite('ConfigurationRegistry', () => { assert.deepStrictEqual(configurationRegistry.getConfigurationProperties()['[lang]'].default, { a: 2, b: 2, c: 3 }); }); - test('configuration defaults - overrides defaults', async () => { + test('configuration defaults - merge object default overrides', async () => { configurationRegistry.registerConfiguration({ 'id': '_test_default', 'type': 'object', @@ -51,7 +51,7 @@ suite('ConfigurationRegistry', () => { configurationRegistry.registerDefaultConfigurations([{ overrides: { 'config': { a: 1, b: 2 } } }]); configurationRegistry.registerDefaultConfigurations([{ overrides: { 'config': { a: 2, c: 3 } } }]); - assert.deepStrictEqual(configurationRegistry.getConfigurationProperties()['config'].default, { a: 2, c: 3 }); + assert.deepStrictEqual(configurationRegistry.getConfigurationProperties()['config'].default, { a: 2, b: 2, c: 3 }); }); test('registering multiple settings with same policy', async () => { @@ -79,4 +79,32 @@ suite('ConfigurationRegistry', () => { assert.ok(actual['policy1'] !== undefined); assert.ok(actual['policy2'] === undefined); }); + + test('configuration defaults - deregister merged object default override', async () => { + configurationRegistry.registerConfiguration({ + 'id': '_test_default', + 'type': 'object', + 'properties': { + 'config': { + 'type': 'object', + } + } + }); + + const overrides1 = [{ overrides: { 'config': { a: 1, b: 2 } }, source: 'source1' }]; + const overrides2 = [{ overrides: { 'config': { a: 2, c: 3 } }, source: 'source2' }]; + + configurationRegistry.registerDefaultConfigurations(overrides1); + configurationRegistry.registerDefaultConfigurations(overrides2); + + assert.deepStrictEqual(configurationRegistry.getConfigurationProperties()['config'].default, { a: 2, b: 2, c: 3 }); + + configurationRegistry.deregisterDefaultConfigurations(overrides2); + + assert.deepStrictEqual(configurationRegistry.getConfigurationProperties()['config'].default, { b: 2 }); // TODO this should actualy equal overrides1 + + configurationRegistry.deregisterDefaultConfigurations(overrides1); + + assert.deepStrictEqual(configurationRegistry.getConfigurationProperties()['config'].default, {}); + }); }); diff --git a/src/vs/workbench/browser/workbench.contribution.ts b/src/vs/workbench/browser/workbench.contribution.ts index 8494c963f3fa2..e017dcd997fb2 100644 --- a/src/vs/workbench/browser/workbench.contribution.ts +++ b/src/vs/workbench/browser/workbench.contribution.ts @@ -112,7 +112,7 @@ const registry = Registry.as(ConfigurationExtensions.Con })(), additionalProperties: { - type: 'string', + type: ['string', 'null'], markdownDescription: localize('workbench.editor.label.template', "The template which should be rendered when the pattern mtches. May include the variables ${dirname}, ${filename} and ${extname}."), minLength: 1, pattern: '.*[a-zA-Z0-9].*' diff --git a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts index 6db2e7883d966..59b31491a53ae 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts @@ -9,7 +9,7 @@ import { HoverPosition } from 'vs/base/browser/ui/hover/hoverWidget'; import { SimpleIconLabel } from 'vs/base/browser/ui/iconLabel/simpleIconLabel'; import { RunOnceScheduler } from 'vs/base/common/async'; import { Emitter } from 'vs/base/common/event'; -import { IMarkdownString } from 'vs/base/common/htmlContent'; +import { IMarkdownString, MarkdownString } from 'vs/base/common/htmlContent'; import { KeyCode } from 'vs/base/common/keyCodes'; import { IDisposable, DisposableStore } from 'vs/base/common/lifecycle'; import { ILanguageService } from 'vs/editor/common/languages/language'; @@ -448,15 +448,27 @@ export class SettingsTreeIndicatorsLabel implements IDisposable { updateDefaultOverrideIndicator(element: SettingsTreeSettingElement) { this.defaultOverrideIndicator.element.style.display = 'none'; - const sourceToDisplay = getDefaultValueSourceToDisplay(element); + let sourceToDisplay = getDefaultValueSourceToDisplay(element); if (sourceToDisplay !== undefined) { this.defaultOverrideIndicator.element.style.display = 'inline'; this.defaultOverrideIndicator.disposables.clear(); - const defaultOverrideHoverContent = localize('defaultOverriddenDetails', "Default setting value overridden by {0}", sourceToDisplay); + // Show source of default value when hovered + if (Array.isArray(sourceToDisplay) && sourceToDisplay.length === 1) { + sourceToDisplay = sourceToDisplay[0]; + } + + let defaultOverrideHoverContent; + if (!Array.isArray(sourceToDisplay)) { + defaultOverrideHoverContent = localize('defaultOverriddenDetails', "Default setting value overridden by `{0}`", sourceToDisplay); + } else { + sourceToDisplay = sourceToDisplay.map(source => `\`${source}\``); + defaultOverrideHoverContent = localize('multipledefaultOverriddenDetails', "A default values has been set by {0}", sourceToDisplay.slice(0, -1).join(', ') + ' & ' + sourceToDisplay.slice(-1)); + } + const showHover = (focus: boolean) => { return this.hoverService.showHover({ - content: defaultOverrideHoverContent, + content: new MarkdownString().appendMarkdown(defaultOverrideHoverContent), target: this.defaultOverrideIndicator.element, position: { hoverPosition: HoverPosition.BELOW, @@ -473,14 +485,22 @@ export class SettingsTreeIndicatorsLabel implements IDisposable { } } -function getDefaultValueSourceToDisplay(element: SettingsTreeSettingElement): string | undefined { - let sourceToDisplay: string | undefined; +function getDefaultValueSourceToDisplay(element: SettingsTreeSettingElement): string | undefined | string[] { + let sourceToDisplay: string | undefined | string[]; const defaultValueSource = element.defaultValueSource; if (defaultValueSource) { - if (typeof defaultValueSource !== 'string') { - sourceToDisplay = defaultValueSource.displayName ?? defaultValueSource.id; + if (defaultValueSource instanceof Map) { + sourceToDisplay = []; + for (const [, value] of defaultValueSource) { + const newValue = typeof value !== 'string' ? value.displayName ?? value.id : value; + if (!sourceToDisplay.includes(newValue)) { + sourceToDisplay.push(newValue); + } + } } else if (typeof defaultValueSource === 'string') { sourceToDisplay = defaultValueSource; + } else { + sourceToDisplay = defaultValueSource.displayName ?? defaultValueSource.id; } } return sourceToDisplay; @@ -538,9 +558,19 @@ export function getIndicatorsLabelAriaLabel(element: SettingsTreeSettingElement, } // Add default override indicator text - const sourceToDisplay = getDefaultValueSourceToDisplay(element); + let sourceToDisplay = getDefaultValueSourceToDisplay(element); if (sourceToDisplay !== undefined) { - ariaLabelSections.push(localize('defaultOverriddenDetailsAriaLabel', "{0} overrides the default value", sourceToDisplay)); + if (Array.isArray(sourceToDisplay) && sourceToDisplay.length === 1) { + sourceToDisplay = sourceToDisplay[0]; + } + + let overriddenDetailsText; + if (!Array.isArray(sourceToDisplay)) { + overriddenDetailsText = localize('defaultOverriddenDetailsAriaLabel', "{0} overrides the default value", sourceToDisplay); + } else { + overriddenDetailsText = localize('multipleDefaultOverriddenDetailsAriaLabel', "{0} override the default value", sourceToDisplay.slice(0, -1).join(', ') + ' & ' + sourceToDisplay.slice(-1)); + } + ariaLabelSections.push(overriddenDetailsText); } // Add text about default values being overridden in other languages diff --git a/src/vs/workbench/contrib/preferences/browser/settingsTree.ts b/src/vs/workbench/contrib/preferences/browser/settingsTree.ts index 79dadc818220a..06efb414415c6 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsTree.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsTree.ts @@ -60,8 +60,8 @@ import { settingsMoreActionIcon } from 'vs/workbench/contrib/preferences/browser import { SettingsTarget } from 'vs/workbench/contrib/preferences/browser/preferencesWidgets'; import { ISettingOverrideClickEvent, SettingsTreeIndicatorsLabel, getIndicatorsLabelAriaLabel } from 'vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators'; import { ITOCEntry } from 'vs/workbench/contrib/preferences/browser/settingsLayout'; -import { ISettingsEditorViewState, SettingsTreeElement, SettingsTreeGroupChild, SettingsTreeGroupElement, SettingsTreeNewExtensionsElement, SettingsTreeSettingElement, inspectSetting, settingKeyToDisplayFormat } from 'vs/workbench/contrib/preferences/browser/settingsTreeModels'; -import { ExcludeSettingWidget, IListDataItem, IObjectDataItem, IObjectEnumOption, IObjectKeySuggester, IObjectValueSuggester, ISettingListChangeEvent, IncludeSettingWidget, ListSettingWidget, ObjectSettingCheckboxWidget, ObjectSettingDropdownWidget, ObjectValue } from 'vs/workbench/contrib/preferences/browser/settingsWidgets'; +import { ISettingsEditorViewState, SettingsTreeElement, SettingsTreeGroupChild, SettingsTreeGroupElement, SettingsTreeNewExtensionsElement, SettingsTreeSettingElement, inspectSetting, objectSettingSupportsRemoveDefaultValue, settingKeyToDisplayFormat } from 'vs/workbench/contrib/preferences/browser/settingsTreeModels'; +import { ExcludeSettingWidget, IIncludeExcludeDataItem, IListDataItem, IObjectDataItem, IObjectEnumOption, IObjectKeySuggester, IObjectValueSuggester, ISettingListChangeEvent, IncludeSettingWidget, ListSettingWidget, ObjectSettingCheckboxWidget, ObjectSettingDropdownWidget, ObjectValue, SettingListEvent } from 'vs/workbench/contrib/preferences/browser/settingsWidgets'; import { LANGUAGE_SETTING_TAG, SETTINGS_EDITOR_COMMAND_SHOW_CONTEXT_MENU, compareTwoNullableNumbers } from 'vs/workbench/contrib/preferences/common/preferences'; import { settingsNumberInputBackground, settingsNumberInputBorder, settingsNumberInputForeground, settingsSelectBackground, settingsSelectBorder, settingsSelectForeground, settingsSelectListBorder, settingsTextInputBackground, settingsTextInputBorder, settingsTextInputForeground } from 'vs/workbench/contrib/preferences/common/settingsEditorColorRegistry'; import { APPLY_ALL_PROFILES_SETTING, IWorkbenchConfigurationService } from 'vs/workbench/services/configuration/common/configuration'; @@ -74,14 +74,27 @@ import { IHoverService } from 'vs/platform/hover/browser/hover'; const $ = DOM.$; -function getIncludeExcludeDisplayValue(element: SettingsTreeSettingElement): IListDataItem[] { +function getIncludeExcludeDisplayValue(element: SettingsTreeSettingElement): IIncludeExcludeDataItem[] { + const elementDefaultValue: Record = typeof element.defaultValue === 'object' + ? element.defaultValue ?? {} + : {}; + const data = element.isConfigured ? - { ...element.defaultValue, ...element.scopeValue } : - element.defaultValue; + { ...elementDefaultValue, ...element.scopeValue } : + elementDefaultValue; return Object.keys(data) .filter(key => !!data[key]) .map(key => { + const defaultValue = elementDefaultValue[key]; + + // Get source if it's a default value + let source: string | undefined; + if (defaultValue === data[key] && element.setting.type === 'object' && element.defaultValueSource instanceof Map) { + const defaultSource = element.defaultValueSource.get(key); + source = typeof defaultSource === 'string' ? defaultSource : defaultSource?.displayName; + } + const value = data[key]; const sibling = typeof value === 'boolean' ? undefined : value.when; return { @@ -90,7 +103,8 @@ function getIncludeExcludeDisplayValue(element: SettingsTreeSettingElement): ILi data: key }, sibling, - elementType: element.valueType + elementType: element.valueType, + source }; }); } @@ -162,6 +176,14 @@ function getObjectDisplayValue(element: SettingsTreeSettingElement): IObjectData return Object.keys(data).map(key => { const defaultValue = elementDefaultValue[key]; + + // Get source if it's a default value + let source: string | undefined; + if (defaultValue === data[key] && element.setting.type === 'object' && element.defaultValueSource instanceof Map) { + const defaultSource = element.defaultValueSource.get(key); + source = typeof defaultSource === 'string' ? defaultSource : defaultSource?.displayName; + } + if (isDefined(objectProperties) && key in objectProperties) { if (element.setting.allKeysAreBoolean) { return { @@ -174,7 +196,9 @@ function getObjectDisplayValue(element: SettingsTreeSettingElement): IObjectData data: data[key] }, keyDescription: objectProperties[key].description, - removable: false + removable: false, + resetable: true, + source } as IObjectDataItem; } @@ -192,12 +216,15 @@ function getObjectDisplayValue(element: SettingsTreeSettingElement): IObjectData }, keyDescription: objectProperties[key].description, removable: isUndefinedOrNull(defaultValue), + resetable: !isUndefinedOrNull(defaultValue), + source } as IObjectDataItem; } - // The row is removable if it doesn't have a default value assigned. - // Otherwise, it is not removable, but its value can be reset to the default. - const removable = !defaultValue; + // The row is removable if it doesn't have a default value assigned or the setting supports removing the default value. + // If a default value is assigned and the user modified the default, it can be reset back to the default. + const removable = defaultValue === undefined || objectSettingSupportsRemoveDefaultValue(element.setting.key); + const resetable = defaultValue && defaultValue !== data[key]; const schema = patternsAndSchemas.find(({ pattern }) => pattern.test(key))?.schema; if (schema) { const valueEnumOptions = getEnumOptionsFromSchema(schema); @@ -210,6 +237,8 @@ function getObjectDisplayValue(element: SettingsTreeSettingElement): IObjectData }, keyDescription: schema.description, removable, + resetable, + source } as IObjectDataItem; } @@ -228,6 +257,8 @@ function getObjectDisplayValue(element: SettingsTreeSettingElement): IObjectData }, keyDescription: typeof objectAdditionalProperties === 'object' ? objectAdditionalProperties.description : undefined, removable, + resetable, + source } as IObjectDataItem; }).filter(item => !isUndefinedOrNull(item.value.data)); } @@ -629,12 +660,12 @@ interface ISettingComplexItemTemplate extends ISettingItemTemplate { } interface ISettingListItemTemplate extends ISettingItemTemplate { - listWidget: ListSettingWidget; + listWidget: ListSettingWidget; validationErrorMessageElement: HTMLElement; } interface ISettingIncludeExcludeItemTemplate extends ISettingItemTemplate { - includeExcludeWidget: ListSettingWidget; + includeExcludeWidget: ListSettingWidget; } interface ISettingObjectItemTemplate extends ISettingItemTemplate | undefined> { @@ -1174,7 +1205,7 @@ class SettingArrayRenderer extends AbstractSettingRenderer implements ITreeRende return template; } - private computeNewList(template: ISettingListItemTemplate, e: ISettingListChangeEvent): string[] | undefined { + private computeNewList(template: ISettingListItemTemplate, e: SettingListEvent): string[] | undefined { if (template.context) { let newValue: string[] = []; if (Array.isArray(template.context.scopeValue)) { @@ -1183,33 +1214,28 @@ class SettingArrayRenderer extends AbstractSettingRenderer implements ITreeRende newValue = [...template.context.value]; } - if (e.sourceIndex !== undefined) { + if (e.type === 'move') { // A drag and drop occurred const sourceIndex = e.sourceIndex; - const targetIndex = e.targetIndex!; + const targetIndex = e.targetIndex; const splicedElem = newValue.splice(sourceIndex, 1)[0]; newValue.splice(targetIndex, 0, splicedElem); - } else if (e.targetIndex !== undefined) { - const itemValueData = e.item?.value.data.toString() ?? ''; - // Delete value - if (!e.item?.value.data && e.originalItem.value.data && e.targetIndex > -1) { - newValue.splice(e.targetIndex, 1); - } + } else if (e.type === 'remove' || e.type === 'reset') { + newValue.splice(e.targetIndex, 1); + } else if (e.type === 'change') { + const itemValueData = e.newItem.value.data.toString(); + // Update value - else if (e.item?.value.data && e.originalItem.value.data) { - if (e.targetIndex > -1) { - newValue[e.targetIndex] = itemValueData; - } - // For some reason, we are updating and cannot find original value - // Just append the value in this case - else { - newValue.push(itemValueData); - } + if (e.targetIndex > -1) { + newValue[e.targetIndex] = itemValueData; } - // Add value - else if (e.item?.value.data && !e.originalItem.value.data && e.targetIndex >= newValue.length) { + // For some reason, we are updating and cannot find original value + // Just append the value in this case + else { newValue.push(itemValueData); } + } else if (e.type === 'add') { + newValue.push(e.newItem.value.data.toString()); } if ( @@ -1288,9 +1314,10 @@ abstract class AbstractSettingObjectRenderer extends AbstractSettingRenderer imp return template; } - protected onDidChangeObject(template: ISettingObjectItemTemplate, e: ISettingListChangeEvent): void { + protected onDidChangeObject(template: ISettingObjectItemTemplate, e: SettingListEvent): void { const widget = (template.objectCheckboxWidget ?? template.objectDropdownWidget)!; if (template.context) { + const settingSupportsRemoveDefault = objectSettingSupportsRemoveDefaultValue(template.context.setting.key); const defaultValue: Record = typeof template.context.defaultValue === 'object' ? template.context.defaultValue ?? {} : {}; @@ -1299,45 +1326,55 @@ abstract class AbstractSettingObjectRenderer extends AbstractSettingRenderer imp ? template.context.scopeValue ?? {} : {}; - const newValue: Record = {}; + const newValue: Record = { ...template.context.scopeValue }; // Initialize with scoped values as removed default values are not rendered const newItems: IObjectDataItem[] = []; widget.items.forEach((item, idx) => { // Item was updated - if (isDefined(e.item) && e.targetIndex === idx) { - newValue[e.item.key.data] = e.item.value.data; - newItems.push(e.item); + if ((e.type === 'change' || e.type === 'move') && e.targetIndex === idx) { + // If the key of the default value is changed, remove the default value + if (e.originalItem.key.data !== e.newItem.key.data && settingSupportsRemoveDefault && e.originalItem.key.data in defaultValue) { + newValue[e.originalItem.key.data] = null; + } + newValue[e.newItem.key.data] = e.newItem.value.data; + newItems.push(e.newItem); } // All remaining items, but skip the one that we just updated - else if (isUndefinedOrNull(e.item) || e.item.key.data !== item.key.data) { + else if ((e.type !== 'change' && e.type !== 'move') || e.newItem.key.data !== item.key.data) { newValue[item.key.data] = item.value.data; newItems.push(item); } }); // Item was deleted - if (isUndefinedOrNull(e.item)) { - delete newValue[e.originalItem.key.data]; + if (e.type === 'remove' || e.type === 'reset') { + const objectKey = e.originalItem.key.data; + const removingDefaultValue = e.type === 'remove' && settingSupportsRemoveDefault && defaultValue[objectKey] === e.originalItem.value.data; + if (removingDefaultValue) { + newValue[objectKey] = null; + } else { + delete newValue[objectKey]; + } - const itemToDelete = newItems.findIndex(item => item.key.data === e.originalItem.key.data); - const defaultItemValue = defaultValue[e.originalItem.key.data] as string | boolean; + const itemToDelete = newItems.findIndex(item => item.key.data === objectKey); + const defaultItemValue = defaultValue[objectKey] as string | boolean; - // Item does not have a default - if (isUndefinedOrNull(defaultValue[e.originalItem.key.data]) && itemToDelete > -1) { + // Item does not have a default or default is bing removed + if (removingDefaultValue || isUndefinedOrNull(defaultValue[objectKey]) && itemToDelete > -1) { newItems.splice(itemToDelete, 1); - } else if (itemToDelete > -1) { + } else if (!removingDefaultValue && itemToDelete > -1) { newItems[itemToDelete].value.data = defaultItemValue; } } // New item was added - else if (widget.isItemNew(e.originalItem) && e.item.key.data !== '') { - newValue[e.item.key.data] = e.item.value.data; - newItems.push(e.item); + else if (e.type === 'add') { + newValue[e.newItem.key.data] = e.newItem.value.data; + newItems.push(e.newItem); } Object.entries(newValue).forEach(([key, value]) => { // value from the scope has changed back to the default - if (scopeValue[key] !== value && defaultValue[key] === value) { + if (scopeValue[key] !== value && defaultValue[key] === value && !(settingSupportsRemoveDefault && value === null)) { delete newValue[key]; } }); @@ -1462,25 +1499,27 @@ abstract class SettingIncludeExcludeRenderer extends AbstractSettingRenderer imp return template; } - private onDidChangeIncludeExclude(template: ISettingIncludeExcludeItemTemplate, e: ISettingListChangeEvent): void { + private onDidChangeIncludeExclude(template: ISettingIncludeExcludeItemTemplate, e: SettingListEvent): void { if (template.context) { const newValue = { ...template.context.scopeValue }; // first delete the existing entry, if present - if (e.originalItem.value.data.toString() in template.context.defaultValue) { - // delete a default by overriding it - newValue[e.originalItem.value.data.toString()] = false; - } else { - delete newValue[e.originalItem.value.data.toString()]; + if (e.type !== 'add') { + if (e.originalItem.value.data.toString() in template.context.defaultValue) { + // delete a default by overriding it + newValue[e.originalItem.value.data.toString()] = false; + } else { + delete newValue[e.originalItem.value.data.toString()]; + } } // then add the new or updated entry, if present - if (e.item?.value) { - if (e.item.value.data.toString() in template.context.defaultValue && !e.item.sibling) { + if (e.type === 'change' || e.type === 'add' || e.type === 'move') { + if (e.newItem.value.data.toString() in template.context.defaultValue && !e.newItem.sibling) { // add a default by deleting its override - delete newValue[e.item.value.data.toString()]; + delete newValue[e.newItem.value.data.toString()]; } else { - newValue[e.item.value.data.toString()] = e.item.sibling ? { when: e.item.sibling } : true; + newValue[e.newItem.value.data.toString()] = e.newItem.sibling ? { when: e.newItem.sibling } : true; } } diff --git a/src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts b/src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts index e30c3bb549a12..e651ffdccf7b3 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts @@ -17,7 +17,7 @@ import { FOLDER_SCOPES, WORKSPACE_SCOPES, REMOTE_MACHINE_SCOPES, LOCAL_MACHINE_S import { IJSONSchema } from 'vs/base/common/jsonSchema'; import { Disposable } from 'vs/base/common/lifecycle'; import { Emitter } from 'vs/base/common/event'; -import { ConfigurationScope, EditPresentationTypes, Extensions, IConfigurationRegistry, IExtensionInfo } from 'vs/platform/configuration/common/configurationRegistry'; +import { ConfigurationDefaultValueSource, ConfigurationScope, EditPresentationTypes, Extensions, IConfigurationRegistry } from 'vs/platform/configuration/common/configurationRegistry'; import { ILanguageService } from 'vs/editor/common/languages/language'; import { Registry } from 'vs/platform/registry/common/platform'; import { IUserDataProfileService } from 'vs/workbench/services/userDataProfile/common/userDataProfile'; @@ -135,7 +135,7 @@ export class SettingsTreeSettingElement extends SettingsTreeElement { * The source of the default value to display. * This value also accounts for extension-contributed language-specific default value overrides. */ - defaultValueSource: string | IExtensionInfo | undefined; + defaultValueSource: ConfigurationDefaultValueSource | undefined; /** * Whether the setting is configured in the selected scope. @@ -792,11 +792,25 @@ function isIncludeSetting(setting: ISetting): boolean { return setting.key === 'files.readonlyInclude'; } -function isObjectRenderableSchema({ type }: IJSONSchema): boolean { - return type === 'string' || type === 'boolean' || type === 'integer' || type === 'number'; +// The values of the following settings when a default values has been removed +export function objectSettingSupportsRemoveDefaultValue(key: string): boolean { + return key === 'workbench.editor.customLabels.patterns'; +} + +function isObjectRenderableSchema({ type }: IJSONSchema, key: string): boolean { + if (type === 'string' || type === 'boolean' || type === 'integer' || type === 'number') { + return true; + } + + if (objectSettingSupportsRemoveDefaultValue(key) && Array.isArray(type) && type.length === 2) { + return type.includes('null') && (type.includes('string') || type.includes('boolean') || type.includes('integer') || type.includes('number')); + } + + return false; } function isObjectSetting({ + key, type, objectProperties, objectPatternProperties, @@ -838,7 +852,7 @@ function isObjectSetting({ return [schema]; }).flat(); - return flatSchemas.every(isObjectRenderableSchema); + return flatSchemas.every((schema) => isObjectRenderableSchema(schema, key)); } function settingTypeEnumRenderable(_type: string | string[]) { diff --git a/src/vs/workbench/contrib/preferences/browser/settingsWidgets.ts b/src/vs/workbench/contrib/preferences/browser/settingsWidgets.ts index 5021036167b24..9e9dcc17e86d1 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsWidgets.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsWidgets.ts @@ -29,6 +29,9 @@ import { settingsSelectBackground, settingsSelectBorder, settingsSelectForegroun import { defaultButtonStyles, getInputBoxStyle, getSelectBoxStyles } from 'vs/platform/theme/browser/defaultStyles'; import { getDefaultHoverDelegate } from 'vs/base/browser/ui/hover/hoverDelegateFactory'; import { IHoverService } from 'vs/platform/hover/browser/hover'; +import { MarkdownString } from 'vs/base/common/htmlContent'; +import { IManagedHoverTooltipMarkdownString } from 'vs/base/browser/ui/hover/hover'; +import { SettingValueType } from 'vs/workbench/services/preferences/common/preferences'; const $ = DOM.$; @@ -110,21 +113,49 @@ export class ListSettingListModel { } export interface ISettingListChangeEvent { + type: 'change'; originalItem: TDataItem; - item?: TDataItem; - targetIndex?: number; - sourceIndex?: number; + newItem: TDataItem; + targetIndex: number; } +export interface ISettingListAddEvent { + type: 'add'; + newItem: TDataItem; + targetIndex: number; +} + +export interface ISettingListMoveEvent { + type: 'move'; + originalItem: TDataItem; + newItem: TDataItem; + targetIndex: number; + sourceIndex: number; +} + +export interface ISettingListRemoveEvent { + type: 'remove'; + originalItem: TDataItem; + targetIndex: number; +} + +export interface ISettingListResetEvent { + type: 'reset'; + originalItem: TDataItem; + targetIndex: number; +} + +export type SettingListEvent = ISettingListChangeEvent | ISettingListAddEvent | ISettingListMoveEvent | ISettingListRemoveEvent | ISettingListResetEvent; + export abstract class AbstractListSettingWidget extends Disposable { private listElement: HTMLElement; private rowElements: HTMLElement[] = []; - protected readonly _onDidChangeList = this._register(new Emitter>()); + protected readonly _onDidChangeList = this._register(new Emitter>()); protected readonly model = new ListSettingListModel(this.getEmptyItem()); protected readonly listDisposables = this._register(new DisposableStore()); - readonly onDidChangeList: Event> = this._onDidChangeList.event; + readonly onDidChangeList: Event> = this._onDidChangeList.event; get domNode(): HTMLElement { return this.listElement; @@ -250,11 +281,20 @@ export abstract class AbstractListSettingWidget extend protected handleItemChange(originalItem: TDataItem, changedItem: TDataItem, idx: number) { this.model.setEditKey('none'); - this._onDidChangeList.fire({ - originalItem, - item: changedItem, - targetIndex: idx, - }); + if (this.isItemNew(originalItem)) { + this._onDidChangeList.fire({ + type: 'add', + newItem: changedItem, + targetIndex: idx, + }); + } else { + this._onDidChangeList.fire({ + type: 'change', + originalItem, + newItem: changedItem, + targetIndex: idx, + }); + } this.renderList(); } @@ -396,17 +436,17 @@ export interface IListDataItem { sibling?: string; } -interface ListSettingWidgetDragDetails { +interface ListSettingWidgetDragDetails { element: HTMLElement; - item: IListDataItem; + item: TListDataItem; itemIndex: number; } -export class ListSettingWidget extends AbstractListSettingWidget { +export class ListSettingWidget extends AbstractListSettingWidget { private keyValueSuggester: IObjectKeySuggester | undefined; private showAddButton: boolean = true; - override setValue(listData: IListDataItem[], options?: IListSetValueOptions) { + override setValue(listData: TListDataItem[], options?: IListSetValueOptions) { this.keyValueSuggester = options?.keySuggester; this.showAddButton = options?.showAddButton ?? true; super.setValue(listData); @@ -421,13 +461,13 @@ export class ListSettingWidget extends AbstractListSettingWidget super(container, themeService, contextViewService); } - protected getEmptyItem(): IListDataItem { + protected getEmptyItem(): TListDataItem { return { value: { type: 'string', data: '' } - }; + } as TListDataItem; } protected override isAddButtonVisible(): boolean { @@ -438,7 +478,7 @@ export class ListSettingWidget extends AbstractListSettingWidget return ['setting-list-widget']; } - protected getActionsForItem(item: IListDataItem, idx: number): IAction[] { + protected getActionsForItem(item: TListDataItem, idx: number): IAction[] { return [ { class: ThemeIcon.asClassName(settingsEditIcon), @@ -452,20 +492,20 @@ export class ListSettingWidget extends AbstractListSettingWidget enabled: true, id: 'workbench.action.removeListItem', tooltip: this.getLocalizedStrings().deleteActionTooltip, - run: () => this._onDidChangeList.fire({ originalItem: item, item: undefined, targetIndex: idx }) + run: () => this._onDidChangeList.fire({ type: 'remove', originalItem: item, targetIndex: idx }) } ] as IAction[]; } - private dragDetails: ListSettingWidgetDragDetails | undefined; + private dragDetails: ListSettingWidgetDragDetails | undefined; - private getDragImage(item: IListDataItem): HTMLElement { + private getDragImage(item: TListDataItem): HTMLElement { const dragImage = $('.monaco-drag-image'); dragImage.textContent = item.value.data; return dragImage; } - protected renderItem(item: IListDataItem, idx: number): RowElementGroup { + protected renderItem(item: TListDataItem, idx: number): RowElementGroup { const rowElement = $('.setting-list-row'); const valueElement = DOM.append(rowElement, $('.setting-list-value')); const siblingElement = DOM.append(rowElement, $('.setting-list-sibling')); @@ -477,7 +517,7 @@ export class ListSettingWidget extends AbstractListSettingWidget return { rowElement, keyElement: valueElement, valueElement: siblingElement }; } - protected addDragAndDrop(rowElement: HTMLElement, item: IListDataItem, idx: number) { + protected addDragAndDrop(rowElement: HTMLElement, item: TListDataItem, idx: number) { if (this.inReadMode) { rowElement.draggable = true; rowElement.classList.add('draggable'); @@ -530,9 +570,10 @@ export class ListSettingWidget extends AbstractListSettingWidget counter = 0; if (this.dragDetails.element !== rowElement) { this._onDidChangeList.fire({ + type: 'move', originalItem: this.dragDetails.item, sourceIndex: this.dragDetails.itemIndex, - item, + newItem: item, targetIndex: idx }); } @@ -548,7 +589,7 @@ export class ListSettingWidget extends AbstractListSettingWidget })); } - protected renderEdit(item: IListDataItem, idx: number): HTMLElement { + protected renderEdit(item: TListDataItem, idx: number): HTMLElement { const rowElement = $('.setting-list-edit-row'); let valueInput: InputBox | SelectBox; let currentDisplayValue: string; @@ -580,7 +621,7 @@ export class ListSettingWidget extends AbstractListSettingWidget break; } - const updatedInputBoxItem = (): IListDataItem => { + const updatedInputBoxItem = (): TListDataItem => { const inputBox = valueInput as InputBox; return { value: { @@ -588,16 +629,16 @@ export class ListSettingWidget extends AbstractListSettingWidget data: inputBox.value }, sibling: siblingInput?.value - }; + } as TListDataItem; }; - const updatedSelectBoxItem = (selectedValue: string): IListDataItem => { + const updatedSelectBoxItem = (selectedValue: string): TListDataItem => { return { value: { type: 'enum', data: selectedValue, options: currentEnumOptions ?? [] } - }; + } as TListDataItem; }; const onKeyDown = (e: StandardKeyboardEvent) => { if (e.equals(KeyCode.Enter)) { @@ -674,11 +715,11 @@ export class ListSettingWidget extends AbstractListSettingWidget return rowElement; } - override isItemNew(item: IListDataItem): boolean { + override isItemNew(item: TListDataItem): boolean { return item.value.data === ''; } - protected addTooltipsToRow(rowElementGroup: RowElementGroup, { value, sibling }: IListDataItem) { + protected addTooltipsToRow(rowElementGroup: RowElementGroup, { value, sibling }: TListDataItem) { const title = isUndefinedOrNull(sibling) ? localize('listValueHintLabel', "List item `{0}`", value.data) : localize('listSiblingHintLabel', "List item `{0}` with sibling `${1}`", value.data, sibling); @@ -729,22 +770,28 @@ export class ListSettingWidget extends AbstractListSettingWidget } } -export class ExcludeSettingWidget extends ListSettingWidget { +export class ExcludeSettingWidget extends ListSettingWidget { protected override getContainerClasses() { return ['setting-list-include-exclude-widget']; } - protected override addDragAndDrop(rowElement: HTMLElement, item: IListDataItem, idx: number) { + protected override addDragAndDrop(rowElement: HTMLElement, item: IIncludeExcludeDataItem, idx: number) { return; } - protected override addTooltipsToRow(rowElementGroup: RowElementGroup, { value, sibling }: IListDataItem): void { - const title = isUndefinedOrNull(sibling) - ? localize('excludePatternHintLabel', "Exclude files matching `{0}`", value.data) - : localize('excludeSiblingHintLabel', "Exclude files matching `{0}`, only when a file matching `{1}` is present", value.data, sibling); + protected override addTooltipsToRow(rowElementGroup: RowElementGroup, item: IIncludeExcludeDataItem): void { + let title = isUndefinedOrNull(item.sibling) + ? localize('excludePatternHintLabel', "Exclude files matching `{0}`", item.value.data) + : localize('excludeSiblingHintLabel', "Exclude files matching `{0}`, only when a file matching `{1}` is present", item.value.data, item.sibling); + + if (item.source) { + title += localize('excludeIncludeSource', ". Default value provided by `{0}`", item.source); + } + + const markdownTitle = new MarkdownString().appendMarkdown(title); const { rowElement } = rowElementGroup; - this.listDisposables.add(this.hoverService.setupManagedHover(getDefaultHoverDelegate('mouse'), rowElement, title)); + this.listDisposables.add(this.hoverService.setupManagedHover(getDefaultHoverDelegate('mouse'), rowElement, { markdown: markdownTitle, markdownNotSupportedFallback: title })); rowElement.setAttribute('aria-label', title); } @@ -759,22 +806,28 @@ export class ExcludeSettingWidget extends ListSettingWidget { } } -export class IncludeSettingWidget extends ListSettingWidget { +export class IncludeSettingWidget extends ListSettingWidget { protected override getContainerClasses() { return ['setting-list-include-exclude-widget']; } - protected override addDragAndDrop(rowElement: HTMLElement, item: IListDataItem, idx: number) { + protected override addDragAndDrop(rowElement: HTMLElement, item: IIncludeExcludeDataItem, idx: number) { return; } - protected override addTooltipsToRow(rowElementGroup: RowElementGroup, { value, sibling }: IListDataItem): void { - const title = isUndefinedOrNull(sibling) - ? localize('includePatternHintLabel', "Include files matching `{0}`", value.data) - : localize('includeSiblingHintLabel', "Include files matching `{0}`, only when a file matching `{1}` is present", value.data, sibling); + protected override addTooltipsToRow(rowElementGroup: RowElementGroup, item: IIncludeExcludeDataItem): void { + let title = isUndefinedOrNull(item.sibling) + ? localize('includePatternHintLabel', "Include files matching `{0}`", item.value.data) + : localize('includeSiblingHintLabel', "Include files matching `{0}`, only when a file matching `{1}` is present", item.value.data, item.sibling); + + if (item.source) { + title += localize('excludeIncludeSource', ". Default value provided by `{0}`", item.source); + } + + const markdownTitle = new MarkdownString().appendMarkdown(title); const { rowElement } = rowElementGroup; - this.listDisposables.add(this.hoverService.setupManagedHover(getDefaultHoverDelegate('mouse'), rowElement, title)); + this.listDisposables.add(this.hoverService.setupManagedHover(getDefaultHoverDelegate('mouse'), rowElement, { markdown: markdownTitle, markdownNotSupportedFallback: title })); rowElement.setAttribute('aria-label', title); } @@ -818,7 +871,16 @@ export interface IObjectDataItem { key: ObjectKey; value: ObjectValue; keyDescription?: string; + source?: string; removable: boolean; + resetable: boolean; +} + +export interface IIncludeExcludeDataItem { + value: ObjectKey; + elementType: SettingValueType; + sibling?: string; + source?: string; } export interface IObjectValueSuggester { @@ -886,6 +948,7 @@ export class ObjectSettingDropdownWidget extends AbstractListSettingWidget this._onDidChangeList.fire({ originalItem: item, item: undefined, targetIndex: idx }) + tooltip: this.getLocalizedStrings().resetActionTooltip, + run: () => this._onDidChangeList.fire({ type: 'reset', originalItem: item, targetIndex: idx }) }); - } else { + } + + if (item.removable) { actions.push({ - class: ThemeIcon.asClassName(settingsDiscardIcon), + class: ThemeIcon.asClassName(settingsRemoveIcon), enabled: true, - id: 'workbench.action.resetListItem', + id: 'workbench.action.removeListItem', label: '', - tooltip: this.getLocalizedStrings().resetActionTooltip, - run: () => this._onDidChangeList.fire({ originalItem: item, item: undefined, targetIndex: idx }) + tooltip: this.getLocalizedStrings().deleteActionTooltip, + run: () => this._onDidChangeList.fire({ type: 'remove', originalItem: item, targetIndex: idx }) }); } @@ -1181,12 +1246,20 @@ export class ObjectSettingDropdownWidget extends AbstractListSettingWidget Date: Mon, 24 Jun 2024 15:45:29 +0200 Subject: [PATCH 2/4] Fix merge conflict --- src/vs/workbench/browser/workbench.contribution.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/vs/workbench/browser/workbench.contribution.ts b/src/vs/workbench/browser/workbench.contribution.ts index 984db318ce374..ae4d22c9eaa41 100644 --- a/src/vs/workbench/browser/workbench.contribution.ts +++ b/src/vs/workbench/browser/workbench.contribution.ts @@ -113,13 +113,8 @@ const registry = Registry.as(ConfigurationExtensions.Con })(), additionalProperties: { -<<<<<<< benibenj/radical-skink type: ['string', 'null'], - markdownDescription: localize('workbench.editor.label.template', "The template which should be rendered when the pattern mtches. May include the variables ${dirname}, ${filename} and ${extname}."), -======= - type: 'string', markdownDescription: localize('workbench.editor.label.template', "The template which should be rendered when the pattern matches. May include the variables ${dirname}, ${filename} and ${extname}."), ->>>>>>> main minLength: 1, pattern: '.*[a-zA-Z0-9].*' }, From d6c4595115254abdf37c199e3a1a690689f75480 Mon Sep 17 00:00:00 2001 From: BeniBenj Date: Mon, 24 Jun 2024 16:34:47 +0200 Subject: [PATCH 3/4] type check --- .../configuration/common/configurationRegistry.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/vs/platform/configuration/common/configurationRegistry.ts b/src/vs/platform/configuration/common/configurationRegistry.ts index 4dd83106eeab2..4722f8dd643e1 100644 --- a/src/vs/platform/configuration/common/configurationRegistry.ts +++ b/src/vs/platform/configuration/common/configurationRegistry.ts @@ -367,11 +367,15 @@ class ConfigurationRegistry implements IConfigurationRegistry { // Track the source of each value in the object if (source) { - let objectConfigurationSources = valuesSources.get(configuration) as Map | undefined; + let objectConfigurationSources = valuesSources.get(configuration); if (!objectConfigurationSources) { objectConfigurationSources = new Map(); valuesSources.set(configuration, objectConfigurationSources); } + if (!(objectConfigurationSources instanceof Map)) { + console.error('objectConfigurationSources is not a Map'); + continue; + } for (const objectKey in overrideValue) { objectConfigurationSources.set(objectKey, source); @@ -412,7 +416,12 @@ class ConfigurationRegistry implements IConfigurationRegistry { defaultValue = { ...existingDefaultValue, ...overrides[key] }; if (source) { - defaultValueSource = objectDefaults?.source as Map ?? new Map(); + defaultValueSource = objectDefaults?.source ?? new Map(); + if (!(defaultValueSource instanceof Map)) { + console.error('defaultValueSource is not a Map'); + continue; + } + for (const objectKey in overrides[key]) { defaultValueSource.set(objectKey, source); } From bff21712dbe80446c17fa8788dd5b4218275a734 Mon Sep 17 00:00:00 2001 From: BeniBenj Date: Mon, 24 Jun 2024 22:41:35 +0200 Subject: [PATCH 4/4] fix tests --- .../common/configurationRegistry.ts | 104 ++++++++++++------ .../test/common/configurations.test.ts | 61 +++++++++- 2 files changed, 125 insertions(+), 40 deletions(-) diff --git a/src/vs/platform/configuration/common/configurationRegistry.ts b/src/vs/platform/configuration/common/configurationRegistry.ts index 4722f8dd643e1..d7e9ae576b0bd 100644 --- a/src/vs/platform/configuration/common/configurationRegistry.ts +++ b/src/vs/platform/configuration/common/configurationRegistry.ts @@ -360,7 +360,7 @@ class ConfigurationRegistry implements IConfigurationRegistry { for (const configuration of Object.keys(overrides[key])) { const overrideValue = overrides[key][configuration]; - const isObjectSetting = types.isObject(overrideValue) && (types.isUndefined(defaultValue[configuration] || types.isObject(defaultValue[configuration]))); + const isObjectSetting = types.isObject(overrideValue) && (types.isUndefined(defaultValue[configuration]) || types.isObject(defaultValue[configuration])); if (isObjectSetting) { // Objects are merged instead of overridden defaultValue[configuration] = { ...(defaultValue[configuration] ?? {}), ...overrideValue }; @@ -415,15 +415,17 @@ class ConfigurationRegistry implements IConfigurationRegistry { const existingDefaultValue = objectDefaults?.value ?? property.defaultDefaultValue ?? {}; defaultValue = { ...existingDefaultValue, ...overrides[key] }; - if (source) { - defaultValueSource = objectDefaults?.source ?? new Map(); - if (!(defaultValueSource instanceof Map)) { - console.error('defaultValueSource is not a Map'); - continue; - } + defaultValueSource = objectDefaults?.source ?? new Map(); + if (!(defaultValueSource instanceof Map)) { + console.error('defaultValueSource is not a Map'); + continue; + } - for (const objectKey in overrides[key]) { + for (const objectKey in overrides[key]) { + if (source) { defaultValueSource.set(objectKey, source); + } else { + defaultValueSource.delete(objectKey); } } } @@ -455,50 +457,84 @@ class ConfigurationRegistry implements IConfigurationRegistry { const id = types.isString(source) ? source : source?.id; const configurationDefaultsOverride = this.configurationDefaultsOverrides.get(key); - if (!configurationDefaultsOverride || !configurationDefaultsOverride.source) { + if (!configurationDefaultsOverride) { continue; } - // If the default value is an object, remove the source of each key - if (configurationDefaultsOverride.source instanceof Map) { + if (OVERRIDE_PROPERTY_REGEX.test(key)) { + for (const configuration of Object.keys(overrides[key])) { + const overrideValue = overrides[key][configuration]; + + if (types.isObject(overrideValue)) { + const configurationSource = configurationDefaultsOverride.valuesSources?.get(configuration) as Map | undefined; - const keySources = configurationDefaultsOverride.source; - for (const objectKey in overrides[key]) { - const keySource = keySources.get(objectKey); - const keySourceId = types.isString(keySource) ? keySource : keySource?.id; + for (const overrideObjectKey of Object.keys(overrideValue)) { + const keySource = configurationSource?.get(overrideObjectKey); + const keySourceId = types.isString(keySource) ? keySource : keySource?.id; + if (keySourceId === id) { + configurationSource?.delete(overrideObjectKey); + delete configurationDefaultsOverride.value[configuration][overrideObjectKey]; + } + } - if (keySourceId === id) { - keySources.delete(objectKey); - delete configurationDefaultsOverride.value[objectKey]; + if (Object.keys(configurationDefaultsOverride.value[configuration]).length === 0) { + delete configurationDefaultsOverride.value[configuration]; + configurationDefaultsOverride.valuesSources?.delete(configuration); + } + } else { + const configurationSource = configurationDefaultsOverride.valuesSources?.get(configuration) as string | IExtensionInfo | undefined; + + const keySourceId = types.isString(configurationSource) ? configurationSource : configurationSource?.id; + if (keySourceId === id) { + configurationDefaultsOverride.valuesSources?.delete(configuration); + delete configurationDefaultsOverride.value[configuration]; + } } } - - if (keySources.size === 0) { + // Remove language configuration if empty ({[css]: {}} => {}) + const languageValues = this.configurationDefaultsOverrides.get(key); + if (languageValues && Object.keys(languageValues.value).length === 0) { this.configurationDefaultsOverrides.delete(key); + delete this.configurationProperties[key]; + delete this.defaultLanguageConfigurationOverridesNode.properties![key]; } - } - // Otherwise, remove the default value if the source matches - else { - const configurationDefaultsOverrideSourceId = types.isString(configurationDefaultsOverride.source) ? configurationDefaultsOverride.source : configurationDefaultsOverride.source.id; - if (id !== configurationDefaultsOverrideSourceId) { - continue; // Another source is overriding this default value - } + } else { + // If the default value is an object, remove the source of each key + if (configurationDefaultsOverride.source instanceof Map) { - this.configurationDefaultsOverrides.delete(key); - } + const keySources = configurationDefaultsOverride.source; + for (const objectKey in overrides[key]) { + const keySource = keySources.get(objectKey); + const keySourceId = types.isString(keySource) ? keySource : keySource?.id; - bucket.add(key); + if (keySourceId === id) { + keySources.delete(objectKey); + delete configurationDefaultsOverride.value[objectKey]; + } + } - if (OVERRIDE_PROPERTY_REGEX.test(key)) { - delete this.configurationProperties[key]; - delete this.defaultLanguageConfigurationOverridesNode.properties![key]; - } else { + if (keySources.size === 0) { + this.configurationDefaultsOverrides.delete(key); + } + } + // Otherwise, remove the default value if the source matches + else { + const configurationDefaultsOverrideSourceId = types.isString(configurationDefaultsOverride.source) ? configurationDefaultsOverride.source : configurationDefaultsOverride.source?.id; + if (id !== configurationDefaultsOverrideSourceId) { + continue; // Another source is overriding this default value + } + + this.configurationDefaultsOverrides.delete(key); + + } const property = this.configurationProperties[key]; if (property) { this.updatePropertyDefaultValue(key, property); this.updateSchema(key, property); } } + + bucket.add(key); } } diff --git a/src/vs/platform/configuration/test/common/configurations.test.ts b/src/vs/platform/configuration/test/common/configurations.test.ts index f86e6f662498c..eb02aec4f9a2a 100644 --- a/src/vs/platform/configuration/test/common/configurations.test.ts +++ b/src/vs/platform/configuration/test/common/configurations.test.ts @@ -110,7 +110,7 @@ suite('DefaultConfiguration', () => { assert.ok(equals(actual.getValue('a'), { b: { c: '2' } })); assert.ok(equals(actual.contents, { 'a': { b: { c: '2' } } })); - assert.deepStrictEqual(actual.keys, ['a.b', 'a.b.c']); + assert.deepStrictEqual(actual.keys.sort(), ['a.b', 'a.b.c']); }); test('Test registering the same property again', async () => { @@ -158,7 +158,7 @@ suite('DefaultConfiguration', () => { assert.ok(equals(actual.getValue('[a]'), { 'b': true })); assert.ok(equals(actual.contents, { '[a]': { 'b': true } })); assert.ok(equals(actual.overrides, [{ contents: { 'b': true }, identifiers: ['a'], keys: ['b'] }])); - assert.deepStrictEqual(actual.keys, ['[a]']); + assert.deepStrictEqual(actual.keys.sort(), ['[a]']); assert.strictEqual(actual.getOverrideValue('b', 'a'), true); }); @@ -191,7 +191,7 @@ suite('DefaultConfiguration', () => { assert.ok(equals(actual.getValue('[a]'), { 'b': true })); assert.ok(equals(actual.contents, { 'b': false, '[a]': { 'b': true } })); assert.ok(equals(actual.overrides, [{ contents: { 'b': true }, identifiers: ['a'], keys: ['b'] }])); - assert.deepStrictEqual(actual.keys, ['b', '[a]']); + assert.deepStrictEqual(actual.keys.sort(), ['[a]', 'b']); assert.strictEqual(actual.getOverrideValue('b', 'a'), true); }); @@ -227,7 +227,7 @@ suite('DefaultConfiguration', () => { assert.ok(equals(actual.getValue('[a]'), { 'b': true })); assert.ok(equals(actual.contents, { 'b': false, '[a]': { 'b': true } })); assert.ok(equals(actual.overrides, [{ contents: { 'b': true }, identifiers: ['a'], keys: ['b'] }])); - assert.deepStrictEqual(actual.keys, ['[a]', 'b']); + assert.deepStrictEqual(actual.keys.sort(), ['[a]', 'b']); assert.strictEqual(actual.getOverrideValue('b', 'a'), true); assert.deepStrictEqual(properties, ['b']); }); @@ -263,7 +263,7 @@ suite('DefaultConfiguration', () => { assert.ok(equals(actual.getValue('[a]'), { 'b': true })); assert.ok(equals(actual.contents, { 'b': false, '[a]': { 'b': true } })); assert.ok(equals(actual.overrides, [{ contents: { 'b': true }, identifiers: ['a'], keys: ['b'] }])); - assert.deepStrictEqual(actual.keys, ['b', '[a]']); + assert.deepStrictEqual(actual.keys.sort(), ['[a]', 'b']); assert.strictEqual(actual.getOverrideValue('b', 'a'), true); assert.deepStrictEqual(properties, ['[a]']); }); @@ -299,7 +299,7 @@ suite('DefaultConfiguration', () => { assert.ok(equals(actual.getValue('[a]'), { 'b': true })); assert.ok(equals(actual.contents, { 'b': false, '[a]': { 'b': true } })); assert.ok(equals(actual.overrides, [{ contents: { 'b': true }, identifiers: ['a'], keys: ['b'] }])); - assert.deepStrictEqual(actual.keys, ['b', '[a]']); + assert.deepStrictEqual(actual.keys.sort(), ['[a]', 'b']); assert.strictEqual(actual.getOverrideValue('b', 'a'), true); }); @@ -361,4 +361,53 @@ suite('DefaultConfiguration', () => { assert.deepStrictEqual(testObject.configurationModel.keys, ['b']); assert.strictEqual(testObject.configurationModel.getOverrideValue('b', 'a'), undefined); }); + + test('Test deregistering a merged language object setting', async () => { + const testObject = disposables.add(new DefaultConfiguration(new NullLogService())); + configurationRegistry.registerConfiguration({ + 'id': 'b', + 'order': 1, + 'title': 'b', + 'type': 'object', + 'properties': { + 'b': { + 'description': 'b', + 'type': 'object', + 'default': {}, + } + } + }); + const node1 = { + overrides: { + '[a]': { + 'b': { + 'aa': '1', + 'bb': '2' + } + } + }, + source: 'source1' + }; + + const node2 = { + overrides: { + '[a]': { + 'b': { + 'bb': '20', + 'cc': '30' + } + } + }, + source: 'source2' + }; + configurationRegistry.registerDefaultConfigurations([node1]); + configurationRegistry.registerDefaultConfigurations([node2]); + await testObject.initialize(); + configurationRegistry.deregisterDefaultConfigurations([node1]); + assert.ok(equals(testObject.configurationModel.getValue('[a]'), { 'b': { 'bb': '20', 'cc': '30' } })); + assert.ok(equals(testObject.configurationModel.contents, { '[a]': { 'b': { 'bb': '20', 'cc': '30' } }, 'b': {} })); + //assert.ok(equals(testObject.configurationModel.overrides, [{ '[a]': { 'b': { 'bb': '20', 'cc': '30' } } }])); TODO: Check this later + //assert.deepStrictEqual(testObject.configurationModel.keys.sort(), ['[a]', 'b']); + assert.ok(equals(testObject.configurationModel.getOverrideValue('b', 'a'), { 'bb': '20', 'cc': '30' })); + }); });