save preferences if not dirty

Signed-off-by: Colin Grant <[email protected]>

examples/api-tests/src/saveable.spec.js

@@ -23,14 +23,17 @@ describe('Saveable', function () {
     const { EditorManager } = require('@theia/editor/lib/browser/editor-manager');
     const { EditorWidget } = require('@theia/editor/lib/browser/editor-widget');
     const { PreferenceService } = require('@theia/core/lib/browser/preferences/preference-service');
+    const { PreferenceScope } = require('@theia/core/lib/browser/preferences/preference-scope');
     const { Saveable, SaveableWidget } = require('@theia/core/lib/browser/saveable');
     const { WorkspaceService } = require('@theia/workspace/lib/browser/workspace-service');
     const { FileService } = require('@theia/filesystem/lib/browser/file-service');
     const { FileResource } = require('@theia/filesystem/lib/browser/file-resource');
     const { ETAG_DISABLED } = require('@theia/filesystem/lib/common/files');
     const { MonacoEditor } = require('@theia/monaco/lib/browser/monaco-editor');
+    const { MonacoEditorModel } = require('@theia/monaco/lib/browser/monaco-editor-model');
     const { Deferred } = require('@theia/core/lib/common/promise-util');
     const { Disposable, DisposableCollection } = require('@theia/core/lib/common/disposable');
+    const { AbstractResourcePreferenceProvider } = require('@theia/preferences/lib/browser/abstract-resource-preference-provider');
 
     const container = window.theia.container;
     const editorManager = container.get(EditorManager);
@@ -62,6 +65,72 @@ describe('Saveable', function () {
         return Disposable.create(() => fileResource['shouldOverwrite'] = originalShouldOverwrite);
     }
 
+    /**
+     * @param {MonacoEditorModel} modelToSpyOn
+     * @returns {{restore: Disposable, record: {calls: number}}}
+     */
+    function spyOnSave(modelToSpyOn) {
+        assert.isTrue(modelToSpyOn instanceof MonacoEditorModel);
+        const toRestore = modelToSpyOn.save;
+        const callable = toRestore.bind(modelToSpyOn);
+        const record = { calls: 0 };
+        modelToSpyOn.save = () => {
+            record.calls++;
+            return callable();
+        };
+        const restore = Disposable.create(() => modelToSpyOn.save = toRestore);
+        return { restore, record };
+    }
+
+    /** @typedef {Object} PrefTest
+     * @property {{calls: number}} record
+     * @property {boolean[]} initialValues
+     * @property {EditorWidget & SaveableWidget} editorWidget
+     * @property {MonacoEditor} prefEditor
+     * @property {AbstractResourcePreferenceProvider} provider
+     */
+
+    /**
+     * @param {string[]} preferencesToModify
+     * @returns {Promise<undefined | PrefTest>}
+     */
+    async function setUpPrefsTest(preferencesToModify) {
+        const preferenceFile = preferences.getConfigUri(PreferenceScope.Folder, rootUri.toString());
+        assert.isDefined(preferenceFile);
+        if (!preferenceFile) { return; }
+
+        const provider = preferences
+            .preferenceProviders
+            .get(PreferenceScope.Folder)
+            .providers
+            .get(preferenceFile.toString());
+
+        assert.isDefined(provider);
+        assert.isTrue(provider instanceof AbstractResourcePreferenceProvider);
+
+        await Promise.all(preferencesToModify.map(name => preferences.set(name, undefined, undefined, rootUri.toString())));
+
+        const editorWidget =  /** @type {EditorWidget & SaveableWidget} */
+            (await editorManager.open(preferenceFile, { mode: 'reveal' }));
+        const prefEditor = MonacoEditor.get(editorWidget);
+        assert.isDefined(prefEditor);
+        if (!prefEditor) { return; }
+        assert.isFalse(Saveable.isDirty(editorWidget), 'should NOT be dirty on open');
+        const model = prefEditor?.document;
+        assert.isDefined(model);
+
+        if (!model) { return; }
+        const { restore, record } = spyOnSave(model);
+        toTearDown.push(restore);
+        /** @type {boolean[]} */
+        const initialValues = preferencesToModify.map(name =>
+            /** @type {boolean | undefined} */
+            !!preferences.get(name, undefined, rootUri.toString())
+        );
+
+        return { record, initialValues, editorWidget, prefEditor, provider };
+    }
+
     const toTearDown = new DisposableCollection();
 
     /** @type {string | undefined} */
@@ -485,4 +554,93 @@ describe('Saveable', function () {
         }
     });
 
+    it('saves preference file when open and not dirty', async function () {
+        const prefName = 'editor.copyWithSyntaxHighlighting';
+        const prefTest = await setUpPrefsTest([prefName]);
+        if (!prefTest) { return; }
+        const { record, initialValues: [initialCopyWithHighlightPref], editorWidget } = prefTest;
+        await preferences.set(prefName, !initialCopyWithHighlightPref, undefined, rootUri.toString());
+        /** @type {boolean | undefined} */
+        const newValue = preferences.get(prefName, undefined, rootUri.toString());
+        assert.equal(newValue, !initialCopyWithHighlightPref);
+        assert.isFalse(Saveable.isDirty(editorWidget), "editor should not be dirty if it wasn't dirty before");
+        assert.equal(1, record.calls, 'save should have been called one time.');
+    });
+
+    it('saves once when many edits are made (editor open)', async function () {
+        const prefNames = ['editor.copyWithSyntaxHighlighting', 'debug.inlineValues'];
+        const prefTest = await setUpPrefsTest(prefNames);
+        if (!prefTest) { return; }
+
+        const { record, initialValues, editorWidget } = prefTest;
+
+        /** @type {Promise<void>[]} */
+        const attempts = [];
+        while (attempts.length < 250) {
+            prefNames.forEach((prefName, index) => {
+                const value = attempts.length % 2 === 0 ? !initialValues[index] : initialValues[index];
+                attempts.push(preferences.set(prefName, value, undefined, rootUri.toString()));
+            });
+        }
+        await Promise.all(attempts);
+        assert.isFalse(Saveable.isDirty(editorWidget), "editor should not be dirty if it wasn't dirty before");
+        assert.equal(1, record.calls, 'save should have been called one time.');
+    });
+
+    it('saves once when many edits are made (editor closed)', async function () {
+        const prefNames = ['editor.copyWithSyntaxHighlighting', 'debug.inlineValues'];
+        const prefTest = await setUpPrefsTest(prefNames);
+        if (!prefTest) { return; }
+
+        const { record, initialValues, editorWidget } = prefTest;
+        await editorWidget.closeWithoutSaving();
+
+        /** @type {Promise<void>[]} */
+        const attempts = [];
+        while (attempts.length < 250) {
+            prefNames.forEach((prefName, index) => {
+                const value = attempts.length % 2 === 0 ? !initialValues[index] : initialValues[index];
+                attempts.push(preferences.set(prefName, value, undefined, rootUri.toString()));
+            });
+        }
+        await Promise.all(attempts);
+        assert.equal(1, record.calls, 'save should have been called one time.');
+    });
+
+    it('displays the toast once no matter how many edits are queued', async function () {
+        const prefNames = ['editor.copyWithSyntaxHighlighting', 'debug.inlineValues'];
+        const prefTest = await setUpPrefsTest(prefNames);
+        if (!prefTest) { return; }
+
+        const { record, initialValues, editorWidget, prefEditor, provider } = prefTest;
+        const initialContent = prefEditor.getControl().getValue();
+        prefEditor.getControl().setValue('anything dirty will do.');
+        prefEditor.getControl().setValue(initialContent);
+        assert.isTrue(Saveable.isDirty(editorWidget));
+
+        const toRestore = provider['handleDirtyEditor'];
+        const callable = provider['handleDirtyEditor'].bind(provider);
+        const dirtyEditorRecord = { calls: 0 };
+        const spy = () => {
+            dirtyEditorRecord.calls++;
+            return callable();
+        };
+        provider['handleDirtyEditor'] = spy;
+        toTearDown.push(Disposable.create(() => provider['handleDirtyEditor'] = toRestore));
+        /** @type {Promise<void>[]} */
+        const attempts = [];
+        while (attempts.length < 250) {
+            prefNames.forEach((prefName, index) => {
+                const value = attempts.length % 2 === 0 ? !initialValues[index] : initialValues[index];
+                attempts.push(preferences.set(prefName, value, undefined, rootUri.toString()));
+            });
+        }
+        assert.equal(0, record.calls, 'should not have been saved yet');
+        await Saveable.save(editorWidget); // Simulate the user saving and retrying.
+        provider['dirtyEditorHandled'].resolve();
+        await Promise.all(attempts);
+        assert.equal(record.calls, 2, 'should have saved twice');
+        assert.equal(dirtyEditorRecord.calls, 1, 'should have displayed the toast once');
+        assert.equal(provider['editQueue'].length, 0, 'there should be no pending edits');
+    });
 });

