Skip to content

Fix dirty setting update logic #58926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Sep 21, 2018
Merged

Fix dirty setting update logic #58926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
471204f
Fix dirty setting update logic
guywaldman Sep 19, 2018
b44b373
Add test
guywaldman Sep 19, 2018
6855b54
Remove redundant async
guywaldman Sep 20, 2018
6bc7ce6
Change behavior for non-text inputs
guywaldman Sep 20, 2018
26f3dcc
Fix negated logic
guywaldman Sep 21, 2018
b1b3db9
Stop updating setting on blur
guywaldman Sep 21, 2018
83dccc6
Scope enum for settings more closely
guywaldman Sep 21, 2018
b5865f6
Remove Delayer.fire
guywaldman Sep 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions src/vs/workbench/parts/preferences/browser/settingsEditor2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { Separator } from 'vs/base/browser/ui/actionbar/actionbar';
import { ToolBar } from 'vs/base/browser/ui/toolbar/toolbar';
import { Action } from 'vs/base/common/actions';
import * as arrays from 'vs/base/common/arrays';
import { isArray } from 'vs/base/common/types';
import { Delayer, ThrottledDelayer } from 'vs/base/common/async';
import { CancellationToken, CancellationTokenSource } from 'vs/base/common/cancellation';
import * as collections from 'vs/base/common/collections';
Expand Down Expand Up @@ -43,7 +44,7 @@ import { ISettingsEditorViewState, MODIFIED_SETTING_TAG, ONLINE_SERVICES_SETTING
import { settingsTextInputBorder } from 'vs/workbench/parts/preferences/browser/settingsWidgets';
import { TOCRenderer, TOCTree, TOCTreeModel } from 'vs/workbench/parts/preferences/browser/tocTree';
import { CONTEXT_SETTINGS_EDITOR, CONTEXT_SETTINGS_SEARCH_FOCUS, CONTEXT_TOC_ROW_FOCUS, IPreferencesSearchService, ISearchProvider, SETTINGS_EDITOR_COMMAND_SHOW_CONTEXT_MENU } from 'vs/workbench/parts/preferences/common/preferences';
import { IPreferencesService, ISearchResult, ISettingsEditorModel, ISettingsEditorOptions, SettingsEditorOptions } from 'vs/workbench/services/preferences/common/preferences';
import { IPreferencesService, ISearchResult, ISettingsEditorModel, ISettingsEditorOptions, SettingsEditorOptions, SettingValueType } from 'vs/workbench/services/preferences/common/preferences';
import { SettingsEditor2Input } from 'vs/workbench/services/preferences/common/preferencesEditorInput';
import { Settings2EditorModel } from 'vs/workbench/services/preferences/common/preferencesModels';

Expand All @@ -53,11 +54,21 @@ export class SettingsEditor2 extends BaseEditor {

public static readonly ID: string = 'workbench.editor.settings2';
private static NUM_INSTANCES: number = 0;
private static SETTING_UPDATE_FAST_DEBOUNCE: number = 200;
private static SETTING_UPDATE_SLOW_DEBOUNCE: number = 1000;

private static readonly SUGGESTIONS: string[] = [
'@modified', '@tag:usesOnlineServices'
];

private static shouldSettingUpdateFast(type: SettingValueType | SettingValueType[]): boolean {
if (isArray(type)) {
// nullable integer/number or complex
return false;
}
return type === SettingValueType.Enum || type === SettingValueType.Complex;
}

private defaultSettingsEditorModel: Settings2EditorModel;

private rootElement: HTMLElement;
Expand Down Expand Up @@ -88,7 +99,8 @@ export class SettingsEditor2 extends BaseEditor {
private delayRefreshOnLayout: Delayer<void>;
private lastLayedoutWidth: number;

private settingUpdateDelayer: Delayer<void>;
private settingFastUpdateDelayer: Delayer<void>;
private settingSlowUpdateDelayer: Delayer<void>;
private pendingSettingUpdate: { key: string, value: any };

private viewState: ISettingsEditorViewState;
Expand Down Expand Up @@ -126,7 +138,8 @@ export class SettingsEditor2 extends BaseEditor {
this.viewState = { settingsTarget: ConfigurationTarget.USER };
this.delayRefreshOnLayout = new Delayer(100);

this.settingUpdateDelayer = new Delayer<void>(200);
this.settingFastUpdateDelayer = new Delayer<void>(SettingsEditor2.SETTING_UPDATE_FAST_DEBOUNCE);
this.settingSlowUpdateDelayer = new Delayer<void>(SettingsEditor2.SETTING_UPDATE_SLOW_DEBOUNCE);

this.inSettingsEditorContextKey = CONTEXT_SETTINGS_EDITOR.bindTo(contextKeyService);
this.searchFocusContextKey = CONTEXT_SETTINGS_SEARCH_FOCUS.bindTo(contextKeyService);
Expand Down Expand Up @@ -542,7 +555,7 @@ export class SettingsEditor2 extends BaseEditor {
labelDiv.setAttribute('aria-label', '');

this.settingsTreeRenderer = this.instantiationService.createInstance(SettingsRenderer, this.settingsTreeContainer);
this._register(this.settingsTreeRenderer.onDidChangeSetting(e => this.onDidChangeSetting(e.key, e.value)));
this._register(this.settingsTreeRenderer.onDidChangeSetting(e => this.onDidChangeSetting(e.key, e.value, e.type)));
this._register(this.settingsTreeRenderer.onDidOpenSettings(settingKey => {
this.openSettingsFile(settingKey);
}));
Expand Down Expand Up @@ -580,15 +593,19 @@ export class SettingsEditor2 extends BaseEditor {
}
}

private onDidChangeSetting(key: string, value: any): void {
private onDidChangeSetting(key: string, value: any, type: SettingValueType | SettingValueType[]): void {
this.notifyNoSaveNeeded(false);

if (this.pendingSettingUpdate && this.pendingSettingUpdate.key !== key) {
this.updateChangedSetting(key, value);
}

this.pendingSettingUpdate = { key, value };
this.settingUpdateDelayer.trigger(() => this.updateChangedSetting(key, value));
if (SettingsEditor2.shouldSettingUpdateFast(type)) {
this.settingFastUpdateDelayer.trigger(() => this.updateChangedSetting(key, value));
} else {
this.settingSlowUpdateDelayer.trigger(() => this.updateChangedSetting(key, value));
}
}

private updateTreeScrollSync(): void {
Expand Down
10 changes: 6 additions & 4 deletions src/vs/workbench/parts/preferences/browser/settingsTree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import { ITOCEntry } from 'vs/workbench/parts/preferences/browser/settingsLayout
import { ISettingsEditorViewState, isExcludeSetting, settingKeyToDisplayFormat, SettingsTreeElement, SettingsTreeGroupElement, SettingsTreeNewExtensionsElement, SettingsTreeSettingElement } from 'vs/workbench/parts/preferences/browser/settingsTreeModels';
import { ExcludeSettingWidget, IExcludeDataItem, settingsHeaderForeground, settingsNumberInputBackground, settingsNumberInputBorder, settingsNumberInputForeground, settingsSelectBackground, settingsSelectBorder, settingsSelectForeground, settingsSelectListBorder, settingsTextInputBackground, settingsTextInputBorder, settingsTextInputForeground } from 'vs/workbench/parts/preferences/browser/settingsWidgets';
import { SETTINGS_EDITOR_COMMAND_SHOW_CONTEXT_MENU } from 'vs/workbench/parts/preferences/common/preferences';
import { ISetting, ISettingsGroup } from 'vs/workbench/services/preferences/common/preferences';
import { ISetting, ISettingsGroup, SettingValueType } from 'vs/workbench/services/preferences/common/preferences';

const $ = DOM.$;

Expand Down Expand Up @@ -331,6 +331,7 @@ const SETTINGS_GROUP_ELEMENT_TEMPLATE_ID = 'settings.group.template';
export interface ISettingChangeEvent {
key: string;
value: any; // undefined => reset/unconfigure
type: SettingValueType | SettingValueType[];
}

export interface ISettingLinkClickEvent {
Expand Down Expand Up @@ -384,7 +385,7 @@ export class SettingsRenderer implements ITreeRenderer {
this.settingActions = [
new Action('settings.resetSetting', localize('resetSettingLabel', "Reset Setting"), undefined, undefined, (context: SettingsTreeSettingElement) => {
if (context) {
this._onDidChangeSetting.fire({ key: context.setting.key, value: undefined });
this._onDidChangeSetting.fire({ key: context.setting.key, value: undefined, type: context.setting.type as SettingValueType });
}

return TPromise.wrap(null);
Expand Down Expand Up @@ -913,7 +914,8 @@ export class SettingsRenderer implements ITreeRenderer {

this._onDidChangeSetting.fire({
key: template.context.setting.key,
value: Object.keys(newValue).length === 0 ? undefined : sortKeys(newValue)
value: Object.keys(newValue).length === 0 ? undefined : sortKeys(newValue),
type: template.context.valueType
});
}
}));
Expand Down Expand Up @@ -1095,7 +1097,7 @@ export class SettingsRenderer implements ITreeRenderer {
}

private renderValue(element: SettingsTreeSettingElement, templateId: string, template: ISettingItemTemplate | ISettingBoolItemTemplate): void {
const onChange = value => this._onDidChangeSetting.fire({ key: element.setting.key, value });
const onChange = value => this._onDidChangeSetting.fire({ key: element.setting.key, value, type: template.context.valueType });
template.deprecationWarningElement.innerText = element.setting.deprecationMessage || '';

if (templateId === SETTINGS_ENUM_TEMPLATE_ID) {
Expand Down
30 changes: 15 additions & 15 deletions src/vs/workbench/parts/preferences/browser/settingsTreeModels.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { ConfigurationTarget, IConfigurationService } from 'vs/platform/configur
import { ConfigurationScope } from 'vs/platform/configuration/common/configurationRegistry';
import { SettingsTarget } from 'vs/workbench/parts/preferences/browser/preferencesWidgets';
import { ITOCEntry, knownAcronyms } from 'vs/workbench/parts/preferences/browser/settingsLayout';
import { IExtensionSetting, ISearchResult, ISetting } from 'vs/workbench/services/preferences/common/preferences';
import { IExtensionSetting, ISearchResult, ISetting, SettingValueType } from 'vs/workbench/services/preferences/common/preferences';

export const MODIFIED_SETTING_TAG = 'modified';
export const ONLINE_SERVICES_SETTING_TAG = 'usesOnlineServices';
Expand Down Expand Up @@ -99,7 +99,7 @@ export class SettingsTreeSettingElement extends SettingsTreeElement {
tags?: Set<string>;
overriddenScopeList: string[];
description: string;
valueType: 'enum' | 'string' | 'integer' | 'number' | 'boolean' | 'exclude' | 'complex' | 'nullable-integer' | 'nullable-number';
valueType: SettingValueType;

constructor(setting: ISetting, parent: SettingsTreeGroupElement, index: number, inspectResult: IInspectResult) {
super();
Expand Down Expand Up @@ -167,27 +167,27 @@ export class SettingsTreeSettingElement extends SettingsTreeElement {
this.description = this.setting.description.join('\n');

if (this.setting.enum && (!this.setting.type || settingTypeEnumRenderable(this.setting.type))) {
this.valueType = 'enum';
this.valueType = SettingValueType.Enum;
} else if (this.setting.type === 'string') {
this.valueType = 'string';
this.valueType = SettingValueType.String;
} else if (isExcludeSetting(this.setting)) {
this.valueType = 'exclude';
this.valueType = SettingValueType.Exclude;
} else if (this.setting.type === 'integer') {
this.valueType = 'integer';
this.valueType = SettingValueType.Integer;
} else if (this.setting.type === 'number') {
this.valueType = 'number';
this.valueType = SettingValueType.Number;
} else if (this.setting.type === 'boolean') {
this.valueType = 'boolean';
} else if (isArray(this.setting.type) && this.setting.type.indexOf('null') > -1 && this.setting.type.length === 2) {
if (this.setting.type.indexOf('integer') > -1) {
this.valueType = 'nullable-integer';
} else if (this.setting.type.indexOf('number') > -1) {
this.valueType = 'nullable-number';
this.valueType = SettingValueType.Boolean;
} else if (isArray(this.setting.type) && this.setting.type.indexOf(SettingValueType.Null) > -1 && this.setting.type.length === 2) {
if (this.setting.type.indexOf(SettingValueType.Integer) > -1) {
this.valueType = SettingValueType.NullableInteger;
} else if (this.setting.type.indexOf(SettingValueType.Number) > -1) {
this.valueType = SettingValueType.NullableNumber;
} else {
this.valueType = 'complex';
this.valueType = SettingValueType.Complex;
}
} else {
this.valueType = 'complex';
this.valueType = SettingValueType.Complex;
}
}

Expand Down
13 changes: 13 additions & 0 deletions src/vs/workbench/services/preferences/common/preferences.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,19 @@ import { IEditorGroup } from 'vs/workbench/services/group/common/editorGroupsSer
import { Settings2EditorModel } from 'vs/workbench/services/preferences/common/preferencesModels';
import { ConfigurationScope } from 'vs/platform/configuration/common/configurationRegistry';

export enum SettingValueType {
Null = 'null',
Enum = 'enum',
String = 'string',
Integer = 'integer',
Number = 'number',
Boolean = 'boolean',
Exclude = 'exclude',
Complex = 'complex',
NullableInteger = 'nullable-integer',
NullableNumber = 'nullable-number'
}

export interface ISettingsGroup {
id: string;
range: IRange;
Expand Down