From 09875353392fe46bba78cf191d7a21c7ce6fcad8 Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 24 Nov 2021 17:57:32 +0100 Subject: [PATCH 1/9] Update provider pending change logic Signed-off-by: Colin Grant --- .../src/browser/preferences/preference-provider.ts | 13 ++++++++----- .../abstract-resource-preference-provider.ts | 13 +++++++------ 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/packages/core/src/browser/preferences/preference-provider.ts b/packages/core/src/browser/preferences/preference-provider.ts index 17479c52251b2..fabbe37a62fef 100644 --- a/packages/core/src/browser/preferences/preference-provider.ts +++ b/packages/core/src/browser/preferences/preference-provider.ts @@ -75,9 +75,12 @@ export abstract class PreferenceProvider implements Disposable { } protected deferredChanges: PreferenceProviderDataChanges | undefined; - protected _pendingChanges: Promise = Promise.resolve(false); + protected _pendingChanges = new Deferred(); get pendingChanges(): Promise { - return this._pendingChanges; + if (this._pendingChanges.state !== 'unresolved') { + this._pendingChanges = new Deferred(); + } + return this._pendingChanges.promise; } /** @@ -94,7 +97,7 @@ export abstract class PreferenceProvider implements Disposable { this.mergePreferenceProviderDataChange(changes[preferenceName]); } } - return this._pendingChanges = this.fireDidPreferencesChanged(); + return this.fireDidPreferencesChanged(); } protected mergePreferenceProviderDataChange(change: PreferenceProviderDataChange): void { @@ -120,9 +123,9 @@ export abstract class PreferenceProvider implements Disposable { this.deferredChanges = undefined; if (changes && Object.keys(changes).length) { this.onDidPreferencesChangedEmitter.fire(changes); - return true; } - return false; + this._pendingChanges.resolve(true); + return true; }, 0); /** diff --git a/packages/preferences/src/browser/abstract-resource-preference-provider.ts b/packages/preferences/src/browser/abstract-resource-preference-provider.ts index 6b85d4c45590b..a6a292dd26035 100644 --- a/packages/preferences/src/browser/abstract-resource-preference-provider.ts +++ b/packages/preferences/src/browser/abstract-resource-preference-provider.ts @@ -71,9 +71,9 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi this.toDispose.push(reference); this.toDispose.push(Disposable.create(() => this.model = undefined)); - this.toDispose.push(this.model.onDidChangeContent(() => this.readPreferences())); - this.toDispose.push(this.model.onDirtyChanged(() => this.readPreferences())); - this.toDispose.push(this.model.onDidChangeValid(() => this.readPreferences())); + this.toDispose.push(this.model.onDidChangeContent(() => (console.log('READING BECAUSE...CONTENT'), this.readPreferences()))); + this.toDispose.push(this.model.onDirtyChanged(() => (console.log('READING BECAUSE...DIRTY'), this.readPreferences()))); + this.toDispose.push(this.model.onDidChangeValid(() => (console.log('READING BECAUSE...VALID'), this.readPreferences()))); this.toDispose.push(Disposable.create(() => this.reset())); } @@ -153,6 +153,9 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi forceMoveMarkers: false }); } + if (editOperations.length === 0) { + return true; + } await this.workspace.applyBackgroundEdit(this.model, editOperations); return await this.pendingChanges; } catch (e) { @@ -234,9 +237,7 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi } } - if (prefChanges.length > 0) { // do not emit the change event if the pref value is not changed - this.emitPreferencesChangedEvent(prefChanges); - } + this.emitPreferencesChangedEvent(prefChanges); } protected reset(): void { From d0d2ec488d4c1d6cb9149c06d953d2ec13020bbb Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 24 Nov 2021 18:11:03 +0100 Subject: [PATCH 2/9] extract edit generation Signed-off-by: Colin Grant --- .../abstract-resource-preference-provider.ts | 65 ++++++++++--------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/packages/preferences/src/browser/abstract-resource-preference-provider.ts b/packages/preferences/src/browser/abstract-resource-preference-provider.ts index a6a292dd26035..5f8d166b1948e 100644 --- a/packages/preferences/src/browser/abstract-resource-preference-provider.ts +++ b/packages/preferences/src/browser/abstract-resource-preference-provider.ts @@ -123,36 +123,7 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi return false; } try { - const content = this.model.getText().trim(); - if (!content && value === undefined) { - return true; - } - const textModel = this.model.textEditorModel; - const editOperations: monaco.editor.IIdentifiedSingleEditOperation[] = []; - if (path.length || value !== undefined) { - const { insertSpaces, tabSize, defaultEOL } = textModel.getOptions(); - for (const edit of jsoncparser.modify(content, path, value, { - formattingOptions: { - insertSpaces, - tabSize, - eol: defaultEOL === monaco.editor.DefaultEndOfLine.LF ? '\n' : '\r\n' - } - })) { - const start = textModel.getPositionAt(edit.offset); - const end = textModel.getPositionAt(edit.offset + edit.length); - editOperations.push({ - range: monaco.Range.fromPositions(start, end), - text: edit.content || null, - forceMoveMarkers: false - }); - } - } else { - editOperations.push({ - range: textModel.getFullModelRange(), - text: null, - forceMoveMarkers: false - }); - } + const editOperations = this.getEditOperations(path, value); if (editOperations.length === 0) { return true; } @@ -166,6 +137,40 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi } } + private getEditOperations(path: string[], value: any): monaco.editor.IIdentifiedSingleEditOperation[] { + const textModel = this.model!.textEditorModel; + const content = this.model!.getText().trim(); + // Everything is already undefined - no need for changes. + if (!content && value === undefined) { + return []; + } + // Delete the entire document. + if (!path.length && value === undefined) { + return [{ + range: textModel.getFullModelRange(), + text: null, + forceMoveMarkers: false + }]; + } + const { insertSpaces, tabSize, defaultEOL } = textModel.getOptions(); + const jsonCOptions = { + formattingOptions: { + insertSpaces, + tabSize, + eol: defaultEOL === monaco.editor.DefaultEndOfLine.LF ? '\n' : '\r\n' + } + }; + return jsoncparser.modify(content, path, value, jsonCOptions).map(edit => { + const start = textModel.getPositionAt(edit.offset); + const end = textModel.getPositionAt(edit.offset + edit.length); + return { + range: monaco.Range.fromPositions(start, end), + text: edit.content || null, + forceMoveMarkers: false + }; + }); + } + protected getPath(preferenceName: string): string[] | undefined { return [preferenceName]; } From eeadb5425f1ae1ff2816dbd99f2d063561b3b54f Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 24 Nov 2021 20:22:47 +0100 Subject: [PATCH 3/9] Remove pendingChanges from base provider; implement transaction in file provider Signed-off-by: Colin Grant --- .../preferences/preference-provider.ts | 8 -- .../preferences/preference-service.spec.ts | 3 - .../monaco/src/browser/monaco-workspace.ts | 4 +- packages/preferences/package.json | 1 + .../abstract-resource-preference-provider.ts | 132 +++++++++++++++--- 5 files changed, 117 insertions(+), 31 deletions(-) diff --git a/packages/core/src/browser/preferences/preference-provider.ts b/packages/core/src/browser/preferences/preference-provider.ts index fabbe37a62fef..404174604f2f3 100644 --- a/packages/core/src/browser/preferences/preference-provider.ts +++ b/packages/core/src/browser/preferences/preference-provider.ts @@ -75,13 +75,6 @@ export abstract class PreferenceProvider implements Disposable { } protected deferredChanges: PreferenceProviderDataChanges | undefined; - protected _pendingChanges = new Deferred(); - get pendingChanges(): Promise { - if (this._pendingChanges.state !== 'unresolved') { - this._pendingChanges = new Deferred(); - } - return this._pendingChanges.promise; - } /** * Informs the listeners that one or more preferences of this provider are changed. @@ -124,7 +117,6 @@ export abstract class PreferenceProvider implements Disposable { if (changes && Object.keys(changes).length) { this.onDidPreferencesChangedEmitter.fire(changes); } - this._pendingChanges.resolve(true); return true; }, 0); diff --git a/packages/core/src/browser/preferences/preference-service.spec.ts b/packages/core/src/browser/preferences/preference-service.spec.ts index cdb13a9b9209a..4a66b20d98d6b 100644 --- a/packages/core/src/browser/preferences/preference-service.spec.ts +++ b/packages/core/src/browser/preferences/preference-service.spec.ts @@ -193,7 +193,6 @@ describe('Preference Service', () => { assert.strictEqual(prefService.get('editor.insertSpaces'), undefined, 'get after'); assert.strictEqual(prefService.get('[go].editor.insertSpaces'), undefined, 'get after overridden'); - assert.strictEqual(await prefSchema.pendingChanges, false); assert.deepStrictEqual([], events.map(e => ({ preferenceName: e.preferenceName, newValue: e.newValue, @@ -488,7 +487,6 @@ describe('Preference Service', () => { it('onPreferenceChanged #0', async () => { const { preferences, schema } = prepareServices(); - await schema.pendingChanges; const events: PreferenceChange[] = []; preferences.onPreferenceChanged(event => events.push(event)); @@ -511,7 +509,6 @@ describe('Preference Service', () => { it('onPreferenceChanged #1', async () => { const { preferences, schema } = prepareServices(); - await schema.pendingChanges; const events: PreferenceChange[] = []; preferences.onPreferenceChanged(event => events.push(event)); diff --git a/packages/monaco/src/browser/monaco-workspace.ts b/packages/monaco/src/browser/monaco-workspace.ts index a24325bc45757..f0b64c65a953c 100644 --- a/packages/monaco/src/browser/monaco-workspace.ts +++ b/packages/monaco/src/browser/monaco-workspace.ts @@ -205,14 +205,14 @@ export class MonacoWorkspace { * Applies given edits to the given model. * The model is saved if no editors is opened for it. */ - applyBackgroundEdit(model: MonacoEditorModel, editOperations: monaco.editor.IIdentifiedSingleEditOperation[]): Promise { + applyBackgroundEdit(model: MonacoEditorModel, editOperations: monaco.editor.IIdentifiedSingleEditOperation[], shouldSave = true): Promise { return this.suppressOpenIfDirty(model, async () => { const editor = MonacoEditor.findByDocument(this.editorManager, model)[0]; const cursorState = editor && editor.getControl().getSelections() || []; model.textEditorModel.pushStackElement(); model.textEditorModel.pushEditOperations(cursorState, editOperations, () => cursorState); model.textEditorModel.pushStackElement(); - if (!editor) { + if (!editor && shouldSave) { await model.save(); } }); diff --git a/packages/preferences/package.json b/packages/preferences/package.json index 068e379dd7fd2..1f95f98abc132 100644 --- a/packages/preferences/package.json +++ b/packages/preferences/package.json @@ -9,6 +9,7 @@ "@theia/monaco": "1.20.0", "@theia/userstorage": "1.20.0", "@theia/workspace": "1.20.0", + "async-mutex": "^0.3.1", "jsonc-parser": "^2.2.0" }, "publishConfig": { diff --git a/packages/preferences/src/browser/abstract-resource-preference-provider.ts b/packages/preferences/src/browser/abstract-resource-preference-provider.ts index 5f8d166b1948e..597705009e38d 100644 --- a/packages/preferences/src/browser/abstract-resource-preference-provider.ts +++ b/packages/preferences/src/browser/abstract-resource-preference-provider.ts @@ -18,6 +18,7 @@ /* eslint-disable no-null/no-null */ import * as jsoncparser from 'jsonc-parser'; +import { Mutex, MutexInterface } from 'async-mutex'; import { inject, injectable, postConstruct } from '@theia/core/shared/inversify'; import { MessageService } from '@theia/core/lib/common/message-service'; import { Disposable } from '@theia/core/lib/common/disposable'; @@ -29,6 +30,21 @@ import { MonacoEditorModel } from '@theia/monaco/lib/browser/monaco-editor-model import { MonacoWorkspace } from '@theia/monaco/lib/browser/monaco-workspace'; import { Deferred } from '@theia/core/lib/common/promise-util'; import { FileService } from '@theia/filesystem/lib/browser/file-service'; +import { nls } from '@theia/core'; +import { EditorManager } from '@theia/editor/lib/browser'; + +export interface FilePreferenceProviderLocks { + /** + * Defined if the current operation is the first operation in a new transaction. + * The first operation is responsible for checking whether the underlying editor is dirty + * and for saving the file when the transaction is complete. + */ + releaseTransaction?: MutexInterface.Releaser | undefined; + /** + * A lock on the queue for single operations. + */ + releaseChange: MutexInterface.Releaser; +} @injectable() export abstract class AbstractResourcePreferenceProvider extends PreferenceProvider { @@ -37,10 +53,14 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi protected model: MonacoEditorModel | undefined; protected readonly loading = new Deferred(); protected modelInitialized = false; + protected readonly singleChangeLock = new Mutex(); + protected readonly transactionLock = new Mutex(); + protected pendingTransaction = new Deferred(); @inject(MessageService) protected readonly messageService: MessageService; @inject(PreferenceSchemaProvider) protected readonly schemaProvider: PreferenceSchemaProvider; @inject(FileService) protected readonly fileService: FileService; + @inject(EditorManager) protected readonly editorManager: EditorManager; @inject(PreferenceConfigurations) protected readonly configurations: PreferenceConfigurations; @@ -53,6 +73,7 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi @postConstruct() protected async init(): Promise { + this.pendingTransaction.resolve(true); const uri = this.getUri(); this.toDispose.push(Disposable.create(() => this.loading.reject(new Error(`preference provider for '${uri}' was disposed`)))); await this.readPreferencesFromFile(); @@ -71,8 +92,12 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi this.toDispose.push(reference); this.toDispose.push(Disposable.create(() => this.model = undefined)); - this.toDispose.push(this.model.onDidChangeContent(() => (console.log('READING BECAUSE...CONTENT'), this.readPreferences()))); - this.toDispose.push(this.model.onDirtyChanged(() => (console.log('READING BECAUSE...DIRTY'), this.readPreferences()))); + this.toDispose.push(this.model.onDidChangeContent(() => ( + console.log('READING BECAUSE...CONTENT', this.transactionLock.isLocked()), !this.transactionLock.isLocked() && this.readPreferences())) + ); + this.toDispose.push(this.model.onDirtyChanged(() => ( + console.log('READING BECAUSE...DIRTY', this.transactionLock.isLocked()), !this.transactionLock.isLocked() && this.readPreferences())) + ); this.toDispose.push(this.model.onDidChangeValid(() => (console.log('READING BECAUSE...VALID'), this.readPreferences()))); this.toDispose.push(Disposable.create(() => this.reset())); @@ -111,30 +136,78 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi } async setPreference(key: string, value: any, resourceUri?: string): Promise { - await this.loading.promise; - if (!this.model) { - return false; - } - if (!this.contains(resourceUri)) { - return false; - } - const path = this.getPath(key); - if (!path) { - return false; - } + const locks = await this.acquireLocks(); + let shouldSave = Boolean(locks?.releaseTransaction); try { - const editOperations = this.getEditOperations(path, value); - if (editOperations.length === 0) { + await this.loading.promise; + let path: string[] | undefined; + if (!this.model || !(path = this.getPath(key)) || !this.contains(resourceUri)) { + return false; + } + if (!locks) { // Action cancelled by user. Consider it complete. return true; } - await this.workspace.applyBackgroundEdit(this.model, editOperations); - return await this.pendingChanges; + if (shouldSave) { + if (this.model.dirty) { + shouldSave = await this.handleDirtyEditor(); + } + if (!shouldSave) { // Action cancelled by user. Consider it complete. + return true; + } + } + const editOperations = this.getEditOperations(path, value); + if (editOperations.length > 0) { + await this.workspace.applyBackgroundEdit(this.model, editOperations, false); + } + return this.pendingTransaction.promise; } catch (e) { const message = `Failed to update the value of '${key}' in '${this.getUri()}'.`; this.messageService.error(`${message} Please check if it is corrupted.`); console.error(`${message}`, e); return false; + } finally { + this.releaseLocks(locks, shouldSave); + } + } + + /** + * @returns `undefined` if the queue has been cleared by a user action. + */ + protected async acquireLocks(): Promise { + let releaseTransaction: MutexInterface.Releaser | undefined; + if (!this.transactionLock.isLocked()) { + await this.pendingTransaction.promise; // Ensure previous transaction complete before starting a new one. + this.pendingTransaction = new Deferred(); + releaseTransaction = await this.transactionLock.acquire(); + } + const releaseChange = await this.singleChangeLock.acquire() + .catch(() => { // Means that the user has cancelled this action + releaseTransaction?.(); + return undefined; + }); + return releaseChange && { releaseTransaction, releaseChange }; + } + + protected releaseLocks(locks: FilePreferenceProviderLocks | undefined, shouldSave: boolean): void { + if (locks?.releaseTransaction) { + if (shouldSave) { + this.singleChangeLock.waitForUnlock().then(() => this.singleChangeLock.runExclusive(async () => { + locks.releaseTransaction!(); // Release lock so that no new changes join this transaction. + let success = false; + try { + await this.model?.save(); + success = true; + } finally { + this.readPreferences(); + this.pendingTransaction.resolve(success); + } + })); + } else { // User cancelled the operation. + this.singleChangeLock.cancel(); + locks.releaseTransaction!(); + } } + locks?.releaseChange(); } private getEditOperations(path: string[], value: any): monaco.editor.IIdentifiedSingleEditOperation[] { @@ -242,7 +315,9 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi } } - this.emitPreferencesChangedEvent(prefChanges); + if (prefChanges.length > 0) { + this.emitPreferencesChangedEvent(prefChanges); + } } protected reset(): void { @@ -262,4 +337,25 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi } } + /** + * @returns whether the setting operation in progress, and any others started in the meantime, should continue. + */ + protected async handleDirtyEditor(): Promise { + const saveAndRetry = nls.localizeByDefault('Save and Retry'); + const open = nls.localizeByDefault('Open File'); + const msg = await this.messageService.error( + nls.localizeByDefault('Unable to write preference change because the settings file is dirty. Please save the file and try again.'), + saveAndRetry, open); + + if (this.model) { + if (msg === open) { + this.editorManager.open(new URI(this.model.uri)); + return false; + } else if (msg === saveAndRetry) { + await this.model.save(); + return true; + } + } + return false; + } } From e9b52a07b2ebd30389f893e3407985bf93159b51 Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 24 Nov 2021 20:33:01 +0100 Subject: [PATCH 4/9] changelog Signed-off-by: Colin Grant --- CHANGELOG.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03686090cdd47..11573313faaac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,18 +4,28 @@ [1.21.0 Milestone](https://github.com/eclipse-theia/theia/milestone/29) +<<<<<<< HEAD - [core, editor, editor-preview] additional commands added to tabbar context menu for editor widgets. [#10394](https://github.com/eclipse-theia/theia/pull/10394) +======= +- [preferences] Updated `AbstractResourcePreferenceProvider` to handle multiple preference settings in the same tick and handle open preference files. It will save the file exactly once, and prompt the user if the file is dirty when a programmatic setting is attempted. [#7775](https://github.com/eclipse-theia/theia/pull/7775) +>>>>>>> 6583dd0a9d4 (changelog) [Breaking Changes:](#breaking_changes_1.21.0) - [webpack] Source maps for the frontend renamed from `webpack://[namespace]/[resource-filename]...` to `webpack:///[resource-path]?[loaders]` where `resource-path` is the path to the file relative to your application package's root. +<<<<<<< HEAD - [core] `SelectionService` added to constructor arguments of `TabBarRenderer`. [#10394](https://github.com/eclipse-theia/theia/pull/10394) +======= +- [preferences] Removed `PreferenceProvider#pendingChanges` field. It was previously set unreliably and caused race conditions. If a `PreferenceProvider` needs a mechanism for deferring the resolution of `PreferenceProvider#setPreference`, it should implement its own system. See PR for example implementation in `AbstractResourcePreferenceProvider`. [#7775](https://github.com/eclipse-theia/theia/pull/7775) +>>>>>>> 6583dd0a9d4 (changelog) ## v1.20.0 - 11/25/2021 -[1.20.0 Milestone](https://github.com/eclipse-theia/theia/milestone/28) +[1.20.0 Milestone](https://github.com/eclipse-theia/theia/milestone/26) + +- [plugindev] Renamed HostedPlugin(Server|Client) to PluginDev(Server|Client) [#10352](https://github.com/eclipse-theia/theia/pull/10352) - [application-manager] added a workaround to the upstream `electron-rebuild` bug [#10429](https://github.com/eclipse-theia/theia/pull/10429) - [application-manager] remove unnecessary `font-awesome-webpack` dependency [#10401](https://github.com/eclipse-theia/theia/pull/10401) From c0585b7827ae2c34c163bb84d060afa6c0b1f1b2 Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 24 Nov 2021 20:38:39 +0100 Subject: [PATCH 5/9] Revert now-unnecessary change Signed-off-by: Colin Grant --- .../src/browser/preferences/preference-provider.ts | 3 ++- .../browser/abstract-resource-preference-provider.ts | 12 ++++-------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/packages/core/src/browser/preferences/preference-provider.ts b/packages/core/src/browser/preferences/preference-provider.ts index 404174604f2f3..cb17aa29015df 100644 --- a/packages/core/src/browser/preferences/preference-provider.ts +++ b/packages/core/src/browser/preferences/preference-provider.ts @@ -116,8 +116,9 @@ export abstract class PreferenceProvider implements Disposable { this.deferredChanges = undefined; if (changes && Object.keys(changes).length) { this.onDidPreferencesChangedEmitter.fire(changes); + return true; } - return true; + return false; }, 0); /** diff --git a/packages/preferences/src/browser/abstract-resource-preference-provider.ts b/packages/preferences/src/browser/abstract-resource-preference-provider.ts index 597705009e38d..37477445a9b1e 100644 --- a/packages/preferences/src/browser/abstract-resource-preference-provider.ts +++ b/packages/preferences/src/browser/abstract-resource-preference-provider.ts @@ -92,13 +92,9 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi this.toDispose.push(reference); this.toDispose.push(Disposable.create(() => this.model = undefined)); - this.toDispose.push(this.model.onDidChangeContent(() => ( - console.log('READING BECAUSE...CONTENT', this.transactionLock.isLocked()), !this.transactionLock.isLocked() && this.readPreferences())) - ); - this.toDispose.push(this.model.onDirtyChanged(() => ( - console.log('READING BECAUSE...DIRTY', this.transactionLock.isLocked()), !this.transactionLock.isLocked() && this.readPreferences())) - ); - this.toDispose.push(this.model.onDidChangeValid(() => (console.log('READING BECAUSE...VALID'), this.readPreferences()))); + this.toDispose.push(this.model.onDidChangeContent(() => !this.transactionLock.isLocked() && this.readPreferences())); + this.toDispose.push(this.model.onDirtyChanged(() => !this.transactionLock.isLocked() && this.readPreferences())); + this.toDispose.push(this.model.onDidChangeValid(() => this.readPreferences())); this.toDispose.push(Disposable.create(() => this.reset())); } @@ -210,7 +206,7 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi locks?.releaseChange(); } - private getEditOperations(path: string[], value: any): monaco.editor.IIdentifiedSingleEditOperation[] { + protected getEditOperations(path: string[], value: any): monaco.editor.IIdentifiedSingleEditOperation[] { const textModel = this.model!.textEditorModel; const content = this.model!.getText().trim(); // Everything is already undefined - no need for changes. From acc0de655352e0e5af77f405151ee7a769ccbf1b Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 24 Nov 2021 21:10:40 +0100 Subject: [PATCH 6/9] tests Signed-off-by: Colin Grant --- .../src/browser/abstract-resource-preference-provider.spec.ts | 2 ++ .../src/browser/abstract-resource-preference-provider.ts | 1 + 2 files changed, 3 insertions(+) diff --git a/packages/preferences/src/browser/abstract-resource-preference-provider.spec.ts b/packages/preferences/src/browser/abstract-resource-preference-provider.spec.ts index 845cd97bc0527..f7279133c1bf4 100644 --- a/packages/preferences/src/browser/abstract-resource-preference-provider.spec.ts +++ b/packages/preferences/src/browser/abstract-resource-preference-provider.spec.ts @@ -36,6 +36,7 @@ import { MonacoTextModelService } from '@theia/monaco/lib/browser/monaco-text-mo import { Disposable, MessageService } from '@theia/core/lib/common'; import { MonacoWorkspace } from '@theia/monaco/lib/browser/monaco-workspace'; import { PreferenceSchemaProvider } from '@theia/core/lib/browser'; +import { EditorManager } from '@theia/editor/lib/browser'; disableJSDOM(); @@ -84,6 +85,7 @@ describe('AbstractResourcePreferenceProvider', () => { testContainer.bind(MonacoTextModelService).toConstantValue(new MockTextModelService); testContainer.bind(MessageService).toConstantValue(undefined); testContainer.bind(MonacoWorkspace).toConstantValue(undefined); + testContainer.bind(EditorManager).toConstantValue(undefined); provider = testContainer.resolve(LessAbstractPreferenceProvider); }); diff --git a/packages/preferences/src/browser/abstract-resource-preference-provider.ts b/packages/preferences/src/browser/abstract-resource-preference-provider.ts index 37477445a9b1e..cb83bcc38653c 100644 --- a/packages/preferences/src/browser/abstract-resource-preference-provider.ts +++ b/packages/preferences/src/browser/abstract-resource-preference-provider.ts @@ -195,6 +195,7 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi success = true; } finally { this.readPreferences(); + await this.fireDidPreferencesChanged(); // Ensure all consumers of the event have received it. this.pendingTransaction.resolve(success); } })); From ce1596f61fe66c394ec8f6bc842f37ee1a3adbcd Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 24 Nov 2021 23:44:38 +0100 Subject: [PATCH 7/9] Acquire locks faster --- .../abstract-resource-preference-provider.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/preferences/src/browser/abstract-resource-preference-provider.ts b/packages/preferences/src/browser/abstract-resource-preference-provider.ts index cb83bcc38653c..2e2637412e09a 100644 --- a/packages/preferences/src/browser/abstract-resource-preference-provider.ts +++ b/packages/preferences/src/browser/abstract-resource-preference-provider.ts @@ -170,17 +170,18 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi * @returns `undefined` if the queue has been cleared by a user action. */ protected async acquireLocks(): Promise { - let releaseTransaction: MutexInterface.Releaser | undefined; - if (!this.transactionLock.isLocked()) { + // Request locks immediately + const releaseTransactionPromise = this.transactionLock.isLocked() ? undefined : this.transactionLock.acquire(); + const releaseChangePromise = this.singleChangeLock.acquire().catch(() => { + releaseTransactionPromise?.then(release => release()); + return undefined; + }); + if (releaseTransactionPromise) { await this.pendingTransaction.promise; // Ensure previous transaction complete before starting a new one. this.pendingTransaction = new Deferred(); - releaseTransaction = await this.transactionLock.acquire(); } - const releaseChange = await this.singleChangeLock.acquire() - .catch(() => { // Means that the user has cancelled this action - releaseTransaction?.(); - return undefined; - }); + // Wait to acquire locks + const [releaseTransaction, releaseChange] = await Promise.all([releaseTransactionPromise, releaseChangePromise]); return releaseChange && { releaseTransaction, releaseChange }; } From 164daa2ddf524bbf06ddc431a1f003b98e957422 Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 1 Dec 2021 01:01:19 +0100 Subject: [PATCH 8/9] update UI if user cancels --- .../abstract-resource-preference-provider.ts | 20 ++++++++++++------- .../components/preference-node-renderer.ts | 14 ++++++++++++- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/packages/preferences/src/browser/abstract-resource-preference-provider.ts b/packages/preferences/src/browser/abstract-resource-preference-provider.ts index 2e2637412e09a..cab82598a0497 100644 --- a/packages/preferences/src/browser/abstract-resource-preference-provider.ts +++ b/packages/preferences/src/browser/abstract-resource-preference-provider.ts @@ -30,7 +30,7 @@ import { MonacoEditorModel } from '@theia/monaco/lib/browser/monaco-editor-model import { MonacoWorkspace } from '@theia/monaco/lib/browser/monaco-workspace'; import { Deferred } from '@theia/core/lib/common/promise-util'; import { FileService } from '@theia/filesystem/lib/browser/file-service'; -import { nls } from '@theia/core'; +import { CancellationError, nls } from '@theia/core'; import { EditorManager } from '@theia/editor/lib/browser'; export interface FilePreferenceProviderLocks { @@ -140,15 +140,15 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi if (!this.model || !(path = this.getPath(key)) || !this.contains(resourceUri)) { return false; } - if (!locks) { // Action cancelled by user. Consider it complete. - return true; + if (!locks) { + throw new CancellationError(); } if (shouldSave) { if (this.model.dirty) { shouldSave = await this.handleDirtyEditor(); } - if (!shouldSave) { // Action cancelled by user. Consider it complete. - return true; + if (!shouldSave) { + throw new CancellationError(); } } const editOperations = this.getEditOperations(path, value); @@ -157,6 +157,9 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi } return this.pendingTransaction.promise; } catch (e) { + if (e instanceof CancellationError) { + throw e; + } const message = `Failed to update the value of '${key}' in '${this.getUri()}'.`; this.messageService.error(`${message} Please check if it is corrupted.`); console.error(`${message}`, e); @@ -200,9 +203,10 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi this.pendingTransaction.resolve(success); } })); - } else { // User cancelled the operation. + } else { // User canceled the operation. this.singleChangeLock.cancel(); locks.releaseTransaction!(); + this.pendingTransaction.resolve(false); } } locks?.releaseChange(); @@ -342,7 +346,9 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi const saveAndRetry = nls.localizeByDefault('Save and Retry'); const open = nls.localizeByDefault('Open File'); const msg = await this.messageService.error( - nls.localizeByDefault('Unable to write preference change because the settings file is dirty. Please save the file and try again.'), + nls.localizeByDefault('Unable to write into {0} settings because the file has unsaved changes. Please save the {0} settings file first and then try again.', + nls.localizeByDefault(PreferenceScope[this.getScope()].toLocaleLowerCase()) + ), saveAndRetry, open); if (this.model) { diff --git a/packages/preferences/src/browser/views/components/preference-node-renderer.ts b/packages/preferences/src/browser/views/components/preference-node-renderer.ts index 46453e91448b4..9c3e821da5ab5 100644 --- a/packages/preferences/src/browser/views/components/preference-node-renderer.ts +++ b/packages/preferences/src/browser/views/components/preference-node-renderer.ts @@ -403,7 +403,8 @@ export abstract class PreferenceLeafNodeRenderer { - return this.preferenceService.set(this.id, value, this.scopeTracker.currentScope.scope, this.scopeTracker.currentScope.uri); + return this.preferenceService.set(this.id, value, this.scopeTracker.currentScope.scope, this.scopeTracker.currentScope.uri) + .catch(() => this.handleValueChange()); } handleSearchChange(isFiltered = this.model.isFiltered): void { @@ -420,7 +421,18 @@ export abstract class PreferenceLeafNodeRenderer Date: Thu, 2 Dec 2021 16:41:09 +0100 Subject: [PATCH 9/9] reconcile changelog --- CHANGELOG.md | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11573313faaac..a55745f201c51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,28 +4,20 @@ [1.21.0 Milestone](https://github.com/eclipse-theia/theia/milestone/29) -<<<<<<< HEAD - [core, editor, editor-preview] additional commands added to tabbar context menu for editor widgets. [#10394](https://github.com/eclipse-theia/theia/pull/10394) -======= - [preferences] Updated `AbstractResourcePreferenceProvider` to handle multiple preference settings in the same tick and handle open preference files. It will save the file exactly once, and prompt the user if the file is dirty when a programmatic setting is attempted. [#7775](https://github.com/eclipse-theia/theia/pull/7775) ->>>>>>> 6583dd0a9d4 (changelog) [Breaking Changes:](#breaking_changes_1.21.0) - [webpack] Source maps for the frontend renamed from `webpack://[namespace]/[resource-filename]...` to `webpack:///[resource-path]?[loaders]` where `resource-path` is the path to the file relative to your application package's root. -<<<<<<< HEAD - [core] `SelectionService` added to constructor arguments of `TabBarRenderer`. [#10394](https://github.com/eclipse-theia/theia/pull/10394) -======= - [preferences] Removed `PreferenceProvider#pendingChanges` field. It was previously set unreliably and caused race conditions. If a `PreferenceProvider` needs a mechanism for deferring the resolution of `PreferenceProvider#setPreference`, it should implement its own system. See PR for example implementation in `AbstractResourcePreferenceProvider`. [#7775](https://github.com/eclipse-theia/theia/pull/7775) ->>>>>>> 6583dd0a9d4 (changelog) ## v1.20.0 - 11/25/2021 -[1.20.0 Milestone](https://github.com/eclipse-theia/theia/milestone/26) - -- [plugindev] Renamed HostedPlugin(Server|Client) to PluginDev(Server|Client) [#10352](https://github.com/eclipse-theia/theia/pull/10352) +[1.20.0 Milestone](https://github.com/eclipse-theia/theia/milestone/28) - [application-manager] added a workaround to the upstream `electron-rebuild` bug [#10429](https://github.com/eclipse-theia/theia/pull/10429) - [application-manager] remove unnecessary `font-awesome-webpack` dependency [#10401](https://github.com/eclipse-theia/theia/pull/10401)