From 73f5fceccc5f937f6900f9d62ce635b619fb1ca7 Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Thu, 23 Jun 2022 10:05:00 -0700 Subject: [PATCH 1/7] Add language overrides draft --- .../preferences/browser/settingsEditor2.ts | 48 ++++++++++++------- .../settingsEditorSettingIndicators.ts | 45 ++++++++++++----- .../preferences/browser/settingsTreeModels.ts | 24 ++++++---- 3 files changed, 80 insertions(+), 37 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts b/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts index 5f798b2bf3436..4187857031f2a 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts @@ -834,8 +834,23 @@ export class SettingsEditor2 extends EditorPane { })); } - private createSettingsTree(container: HTMLElement): void { + private applyFilter(filter: string) { + if (this.searchWidget && !this.searchWidget.getValue().includes(filter)) { + // Prepend the filter to the query. + const newQuery = `${filter} ${this.searchWidget.getValue().trimStart()}`; + this.focusSearch(newQuery, false); + } + } + + private removeLanguageFilters() { + if (this.searchWidget && this.searchWidget.getValue().includes(`@${LANGUAGE_SETTING_TAG}`)) { + const query = this.searchWidget.getValue().split(' '); + const newQuery = query.filter(word => !word.startsWith(`@${LANGUAGE_SETTING_TAG}`)).join(' '); + this.focusSearch(newQuery, false); + } + } + private createSettingsTree(container: HTMLElement): void { this.settingRenderers = this.instantiationService.createInstance(SettingTreeRenderers); this._register(this.settingRenderers.onDidChangeSetting(e => this.onDidChangeSetting(e.key, e.value, e.type, e.manualReset))); this._register(this.settingRenderers.onDidOpenSettings(settingKey => { @@ -847,17 +862,6 @@ export class SettingsEditor2 extends EditorPane { this._currentFocusContext = SettingsFocusContext.SettingControl; this.settingRowFocused.set(false); })); - this._register(this.settingRenderers.onDidClickOverrideElement((element: ISettingOverrideClickEvent) => { - if (element.scope.toLowerCase() === 'workspace') { - this.settingsTargetsWidget.updateTarget(ConfigurationTarget.WORKSPACE); - } else if (element.scope.toLowerCase() === 'user') { - this.settingsTargetsWidget.updateTarget(ConfigurationTarget.USER_LOCAL); - } else if (element.scope.toLowerCase() === 'remote') { - this.settingsTargetsWidget.updateTarget(ConfigurationTarget.USER_REMOTE); - } - - this.searchWidget.setValue(element.targetKey); - })); this._register(this.settingRenderers.onDidChangeSettingHeight((params: HeightChangeParams) => { const { element, height } = params; try { @@ -866,12 +870,22 @@ export class SettingsEditor2 extends EditorPane { // the element was not found } })); - this._register(this.settingRenderers.onApplyFilter((filter: string) => { - if (this.searchWidget && !this.searchWidget.getValue().includes(filter)) { - // Prepend the filter to the query. - const newQuery = `${filter} ${this.searchWidget.getValue().trimStart()}`; - this.focusSearch(newQuery, false); + this._register(this.settingRenderers.onApplyFilter((filter) => this.applyFilter(filter))); + this._register(this.settingRenderers.onDidClickOverrideElement((element: ISettingOverrideClickEvent) => { + if (element.language) { + this.applyFilter(`@${LANGUAGE_SETTING_TAG}${element.language}`); + } else { + this.removeLanguageFilters(); + } + + if (element.scope === 'workspace') { + this.settingsTargetsWidget.updateTarget(ConfigurationTarget.WORKSPACE); + } else if (element.scope === 'user') { + this.settingsTargetsWidget.updateTarget(ConfigurationTarget.USER_LOCAL); + } else if (element.scope === 'remote') { + this.settingsTargetsWidget.updateTarget(ConfigurationTarget.USER_REMOTE); } + this.applyFilter(`@${ID_SETTING_TAG}${element.settingKey}`); })); this.settingsTree = this._register(this.instantiationService.createInstance(SettingsTree, diff --git a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts index 4398f140cab59..afb9973d336d1 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts @@ -10,6 +10,7 @@ import { ICustomHover, ITooltipMarkdownString, IUpdatableHoverOptions, setupCust import { SimpleIconLabel } from 'vs/base/browser/ui/iconLabel/simpleIconLabel'; import { Emitter } from 'vs/base/common/event'; import { DisposableStore } from 'vs/base/common/lifecycle'; +import { ILanguageService } from 'vs/editor/common/languages/language'; import { localize } from 'vs/nls'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; import { getIgnoredSettings } from 'vs/platform/userDataSync/common/settingsMerge'; @@ -19,9 +20,12 @@ import { IHoverService } from 'vs/workbench/services/hover/browser/hover'; const $ = DOM.$; +type ScopeString = 'workspace' | 'user' | 'remote'; + export interface ISettingOverrideClickEvent { - scope: string; - targetKey: string; + scope: ScopeString; + language: string; + settingKey: string; } /** @@ -39,7 +43,8 @@ export class SettingsTreeIndicatorsLabel { container: HTMLElement, @IConfigurationService configurationService: IConfigurationService, @IHoverService hoverService: IHoverService, - @IUserDataSyncEnablementService private readonly userDataSyncEnablementService: IUserDataSyncEnablementService) { + @IUserDataSyncEnablementService private readonly userDataSyncEnablementService: IUserDataSyncEnablementService, + @ILanguageService private readonly languageService: ILanguageService) { this.indicatorsContainerElement = DOM.append(container, $('.misc-label')); this.indicatorsContainerElement.style.display = 'inline'; @@ -105,6 +110,19 @@ export class SettingsTreeIndicatorsLabel { this.render(); } + private getInlineScopeDisplayText(completeScope: string): string { + const [scope, language] = completeScope.split(':'); + const localizedScope = scope === 'user' ? + localize('user', "User") : scope === 'workspace' ? + localize('workspace', "Workspace") : + localize('remote', "Remote"); + if (language.length) { + const languageName = this.languageService.getLanguageName(language) ?? language; + return `${localizedScope} [${languageName}]`; + } + return localizedScope; + } + updateScopeOverrides(element: SettingsTreeSettingElement, elementDisposables: DisposableStore, onDidClickOverrideElement: Emitter) { this.scopeOverridesElement.innerText = ''; this.scopeOverridesElement.style.display = 'none'; @@ -118,12 +136,14 @@ export class SettingsTreeIndicatorsLabel { this.scopeOverridesLabel.text = `${prefaceText}: `; const firstScope = element.overriddenScopeList[0]; - const view = DOM.append(this.scopeOverridesElement, $('a.modified-scope', undefined, firstScope)); + const view = DOM.append(this.scopeOverridesElement, $('a.modified-scope', undefined, this.getInlineScopeDisplayText(firstScope))); elementDisposables.add( DOM.addStandardDisposableListener(view, DOM.EventType.CLICK, (e: IMouseEvent) => { + const [scope, language] = firstScope.split(':'); onDidClickOverrideElement.fire({ - targetKey: element.setting.key, - scope: firstScope + settingKey: element.setting.key, + scope: scope as ScopeString, + language }); e.preventDefault(); e.stopPropagation(); @@ -142,8 +162,9 @@ export class SettingsTreeIndicatorsLabel { let contentMarkdownString = prefaceText; let contentFallback = prefaceText; for (const scope of element.overriddenScopeList) { - contentMarkdownString += `\n- [${scope}](${scope})`; - contentFallback += `\n• ${scope}`; + const scopeDisplayText = this.getInlineScopeDisplayText(scope); + contentMarkdownString += `\n- [${scopeDisplayText}](${encodeURIComponent(scope)})`; + contentFallback += `\n• ${scopeDisplayText}`; } const content: ITooltipMarkdownString = { markdown: { @@ -155,10 +176,12 @@ export class SettingsTreeIndicatorsLabel { }; let hover: ICustomHover | undefined = undefined; const options: IUpdatableHoverOptions = { - linkHandler: (scope: string) => { + linkHandler: (url: string) => { + const [scope, language] = decodeURIComponent(url).split(':'); onDidClickOverrideElement.fire({ - targetKey: element.setting.key, - scope + settingKey: element.setting.key, + scope: scope as ScopeString, + language }); hover!.dispose(); } diff --git a/src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts b/src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts index db27fd2a45241..6c3966a249f31 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts @@ -7,7 +7,6 @@ import * as arrays from 'vs/base/common/arrays'; import { escapeRegExpCharacters, isFalsyOrWhitespace } from 'vs/base/common/strings'; import { isArray, withUndefinedAsNull, isUndefinedOrNull } from 'vs/base/common/types'; import { URI } from 'vs/base/common/uri'; -import { localize } from 'vs/nls'; import { ConfigurationTarget, IConfigurationValue } from 'vs/platform/configuration/common/configuration'; import { SettingsTarget } from 'vs/workbench/contrib/preferences/browser/preferencesWidgets'; import { ITOCEntry, knownAcronyms, knownTermMappings, tocData } from 'vs/workbench/contrib/preferences/browser/settingsLayout'; @@ -211,22 +210,29 @@ export class SettingsTreeSettingElement extends SettingsTreeElement { let displayValue = isConfigured ? inspected[targetSelector] : inspected.defaultValue; const overriddenScopeList: string[] = []; - if (targetSelector !== 'workspaceValue' && typeof inspected.workspaceValue !== 'undefined') { - overriddenScopeList.push(localize('workspace', "Workspace")); + if ((languageSelector || targetSelector !== 'workspaceValue') && typeof inspected.workspaceValue !== 'undefined') { + overriddenScopeList.push('workspace:'); } - - if (targetSelector !== 'userRemoteValue' && typeof inspected.userRemoteValue !== 'undefined') { - overriddenScopeList.push(localize('remote', "Remote")); + if ((languageSelector || targetSelector !== 'userRemoteValue') && typeof inspected.userRemoteValue !== 'undefined') { + overriddenScopeList.push('remote:'); } - - if (targetSelector !== 'userLocalValue' && typeof inspected.userLocalValue !== 'undefined') { - overriddenScopeList.push(localize('user', "User")); + if ((languageSelector || targetSelector !== 'userLocalValue') && typeof inspected.userLocalValue !== 'undefined') { + overriddenScopeList.push('user:'); } if (inspected.overrideIdentifiers) { for (const overrideIdentifier of inspected.overrideIdentifiers) { const inspectedOverride = inspectedLanguageOverrides.get(overrideIdentifier); if (inspectedOverride) { + if ((languageSelector !== overrideIdentifier || targetSelector !== 'workspaceValue') && typeof inspectedOverride.workspace?.override !== 'undefined') { + overriddenScopeList.push(`workspace:${overrideIdentifier}`); + } + if ((languageSelector !== overrideIdentifier || targetSelector !== 'userRemoteValue') && typeof inspectedOverride.userRemote?.override !== 'undefined') { + overriddenScopeList.push(`remote:${overrideIdentifier}`); + } + if ((languageSelector !== overrideIdentifier || targetSelector !== 'userLocalValue') && typeof inspectedOverride.userLocal?.override !== 'undefined') { + overriddenScopeList.push(`user:${overrideIdentifier}`); + } this.languageOverrideValues.set(overrideIdentifier, inspectedOverride); } } From fd199ec5ce8114561567be63571289bee3062d76 Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Thu, 23 Jun 2022 11:39:56 -0700 Subject: [PATCH 2/7] Create new section for overridden defaults --- .../preferences/browser/settingsEditor2.ts | 3 +- .../settingsEditorSettingIndicators.ts | 52 ++++++++++++------- .../preferences/browser/settingsTreeModels.ts | 26 ++++++---- 3 files changed, 52 insertions(+), 29 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts b/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts index 4187857031f2a..3412eac14fc7b 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts @@ -872,10 +872,9 @@ export class SettingsEditor2 extends EditorPane { })); this._register(this.settingRenderers.onApplyFilter((filter) => this.applyFilter(filter))); this._register(this.settingRenderers.onDidClickOverrideElement((element: ISettingOverrideClickEvent) => { + this.removeLanguageFilters(); if (element.language) { this.applyFilter(`@${LANGUAGE_SETTING_TAG}${element.language}`); - } else { - this.removeLanguageFilters(); } if (element.scope === 'workspace') { diff --git a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts index afb9973d336d1..99d25faa16415 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts @@ -114,11 +114,9 @@ export class SettingsTreeIndicatorsLabel { const [scope, language] = completeScope.split(':'); const localizedScope = scope === 'user' ? localize('user', "User") : scope === 'workspace' ? - localize('workspace', "Workspace") : - localize('remote', "Remote"); - if (language.length) { - const languageName = this.languageService.getLanguageName(language) ?? language; - return `${localizedScope} [${languageName}]`; + localize('workspace', "Workspace") : localize('remote', "Remote"); + if (language) { + return `${this.languageService.getLanguageName(language)} > ${localizedScope}`; } return localizedScope; } @@ -126,14 +124,14 @@ export class SettingsTreeIndicatorsLabel { updateScopeOverrides(element: SettingsTreeSettingElement, elementDisposables: DisposableStore, onDidClickOverrideElement: Emitter) { this.scopeOverridesElement.innerText = ''; this.scopeOverridesElement.style.display = 'none'; - if (element.overriddenScopeList.length) { + if (element.overriddenScopeList.length || element.overriddenDefaultsLanguageList.length) { this.scopeOverridesElement.style.display = 'inline'; - if (element.overriddenScopeList.length === 1) { + if (element.overriddenScopeList.length === 1 && !element.overriddenDefaultsLanguageList.length) { // Just show all the text in the label. const prefaceText = element.isConfigured ? localize('alsoConfiguredIn', "Also modified in") : localize('configuredIn', "Modified in"); - this.scopeOverridesLabel.text = `${prefaceText}: `; + this.scopeOverridesLabel.text = `${prefaceText} `; const firstScope = element.overriddenScopeList[0]; const view = DOM.append(this.scopeOverridesElement, $('a.modified-scope', undefined, this.getInlineScopeDisplayText(firstScope))); @@ -156,15 +154,33 @@ export class SettingsTreeIndicatorsLabel { localize('configuredElsewhere', "Modified elsewhere"); this.scopeOverridesLabel.text = scopeOverridesLabelText; - const prefaceText = element.isConfigured ? - localize('alsoModifiedInScopes', "The setting has also been modified in the following scopes:") : - localize('modifiedInScopes', "The setting has been modified in the following scopes:"); - let contentMarkdownString = prefaceText; - let contentFallback = prefaceText; - for (const scope of element.overriddenScopeList) { - const scopeDisplayText = this.getInlineScopeDisplayText(scope); - contentMarkdownString += `\n- [${scopeDisplayText}](${encodeURIComponent(scope)})`; - contentFallback += `\n• ${scopeDisplayText}`; + let contentMarkdownString = ''; + let contentFallback = ''; + if (element.overriddenScopeList.length) { + const prefaceText = element.isConfigured ? + localize('alsoModifiedInScopes', "The setting has also been modified in the following scopes:") : + localize('modifiedInScopes', "The setting has been modified in the following scopes:"); + contentMarkdownString = prefaceText; + contentFallback = prefaceText; + for (const scope of element.overriddenScopeList) { + const scopeDisplayText = this.getInlineScopeDisplayText(scope); + contentMarkdownString += `\n- [${scopeDisplayText}](${encodeURIComponent(scope)})`; + contentFallback += `\n• ${scopeDisplayText}`; + } + } + if (element.overriddenDefaultsLanguageList.length) { + if (contentMarkdownString) { + contentMarkdownString += `\n\n`; + contentFallback += `\n\n`; + } + const prefaceText = localize('hasDefaultOverridesForLanguages', "The following languages have default overrides:"); + contentMarkdownString += prefaceText; + contentFallback += prefaceText; + for (const language of element.overriddenDefaultsLanguageList) { + const scopeDisplayText = this.languageService.getLanguageName(language); + contentMarkdownString += `\n- [${scopeDisplayText}](${encodeURIComponent(`default:${language}`)})`; + contentFallback += `\n• ${scopeDisplayText}`; + } } const content: ITooltipMarkdownString = { markdown: { @@ -183,7 +199,7 @@ export class SettingsTreeIndicatorsLabel { scope: scope as ScopeString, language }); - hover!.dispose(); + hover!.hide(); } }; hover = setupCustomHover(this.hoverDelegate, this.scopeOverridesElement, content, options); diff --git a/src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts b/src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts index 6c3966a249f31..8f7f3f598b88d 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts @@ -152,6 +152,7 @@ export class SettingsTreeSettingElement extends SettingsTreeElement { tags?: Set; overriddenScopeList: string[] = []; + overriddenDefaultsLanguageList: string[] = []; /** * For each language that contributes setting values or default overrides, we can see those values here. @@ -210,6 +211,7 @@ export class SettingsTreeSettingElement extends SettingsTreeElement { let displayValue = isConfigured ? inspected[targetSelector] : inspected.defaultValue; const overriddenScopeList: string[] = []; + const overriddenDefaultsLanguageList: string[] = []; if ((languageSelector || targetSelector !== 'workspaceValue') && typeof inspected.workspaceValue !== 'undefined') { overriddenScopeList.push('workspace:'); } @@ -224,19 +226,26 @@ export class SettingsTreeSettingElement extends SettingsTreeElement { for (const overrideIdentifier of inspected.overrideIdentifiers) { const inspectedOverride = inspectedLanguageOverrides.get(overrideIdentifier); if (inspectedOverride) { - if ((languageSelector !== overrideIdentifier || targetSelector !== 'workspaceValue') && typeof inspectedOverride.workspace?.override !== 'undefined') { - overriddenScopeList.push(`workspace:${overrideIdentifier}`); - } - if ((languageSelector !== overrideIdentifier || targetSelector !== 'userRemoteValue') && typeof inspectedOverride.userRemote?.override !== 'undefined') { - overriddenScopeList.push(`remote:${overrideIdentifier}`); - } - if ((languageSelector !== overrideIdentifier || targetSelector !== 'userLocalValue') && typeof inspectedOverride.userLocal?.override !== 'undefined') { - overriddenScopeList.push(`user:${overrideIdentifier}`); + if (this.languageService.isRegisteredLanguageId(overrideIdentifier)) { + if (languageSelector !== overrideIdentifier && typeof inspectedOverride.default?.override !== 'undefined') { + overriddenDefaultsLanguageList.push(overrideIdentifier); + } + if ((languageSelector !== overrideIdentifier || targetSelector !== 'workspaceValue') && typeof inspectedOverride.workspace?.override !== 'undefined') { + overriddenScopeList.push(`workspace:${overrideIdentifier}`); + } + if ((languageSelector !== overrideIdentifier || targetSelector !== 'userRemoteValue') && typeof inspectedOverride.userRemote?.override !== 'undefined') { + overriddenScopeList.push(`remote:${overrideIdentifier}`); + } + if ((languageSelector !== overrideIdentifier || targetSelector !== 'userLocalValue') && typeof inspectedOverride.userLocal?.override !== 'undefined') { + overriddenScopeList.push(`user:${overrideIdentifier}`); + } } this.languageOverrideValues.set(overrideIdentifier, inspectedOverride); } } } + this.overriddenScopeList = overriddenScopeList; + this.overriddenDefaultsLanguageList = overriddenDefaultsLanguageList; // The user might have added, removed, or modified a language filter, // so we reset the default value source to the non-language-specific default value source for now. @@ -286,7 +295,6 @@ export class SettingsTreeSettingElement extends SettingsTreeElement { } } - this.overriddenScopeList = overriddenScopeList; if (this.setting.description.length > SettingsTreeSettingElement.MAX_DESC_LINES) { const truncatedDescLines = this.setting.description.slice(0, SettingsTreeSettingElement.MAX_DESC_LINES); truncatedDescLines.push('[...]'); From d6fd5ebccbc6a7df9acf604862f44ee717c36121 Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Thu, 23 Jun 2022 14:39:21 -0700 Subject: [PATCH 3/7] Improve hover dispose behaviour --- .../browser/settingsEditorSettingIndicators.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts index 99d25faa16415..44b67b7a6eca2 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts @@ -38,6 +38,8 @@ export class SettingsTreeIndicatorsLabel { private syncIgnoredElement: HTMLElement; private defaultOverrideIndicatorElement: HTMLElement; private hoverDelegate: IHoverDelegate; + private hover: ICustomHover | undefined; + private currentHoverMarkdownString: string | undefined; constructor( container: HTMLElement, @@ -127,6 +129,8 @@ export class SettingsTreeIndicatorsLabel { if (element.overriddenScopeList.length || element.overriddenDefaultsLanguageList.length) { this.scopeOverridesElement.style.display = 'inline'; if (element.overriddenScopeList.length === 1 && !element.overriddenDefaultsLanguageList.length) { + this.hover?.dispose(); + // Just show all the text in the label. const prefaceText = element.isConfigured ? localize('alsoConfiguredIn', "Also modified in") : @@ -190,7 +194,6 @@ export class SettingsTreeIndicatorsLabel { }, markdownNotSupportedFallback: contentFallback }; - let hover: ICustomHover | undefined = undefined; const options: IUpdatableHoverOptions = { linkHandler: (url: string) => { const [scope, language] = decodeURIComponent(url).split(':'); @@ -199,10 +202,14 @@ export class SettingsTreeIndicatorsLabel { scope: scope as ScopeString, language }); - hover!.hide(); + this.hover!.hide(); } }; - hover = setupCustomHover(this.hoverDelegate, this.scopeOverridesElement, content, options); + if (this.currentHoverMarkdownString !== contentMarkdownString) { + this.hover?.dispose(); + this.hover = setupCustomHover(this.hoverDelegate, this.scopeOverridesElement, content, options); + this.currentHoverMarkdownString = contentMarkdownString; + } } } this.render(); From d909951a2e66166887f0a2cd9546e96a5539b065 Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Fri, 24 Jun 2022 10:38:51 -0700 Subject: [PATCH 4/7] Improve accessibility text --- .../settingsEditorSettingIndicators.ts | 28 ++++++++++++++++--- .../preferences/browser/settingsTree.ts | 8 ++++-- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts index 44b67b7a6eca2..c82450f98bf3a 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts @@ -168,7 +168,7 @@ export class SettingsTreeIndicatorsLabel { contentFallback = prefaceText; for (const scope of element.overriddenScopeList) { const scopeDisplayText = this.getInlineScopeDisplayText(scope); - contentMarkdownString += `\n- [${scopeDisplayText}](${encodeURIComponent(scope)})`; + contentMarkdownString += `\n- ${scopeDisplayText}`; contentFallback += `\n• ${scopeDisplayText}`; } } @@ -190,7 +190,7 @@ export class SettingsTreeIndicatorsLabel { markdown: { value: contentMarkdownString, isTrusted: false, - supportHtml: false + supportHtml: true }, markdownNotSupportedFallback: contentFallback }; @@ -240,14 +240,26 @@ function getDefaultValueSourceToDisplay(element: SettingsTreeSettingElement): st return sourceToDisplay; } -export function getIndicatorsLabelAriaLabel(element: SettingsTreeSettingElement, configurationService: IConfigurationService): string { +function getAccessibleScopeDisplayText(completeScope: string, languageService: ILanguageService): string { + const [scope, language] = completeScope.split(':'); + const localizedScope = scope === 'user' ? + localize('user', "User") : scope === 'workspace' ? + localize('workspace', "Workspace") : localize('remote', "Remote"); + if (language) { + return localize('modifiedInScopeForLanguage', "the {0} scope for {1}", localizedScope, languageService.getLanguageName(language)); + } + return localizedScope; +} + +export function getIndicatorsLabelAriaLabel(element: SettingsTreeSettingElement, configurationService: IConfigurationService, languageService: ILanguageService): string { const ariaLabelSections: string[] = []; // Add other overrides text const otherOverridesStart = element.isConfigured ? localize('alsoConfiguredIn', "Also modified in") : localize('configuredIn', "Modified in"); - const otherOverridesList = element.overriddenScopeList.join(', '); + const otherOverridesList = element.overriddenScopeList + .map(scope => getAccessibleScopeDisplayText(scope, languageService)).join(', '); if (element.overriddenScopeList.length) { ariaLabelSections.push(`${otherOverridesStart} ${otherOverridesList}`); } @@ -264,6 +276,14 @@ export function getIndicatorsLabelAriaLabel(element: SettingsTreeSettingElement, ariaLabelSections.push(localize('defaultOverriddenDetails', "Default setting value overridden by {0}", sourceToDisplay)); } + // Add text about default values being overridden in other languages + const otherLanguageOverridesStart = localize('defaultOverriddenListPreface', "The default value of the setting has also been overridden for the following languages:"); + const otherLanguageOverridesList = element.overriddenDefaultsLanguageList + .map(language => languageService.getLanguageName(language)).join(', '); + if (element.overriddenDefaultsLanguageList.length) { + ariaLabelSections.push(`${otherLanguageOverridesStart} ${otherLanguageOverridesList}`); + } + const ariaLabel = ariaLabelSections.join('. '); return ariaLabel; } diff --git a/src/vs/workbench/contrib/preferences/browser/settingsTree.ts b/src/vs/workbench/contrib/preferences/browser/settingsTree.ts index fcf032fd281aa..d1a47ae7d1508 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsTree.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsTree.ts @@ -63,6 +63,7 @@ import { MarkdownRenderer } from 'vs/editor/contrib/markdownRenderer/browser/mar import { IExtensionService } from 'vs/workbench/services/extensions/common/extensions'; import { focusedRowBackground, focusedRowBorder, rowHoverBackground, settingsHeaderForeground, settingsNumberInputBackground, settingsNumberInputBorder, settingsNumberInputForeground, settingsSelectBackground, settingsSelectBorder, settingsSelectForeground, settingsSelectListBorder, settingsTextInputBackground, settingsTextInputBorder, settingsTextInputForeground } from 'vs/workbench/contrib/preferences/common/settingsEditorColorRegistry'; import { getIndicatorsLabelAriaLabel, ISettingOverrideClickEvent, SettingsTreeIndicatorsLabel } from 'vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators'; +import { ILanguageService } from 'vs/editor/common/languages/language'; const $ = DOM.$; @@ -2287,7 +2288,7 @@ export class NonCollapsibleObjectTreeModel extends ObjectTreeModel { } class SettingsTreeAccessibilityProvider implements IListAccessibilityProvider { - constructor(private readonly configurationService: IConfigurationService) { + constructor(private readonly configurationService: IConfigurationService, private readonly languageService: ILanguageService) { } getAriaLabel(element: SettingsTreeElement) { @@ -2300,7 +2301,7 @@ class SettingsTreeAccessibilityProvider implements IListAccessibilityProvider { @IKeybindingService keybindingService: IKeybindingService, @IAccessibilityService accessibilityService: IAccessibilityService, @IInstantiationService instantiationService: IInstantiationService, + @ILanguageService languageService: ILanguageService ) { super('SettingsTree', container, new SettingsTreeDelegate(), @@ -2346,7 +2348,7 @@ export class SettingsTree extends WorkbenchObjectTree { return e.id; } }, - accessibilityProvider: new SettingsTreeAccessibilityProvider(configurationService), + accessibilityProvider: new SettingsTreeAccessibilityProvider(configurationService, languageService), styleController: id => new DefaultStyleController(DOM.createStyleSheet(container), id), filter: instantiationService.createInstance(SettingsTreeFilter, viewState), smoothScrolling: configurationService.getValue('workbench.list.smoothScrolling'), From 1ec0b3ed7e87962241b11b09ee2c324ff0d07de1 Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Fri, 24 Jun 2022 11:30:48 -0700 Subject: [PATCH 5/7] Add feature flag and improve dispose --- .../settingsEditorSettingIndicators.ts | 54 ++++++++++++------- .../preferences/browser/settingsTree.ts | 4 +- .../contrib/preferences/common/preferences.ts | 1 + 3 files changed, 39 insertions(+), 20 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts index c82450f98bf3a..5350f9d29ae6b 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts @@ -9,13 +9,14 @@ import { IHoverDelegate, IHoverDelegateOptions } from 'vs/base/browser/ui/iconLa import { ICustomHover, ITooltipMarkdownString, IUpdatableHoverOptions, setupCustomHover } from 'vs/base/browser/ui/iconLabel/iconLabelHover'; import { SimpleIconLabel } from 'vs/base/browser/ui/iconLabel/simpleIconLabel'; import { Emitter } from 'vs/base/common/event'; -import { DisposableStore } from 'vs/base/common/lifecycle'; +import { IDisposable, DisposableStore } from 'vs/base/common/lifecycle'; import { ILanguageService } from 'vs/editor/common/languages/language'; import { localize } from 'vs/nls'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; import { getIgnoredSettings } from 'vs/platform/userDataSync/common/settingsMerge'; import { getDefaultIgnoredSettings, IUserDataSyncEnablementService } from 'vs/platform/userDataSync/common/userDataSync'; import { SettingsTreeSettingElement } from 'vs/workbench/contrib/preferences/browser/settingsTreeModels'; +import { MODIFIED_INDICATOR_USE_INLINE_ONLY } from 'vs/workbench/contrib/preferences/common/preferences'; import { IHoverService } from 'vs/workbench/services/hover/browser/hover'; const $ = DOM.$; @@ -31,7 +32,7 @@ export interface ISettingOverrideClickEvent { /** * Renders the indicators next to a setting, such as "Also Modified In". */ -export class SettingsTreeIndicatorsLabel { +export class SettingsTreeIndicatorsLabel implements IDisposable { private indicatorsContainerElement: HTMLElement; private scopeOverridesElement: HTMLElement; private scopeOverridesLabel: SimpleIconLabel; @@ -123,12 +124,19 @@ export class SettingsTreeIndicatorsLabel { return localizedScope; } + dispose() { + this.hover?.dispose(); + } + updateScopeOverrides(element: SettingsTreeSettingElement, elementDisposables: DisposableStore, onDidClickOverrideElement: Emitter) { this.scopeOverridesElement.innerText = ''; this.scopeOverridesElement.style.display = 'none'; if (element.overriddenScopeList.length || element.overriddenDefaultsLanguageList.length) { - this.scopeOverridesElement.style.display = 'inline'; - if (element.overriddenScopeList.length === 1 && !element.overriddenDefaultsLanguageList.length) { + if ((MODIFIED_INDICATOR_USE_INLINE_ONLY && element.overriddenScopeList.length) || + (element.overriddenScopeList.length === 1 && !element.overriddenDefaultsLanguageList.length)) { + // Render inline if we have the flag and there are scope overrides to render, + // or if there is only one scope override to render and no language overrides. + this.scopeOverridesElement.style.display = 'inline'; this.hover?.dispose(); // Just show all the text in the label. @@ -137,21 +145,29 @@ export class SettingsTreeIndicatorsLabel { localize('configuredIn', "Modified in"); this.scopeOverridesLabel.text = `${prefaceText} `; - const firstScope = element.overriddenScopeList[0]; - const view = DOM.append(this.scopeOverridesElement, $('a.modified-scope', undefined, this.getInlineScopeDisplayText(firstScope))); - elementDisposables.add( - DOM.addStandardDisposableListener(view, DOM.EventType.CLICK, (e: IMouseEvent) => { - const [scope, language] = firstScope.split(':'); - onDidClickOverrideElement.fire({ - settingKey: element.setting.key, - scope: scope as ScopeString, - language - }); - e.preventDefault(); - e.stopPropagation(); - })); - } else { - // Show most of the text in a custom hover. + for (let i = 0; i < element.overriddenScopeList.length; i++) { + const overriddenScope = element.overriddenScopeList[i]; + const view = DOM.append(this.scopeOverridesElement, $('a.modified-scope', undefined, this.getInlineScopeDisplayText(overriddenScope))); + if (i !== element.overriddenScopeList.length - 1) { + DOM.append(this.scopeOverridesElement, $('span.comma', undefined, ', ')); + } + elementDisposables.add( + DOM.addStandardDisposableListener(view, DOM.EventType.CLICK, (e: IMouseEvent) => { + const [scope, language] = overriddenScope.split(':'); + onDidClickOverrideElement.fire({ + settingKey: element.setting.key, + scope: scope as ScopeString, + language + }); + e.preventDefault(); + e.stopPropagation(); + })); + } + } else if (!MODIFIED_INDICATOR_USE_INLINE_ONLY) { + // Even if the check above fails, we want to + // show the text in a custom hover only if + // the feature flag isn't on. + this.scopeOverridesElement.style.display = 'inline'; let scopeOverridesLabelText = '$(info) '; scopeOverridesLabelText += element.isConfigured ? localize('alsoConfiguredElsewhere', "Also modified elsewhere") : diff --git a/src/vs/workbench/contrib/preferences/browser/settingsTree.ts b/src/vs/workbench/contrib/preferences/browser/settingsTree.ts index d1a47ae7d1508..16dd1bb7a51c6 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsTree.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsTree.ts @@ -779,6 +779,8 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre _container.classList.add('setting-item'); _container.classList.add('setting-item-' + typeClass); + const toDispose = new DisposableStore(); + const container = DOM.append(_container, $(AbstractSettingRenderer.CONTENTS_SELECTOR)); container.classList.add('settings-row-inner-container'); const titleElement = DOM.append(container, $('.setting-item-title')); @@ -787,6 +789,7 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre const labelElementContainer = DOM.append(labelCategoryContainer, $('span.setting-item-label')); const labelElement = new SimpleIconLabel(labelElementContainer); const indicatorsLabel = this._instantiationService.createInstance(SettingsTreeIndicatorsLabel, titleElement); + toDispose.add(indicatorsLabel); const descriptionElement = DOM.append(container, $('.setting-item-description')); const modifiedIndicatorElement = DOM.append(container, $('.setting-item-modified-indicator')); @@ -797,7 +800,6 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre const deprecationWarningElement = DOM.append(container, $('.setting-item-deprecation-message')); - const toDispose = new DisposableStore(); const policyWarningElement = this.renderPolicyLabel(container, toDispose); const toolbarContainer = DOM.append(container, $('.setting-toolbar-container')); diff --git a/src/vs/workbench/contrib/preferences/common/preferences.ts b/src/vs/workbench/contrib/preferences/common/preferences.ts index 82ca2f5de3f2d..71d7bc72b07f0 100644 --- a/src/vs/workbench/contrib/preferences/common/preferences.ts +++ b/src/vs/workbench/contrib/preferences/common/preferences.ts @@ -85,3 +85,4 @@ export const REQUIRE_TRUSTED_WORKSPACE_SETTING_TAG = 'requireTrustedWorkspace'; export const KEYBOARD_LAYOUT_OPEN_PICKER = 'workbench.action.openKeyboardLayoutPicker'; export const ENABLE_LANGUAGE_FILTER = true; +export const MODIFIED_INDICATOR_USE_INLINE_ONLY = true; From 082d7ba2e4c21ae8ea75ee923f3c0258b7dcd295 Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Fri, 24 Jun 2022 12:23:55 -0700 Subject: [PATCH 6/7] Disable HTML, toggle feature flag --- .../settingsEditorSettingIndicators.ts | 29 ++++++++++++------- .../contrib/preferences/common/preferences.ts | 2 +- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts index 5350f9d29ae6b..d6b7be92fd04c 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts @@ -40,7 +40,6 @@ export class SettingsTreeIndicatorsLabel implements IDisposable { private defaultOverrideIndicatorElement: HTMLElement; private hoverDelegate: IHoverDelegate; private hover: ICustomHover | undefined; - private currentHoverMarkdownString: string | undefined; constructor( container: HTMLElement, @@ -184,7 +183,7 @@ export class SettingsTreeIndicatorsLabel implements IDisposable { contentFallback = prefaceText; for (const scope of element.overriddenScopeList) { const scopeDisplayText = this.getInlineScopeDisplayText(scope); - contentMarkdownString += `\n- ${scopeDisplayText}`; + contentMarkdownString += `\n- [${scopeDisplayText}](${encodeURIComponent(scope)} "${getAccessibleScopeDisplayText(scope, this.languageService)}")`; contentFallback += `\n• ${scopeDisplayText}`; } } @@ -198,7 +197,7 @@ export class SettingsTreeIndicatorsLabel implements IDisposable { contentFallback += prefaceText; for (const language of element.overriddenDefaultsLanguageList) { const scopeDisplayText = this.languageService.getLanguageName(language); - contentMarkdownString += `\n- [${scopeDisplayText}](${encodeURIComponent(`default:${language}`)})`; + contentMarkdownString += `\n- [${scopeDisplayText}](${encodeURIComponent(`default:${language}`)} "${scopeDisplayText}")`; contentFallback += `\n• ${scopeDisplayText}`; } } @@ -206,7 +205,7 @@ export class SettingsTreeIndicatorsLabel implements IDisposable { markdown: { value: contentMarkdownString, isTrusted: false, - supportHtml: true + supportHtml: false }, markdownNotSupportedFallback: contentFallback }; @@ -221,11 +220,8 @@ export class SettingsTreeIndicatorsLabel implements IDisposable { this.hover!.hide(); } }; - if (this.currentHoverMarkdownString !== contentMarkdownString) { - this.hover?.dispose(); - this.hover = setupCustomHover(this.hoverDelegate, this.scopeOverridesElement, content, options); - this.currentHoverMarkdownString = contentMarkdownString; - } + this.hover?.dispose(); + this.hover = setupCustomHover(this.hoverDelegate, this.scopeOverridesElement, content, options); } } this.render(); @@ -262,7 +258,18 @@ function getAccessibleScopeDisplayText(completeScope: string, languageService: I localize('user', "User") : scope === 'workspace' ? localize('workspace', "Workspace") : localize('remote', "Remote"); if (language) { - return localize('modifiedInScopeForLanguage', "the {0} scope for {1}", localizedScope, languageService.getLanguageName(language)); + return localize('modifiedInScopeForLanguage', "The {0} scope for {1}", localizedScope, languageService.getLanguageName(language)); + } + return localizedScope; +} + +function getAccessibleScopeDisplayMidSentenceText(completeScope: string, languageService: ILanguageService): string { + const [scope, language] = completeScope.split(':'); + const localizedScope = scope === 'user' ? + localize('user', "User") : scope === 'workspace' ? + localize('workspace', "Workspace") : localize('remote', "Remote"); + if (language) { + return localize('modifiedInScopeForLanguageMidSentence', "the {0} scope for {1}", localizedScope.toLowerCase(), languageService.getLanguageName(language)); } return localizedScope; } @@ -275,7 +282,7 @@ export function getIndicatorsLabelAriaLabel(element: SettingsTreeSettingElement, localize('alsoConfiguredIn', "Also modified in") : localize('configuredIn', "Modified in"); const otherOverridesList = element.overriddenScopeList - .map(scope => getAccessibleScopeDisplayText(scope, languageService)).join(', '); + .map(scope => getAccessibleScopeDisplayMidSentenceText(scope, languageService)).join(', '); if (element.overriddenScopeList.length) { ariaLabelSections.push(`${otherOverridesStart} ${otherOverridesList}`); } diff --git a/src/vs/workbench/contrib/preferences/common/preferences.ts b/src/vs/workbench/contrib/preferences/common/preferences.ts index 71d7bc72b07f0..39a4cc0d044f4 100644 --- a/src/vs/workbench/contrib/preferences/common/preferences.ts +++ b/src/vs/workbench/contrib/preferences/common/preferences.ts @@ -85,4 +85,4 @@ export const REQUIRE_TRUSTED_WORKSPACE_SETTING_TAG = 'requireTrustedWorkspace'; export const KEYBOARD_LAYOUT_OPEN_PICKER = 'workbench.action.openKeyboardLayoutPicker'; export const ENABLE_LANGUAGE_FILTER = true; -export const MODIFIED_INDICATOR_USE_INLINE_ONLY = true; +export const MODIFIED_INDICATOR_USE_INLINE_ONLY = false; From 2c5b609e7e95ebcc921ce5d71397892494f2c90a Mon Sep 17 00:00:00 2001 From: Raymond Zhao <7199958+rzhao271@users.noreply.github.com> Date: Fri, 24 Jun 2022 14:08:04 -0700 Subject: [PATCH 7/7] Remove check so custom hover makes sense --- .../preferences/browser/settingsEditorSettingIndicators.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts index d6b7be92fd04c..44bb7b7afd874 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts @@ -243,7 +243,7 @@ function getDefaultValueSourceToDisplay(element: SettingsTreeSettingElement): st let sourceToDisplay: string | undefined; const defaultValueSource = element.defaultValueSource; if (defaultValueSource) { - if (typeof defaultValueSource !== 'string' && defaultValueSource.id !== element.setting.extensionInfo?.id) { + if (typeof defaultValueSource !== 'string') { sourceToDisplay = defaultValueSource.displayName ?? defaultValueSource.id; } else if (typeof defaultValueSource === 'string') { sourceToDisplay = defaultValueSource;