packages/monaco/src/browser/monaco-workspace.ts

@@ -192,14 +192,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<void> {
+    applyBackgroundEdit(model: MonacoEditorModel, editOperations: monaco.editor.IIdentifiedSingleEditOperation[], shouldSave: boolean = true): Promise<void> {
         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();
             }
         });

packages/preferences/src/browser/abstract-resource-preference-provider.ts

@@ -29,13 +29,23 @@ import { MonacoTextModelService } from '@theia/monaco/lib/browser/monaco-text-mo
 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 { EditorManager } from '@theia/editor/lib/browser';
+
+interface EnqueuedPreferenceChange {
+    key: string,
+    value: any,
+    editResolver: Deferred<boolean | Promise<boolean>>,
+}
 
 @injectable()
 export abstract class AbstractResourcePreferenceProvider extends PreferenceProvider {
 
     protected preferences: { [key: string]: any } = {};
     protected model: MonacoEditorModel | undefined;
+    protected editQueue: EnqueuedPreferenceChange[] = [];
     protected readonly loading = new Deferred();
+    protected dirtyEditorHandled = new Deferred();
+    protected pendingTransaction = new Deferred();
 
     @inject(PreferenceService) protected readonly preferenceService: PreferenceService;
     @inject(MessageService) protected readonly messageService: MessageService;
@@ -50,11 +60,16 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi
     @inject(MonacoWorkspace)
     protected readonly workspace: MonacoWorkspace;
 
+    @inject(EditorManager)
+    protected readonly editorManager: EditorManager;
+
     @postConstruct()
     protected async init(): Promise<void> {
         const uri = this.getUri();
         this.toDispose.push(Disposable.create(() => this.loading.reject(new Error(`preference provider for '${uri}' was disposed`))));
         this._ready.resolve();
+        this.dirtyEditorHandled.resolve();
+        this.pendingTransaction.resolve();
 
         const reference = await this.textModelService.createModelReference(uri);
         if (this.toDispose.disposed) {
@@ -110,12 +125,57 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi
 
     async setPreference(key: string, value: any, resourceUri?: string): Promise<boolean> {
         await this.loading.promise;
+        // Wait for save of previous transaction
+        await this.pendingTransaction.promise;
         if (!this.model) {
             return false;
         }
         if (!this.contains(resourceUri)) {
             return false;
         }
+        const isFirstEditInQueue = !this.editQueue.length;
+        const mustHandleDirty = isFirstEditInQueue && this.model.dirty;
+        if (mustHandleDirty) {
+            this.dirtyEditorHandled = new Deferred();
+            this.handleDirtyEditor();
+        }
+        const editResolver = new Deferred<boolean | Promise<boolean>>();
+        this.editQueue.push({ key, value, editResolver });
+        if (isFirstEditInQueue) {
+            this.doSetPreferences();
+        }
+        return editResolver.promise;
+    }
+
+    async doSetPreferences(): Promise<void> {
+        const localPendingChanges = new Deferred<boolean>();
+        await this.dirtyEditorHandled.promise;
+        while (this.editQueue.length) {
+            const { key, value, editResolver } = this.editQueue.shift()!;
+            const result = await this.doSetPreference(key, value) ? localPendingChanges.promise : false;
+            editResolver.resolve(result);
+        }
+        let success = true;
+        // Defer new actions until transaction complete.
+        // Prevents the dirty-editor toast from appearing if save cycle in progress.
+        this.pendingTransaction = new Deferred();
+        try {
+            if (this.model?.dirty) {
+                await this.model.save();
+                // On save, the file change handler will set ._pendingChanges
+                success = await this.pendingChanges;
+            }
+        } catch {
+            success = false;
+        }
+        this.pendingTransaction.resolve();
+        localPendingChanges.resolve(success);
+    }
+
+    protected async doSetPreference(key: string, value: any): Promise<boolean> {
+        if (!this.model) {
+            return false;
+        }
         const path = this.getPath(key);
         if (!path) {
             return false;
@@ -151,8 +211,8 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi
                     forceMoveMarkers: false
                 });
             }
-            await this.workspace.applyBackgroundEdit(this.model, editOperations);
-            return await this.pendingChanges;
+            await this.workspace.applyBackgroundEdit(this.model, editOperations, false);
+            return true;
         } catch (e) {
             const message = `Failed to update the value of '${key}' in '${this.getUri()}'.`;
             this.messageService.error(`${message} Please check if it is corrupted.`);
@@ -161,6 +221,25 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi
         }
     }
 
+    protected async handleDirtyEditor(): Promise<void> {
+        const msg = await this.messageService.error(
+            'Unable to write preference change because the settings file is dirty. Please save the file and try again.',
+            'Save and Retry', 'Open File');
+
+        if (this.model) {
+            if (msg === 'Open File') {
+                this.editorManager.open(new URI(this.model.uri));
+            } else if (msg === 'Save and Retry') {
+                await this.model.save();
+            }
+        }
+        if (msg !== 'Save and Retry') {
+            this.editQueue.forEach(({ editResolver }) => editResolver.resolve(false));
+            this.editQueue = [];
+        }
+        this.dirtyEditorHandled.resolve();
+    }
+
     protected getPath(preferenceName: string): string[] | undefined {
         return [preferenceName];
     }

packages/preferences/src/browser/views/components/preference-array-input.tsx

@@ -20,7 +20,7 @@ import { Preference } from '../../util/preference-types';
 
 interface PreferenceArrayInputProps {
     preferenceDisplayNode: Preference.NodeWithValueInSingleScope;
-    setPreference(preferenceName: string, preferenceValue: string[]): void;
+    setPreference(preferenceName: string, preferenceValue: string[]): Promise<void>;
 }
 
 export const PreferenceArrayInput: React.FC<PreferenceArrayInputProps> = ({ preferenceDisplayNode, setPreference }) => {

packages/preferences/src/browser/views/components/preference-boolean-input.tsx

@@ -19,7 +19,7 @@ import { Preference } from '../../util/preference-types';
 
 interface PreferenceBooleanInputProps {
     preferenceDisplayNode: Preference.NodeWithValueInSingleScope;
-    setPreference: (preferenceName: string, preferenceValue: boolean) => void;
+    setPreference: (preferenceName: string, preferenceValue: boolean) => Promise<void>;
 }
 
 export const PreferenceBooleanInput: React.FC<PreferenceBooleanInputProps> = ({ preferenceDisplayNode, setPreference }) => {
@@ -34,10 +34,15 @@ export const PreferenceBooleanInput: React.FC<PreferenceBooleanInputProps> = ({
         setChecked(!!value);
     }, [value]);
 
-    const setValue = React.useCallback((e: React.ChangeEvent<HTMLInputElement>) => {
+    const setValue = React.useCallback(async (e: React.ChangeEvent<HTMLInputElement>) => {
         setChecked(!checked);
-        setPreference(id, e.target.checked);
-    }, [checked]);
+        const newValue = e.target.checked;
+        try {
+            await setPreference(id, newValue);
+        } catch {
+            setChecked(!!value);
+        }
+    }, [checked, value]);
 
     return (
         <label htmlFor={`preference-checkbox-${id}`}>

packages/preferences/src/browser/views/components/preference-number-input.tsx

@@ -20,7 +20,7 @@ import { Preference } from '../../util/preference-types';
 
 interface PreferenceNumberInputProps {
     preferenceDisplayNode: Preference.NodeWithValueInSingleScope;
-    setPreference(preferenceName: string, preferenceValue: number): void;
+    setPreference(preferenceName: string, preferenceValue: number): Promise<void>;
 }
 
 export const PreferenceNumberInput: React.FC<PreferenceNumberInputProps> = ({ preferenceDisplayNode, setPreference }) => {
@@ -46,7 +46,7 @@ export const PreferenceNumberInput: React.FC<PreferenceNumberInputProps> = ({ pr
         clearTimeout(currentTimeout);
         const { value: newValue } = e.target;
         setCurrentValue(newValue);
-        const { value: inputValue , message } = getInputValidation(newValue);
+        const { value: inputValue, message } = getInputValidation(newValue);
         setValidationMessage(message);
         if (!isNaN(inputValue)) {
             const newTimeout = setTimeout(() => setPreference(id, inputValue), 750);