[preferences] fix #6845: use text models to update content

It resolve following issues: - dirty editors are respected - edits are applied in thread safe fashion Signed-off-by: Anton Kosyakov <[email protected]>
eclipse-theia · Feb 10, 2020 · e3cc3be · e3cc3be
1 parent d5f3262
commit e3cc3be
Show file tree

Hide file tree

Showing 11 changed files with 421 additions and 287 deletions.
diff --git a/.gitignore b/.gitignore
@@ -13,6 +13,8 @@ coverage
 errorShots
 examples/*/src-gen
 examples/*/gen-webpack.config.js
+examples/*/.theia
+examples/*/.vscode
 .browser_modules
 **/docs/api
 package-backup.json

diff --git a/...er/preferences/launch-preferences.spec.ts → .../api-tests/src/launch-preferences.spec.js b/...er/preferences/launch-preferences.spec.ts → .../api-tests/src/launch-preferences.spec.js
diff --git a/packages/filesystem/src/node/file-change-collection.spec.ts b/packages/filesystem/src/node/file-change-collection.spec.ts
@@ -22,79 +22,84 @@ import { FileChangeType } from '../common/filesystem-watcher-protocol';
 describe('FileChangeCollection', () => {
 
     assertChanges({
-        first: FileChangeType.ADDED,
-        second: FileChangeType.ADDED,
+        changes: [FileChangeType.ADDED, FileChangeType.ADDED],
         expected: FileChangeType.ADDED
     });
 
     assertChanges({
-        first: FileChangeType.ADDED,
-        second: FileChangeType.UPDATED,
+        changes: [FileChangeType.ADDED, FileChangeType.UPDATED],
         expected: FileChangeType.ADDED
     });
 
     assertChanges({
-        first: FileChangeType.ADDED,
-        second: FileChangeType.DELETED,
-        expected: undefined
+        changes: [FileChangeType.ADDED, FileChangeType.DELETED],
+        expected: [FileChangeType.ADDED, FileChangeType.DELETED]
     });
 
     assertChanges({
-        first: FileChangeType.UPDATED,
-        second: FileChangeType.ADDED,
+        changes: [FileChangeType.UPDATED, FileChangeType.ADDED],
         expected: FileChangeType.UPDATED
     });
 
     assertChanges({
-        first: FileChangeType.UPDATED,
-        second: FileChangeType.UPDATED,
+        changes: [FileChangeType.UPDATED, FileChangeType.UPDATED],
         expected: FileChangeType.UPDATED
     });
 
     assertChanges({
-        first: FileChangeType.UPDATED,
-        second: FileChangeType.DELETED,
+        changes: [FileChangeType.UPDATED, FileChangeType.DELETED],
         expected: FileChangeType.DELETED
     });
 
     assertChanges({
-        first: FileChangeType.DELETED,
-        second: FileChangeType.ADDED,
+        changes: [FileChangeType.DELETED, FileChangeType.ADDED],
         expected: FileChangeType.UPDATED
     });
 
     assertChanges({
-        first: FileChangeType.DELETED,
-        second: FileChangeType.UPDATED,
+        changes: [FileChangeType.DELETED, FileChangeType.UPDATED],
         expected: FileChangeType.UPDATED
     });
 
     assertChanges({
-        first: FileChangeType.DELETED,
-        second: FileChangeType.DELETED,
+        changes: [FileChangeType.DELETED, FileChangeType.DELETED],
         expected: FileChangeType.DELETED
     });
 
-    function assertChanges({ first, second, expected }: {
-        first: FileChangeType,
-        second: FileChangeType,
-        expected: FileChangeType | undefined
+    assertChanges({
+        changes: [FileChangeType.ADDED, FileChangeType.UPDATED, FileChangeType.DELETED],
+        expected: [FileChangeType.ADDED, FileChangeType.DELETED]
+    });
+
+    assertChanges({
+        changes: [FileChangeType.ADDED, FileChangeType.UPDATED, FileChangeType.DELETED, FileChangeType.ADDED],
+        expected: [FileChangeType.ADDED]
+    });
+
+    assertChanges({
+        changes: [FileChangeType.ADDED, FileChangeType.UPDATED, FileChangeType.DELETED, FileChangeType.UPDATED],
+        expected: [FileChangeType.ADDED]
+    });
+
+    assertChanges({
+        changes: [FileChangeType.ADDED, FileChangeType.UPDATED, FileChangeType.DELETED, FileChangeType.DELETED],
+        expected: [FileChangeType.ADDED, FileChangeType.DELETED]
+    });
+
+    function assertChanges({ changes, expected }: {
+        changes: FileChangeType[],
+        expected: FileChangeType[] | FileChangeType
     }): void {
-        it(`${FileChangeType[first]} + ${FileChangeType[second]} => ${expected !== undefined ? FileChangeType[expected] : 'NONE'}`, () => {
+        const expectedTypes = Array.isArray(expected) ? expected : [expected];
+        const expectation = expectedTypes.map(type => FileChangeType[type]).join(' + ');
+        it(`${changes.map(type => FileChangeType[type]).join(' + ')} => ${expectation}`, () => {
             const collection = new FileChangeCollection();
             const uri = FileUri.create('/root/foo/bar.txt').toString();
-            collection.push({
-                uri,
-                type: first
-            });
-            collection.push({
-                uri,
-                type: second
-            });
-            assert.deepEqual(expected !== undefined ? [{
-                uri,
-                type: expected
-            }] : [], collection.values());
+            for (const type of changes) {
+                collection.push({ uri, type });
+            }
+            const actual = collection.values().map(({ type }) => FileChangeType[type]).join(' + ');
+            assert.equal(expectation, actual);
         });
     }
 

diff --git a/packages/filesystem/src/node/file-change-collection.ts b/packages/filesystem/src/node/file-change-collection.ts
@@ -22,7 +22,7 @@ import { FileChange, FileChangeType } from '../common/filesystem-watcher-protoco
  * Changes are normalized according following rules:
  * - ADDED + ADDED => ADDED
  * - ADDED + UPDATED => ADDED
- * - ADDED + DELETED => NONE
+ * - ADDED + DELETED => [ADDED, DELETED]
  * - UPDATED + ADDED => UPDATED
  * - UPDATED + UPDATED => UPDATED
  * - UPDATED + DELETED => DELETED
@@ -31,37 +31,48 @@ import { FileChange, FileChangeType } from '../common/filesystem-watcher-protoco
  * - DELETED + DELETED => DELETED
  */
 export class FileChangeCollection {
-    protected readonly changes = new Map<string, FileChange>();
+    protected readonly changes = new Map<string, FileChange[]>();
 
     push(change: FileChange): void {
-        const current = this.changes.get(change.uri);
-        if (current) {
-            if (this.isDeleted(current, change)) {
-                this.changes.delete(change.uri);
-            } else if (this.isUpdated(current, change)) {
-                current.type = FileChangeType.UPDATED;
-            } else if (!this.shouldSkip(current, change)) {
-                current.type = change.type;
-            }
-        } else {
-            this.changes.set(change.uri, change);
-        }
+        const changes = this.changes.get(change.uri) || [];
+        this.normalize(changes, change);
+        this.changes.set(change.uri, changes);
     }
 
-    protected isDeleted(current: FileChange, change: FileChange): boolean {
-        return current.type === FileChangeType.ADDED && change.type === FileChangeType.DELETED;
-    }
+    protected normalize(changes: FileChange[], change: FileChange): void {
+        let currentType;
+        let nextType: FileChangeType | [FileChangeType, FileChangeType] = change.type;
+        do {
+            const current = changes.pop();
+            currentType = current && current.type;
+            nextType = this.reduce(currentType, nextType);
+        } while (!Array.isArray(nextType) && currentType !== undefined && currentType !== nextType);
 
-    protected isUpdated(current: FileChange, change: FileChange): boolean {
-        return current.type === FileChangeType.DELETED && change.type === FileChangeType.ADDED;
+        const uri = change.uri;
+        if (Array.isArray(nextType)) {
+            changes.push(...nextType.map(type => ({ uri, type })));
+        } else {
+            changes.push({ uri, type: nextType });
+        }
     }
 
-    protected shouldSkip(current: FileChange, change: FileChange): boolean {
-        return (current.type === FileChangeType.ADDED && change.type === FileChangeType.UPDATED) ||
-            (current.type === FileChangeType.UPDATED && change.type === FileChangeType.ADDED);
+    protected reduce(current: FileChangeType | undefined, change: FileChangeType): FileChangeType | [FileChangeType, FileChangeType] {
+        if (current === undefined) {
+            return change;
+        }
+        if (current === FileChangeType.ADDED) {
+            if (change === FileChangeType.DELETED) {
+                return [FileChangeType.ADDED, FileChangeType.DELETED];
+            }
+            return FileChangeType.ADDED;
+        }
+        if (change === FileChangeType.DELETED) {
+            return FileChangeType.DELETED;
+        }
+        return FileChangeType.UPDATED;
     }
 
     values(): FileChange[] {
-        return Array.from(this.changes.values());
+        return Array.from(this.changes.values()).reduce((acc, val) => acc.concat(val), []);
     }
 }
diff --git a/packages/filesystem/src/node/node-filesystem.ts b/packages/filesystem/src/node/node-filesystem.ts
@@ -327,19 +327,11 @@ export class FileSystemNode implements FileSystem {
             const filePath = FileUri.fsPath(_uri);
             const outputRootPath = paths.join(os.tmpdir(), v4());
             try {
-                await new Promise<void>((resolve, reject) => {
-                    fs.rename(filePath, outputRootPath, async error => {
-                        if (error) {
-                            reject(error);
-                            return;
-                        }
-                        resolve();
-                    });
-                });
+                await fs.rename(filePath, outputRootPath);
                 // There is no reason for the promise returned by this function not to resolve
                 // as soon as the move is complete.  Clearing up the temporary files can be
                 // done in the background.
-                fs.remove(FileUri.fsPath(outputRootPath));
+                fs.remove(outputRootPath);
             } catch (error) {
                 return fs.remove(filePath);
             }

diff --git a/packages/monaco/src/browser/monaco-editor-model.ts b/packages/monaco/src/browser/monaco-editor-model.ts
@@ -75,8 +75,8 @@ export class MonacoEditorModel implements ITextEditorModel, TextEditorDocument {
         this.toDispose.push(this.onDidSaveModelEmitter);
         this.toDispose.push(this.onWillSaveModelEmitter);
         this.toDispose.push(this.onDirtyChangedEmitter);
-        this.resolveModel = resource.readContents(options).then(content => this.initialize(content));
         this.defaultEncoding = options && options.encoding ? options.encoding : undefined;
+        this.resolveModel = this.readContents().then(content => this.initialize(content || ''));
     }
 
     dispose(): void {
@@ -120,6 +120,21 @@ export class MonacoEditorModel implements ITextEditorModel, TextEditorDocument {
         }
     }
 
+    /**
+     * Use `valid` to access it.
+     * Use `setValid` to mutate it.
+     */
+    protected _valid = false;
+    /**
+     * Whether it is possible to load content from the underlying resource.
+     */
+    get valid(): boolean {
+        return this._valid;
+    }
+    protected setValid(valid: boolean): void {
+        this._valid = valid;
+    }
+
     protected _dirty = false;
     get dirty(): boolean {
         return this._dirty;
@@ -243,10 +258,13 @@ export class MonacoEditorModel implements ITextEditorModel, TextEditorDocument {
             return;
         }
 
-        const newText = await this.readContents();
-        if (newText === undefined || token.isCancellationRequested || this._dirty) {
+        let newText = await this.readContents();
+        if (token.isCancellationRequested || this._dirty) {
             return;
         }
+        if (newText === undefined) {
+            newText = '';
+        }
 
         const value = this.model.getValue();
         if (value === newText) {
@@ -261,9 +279,12 @@ export class MonacoEditorModel implements ITextEditorModel, TextEditorDocument {
     }
     protected async readContents(): Promise<string | undefined> {
         try {
-            return await this.resource.readContents({ encoding: this.getEncoding() });
+            const content = await this.resource.readContents({ encoding: this.getEncoding() });
+            this.setValid(true);
+            return content;
         } catch (e) {
             if (ResourceError.NotFound.is(e)) {
+                this.setValid(false);
                 return undefined;
             }
             throw e;
@@ -377,6 +398,7 @@ export class MonacoEditorModel implements ITextEditorModel, TextEditorDocument {
 
         const content = this.model.getValue();
         await Resource.save(this.resource, { changes, content, options: { encoding: this.getEncoding(), overwriteEncoding } }, token);
+        this.setValid(true);
         if (token.isCancellationRequested) {
             return;
         }

diff --git a/packages/monaco/src/browser/monaco-workspace.ts b/packages/monaco/src/browser/monaco-workspace.ts
@@ -32,6 +32,7 @@ import { WillSaveMonacoModelEvent, MonacoEditorModel, MonacoModelContentChangedE
 import { MonacoEditor } from './monaco-editor';
 import { MonacoConfigurations } from './monaco-configurations';
 import { ProblemManager } from '@theia/markers/lib/browser';
+import { MaybePromise } from '@theia/core/lib/common/types';
 
 export interface MonacoDidChangeTextDocumentParams extends lang.DidChangeTextDocumentParams {
     readonly textDocument: MonacoEditorModel;
@@ -276,7 +277,12 @@ export class MonacoWorkspace implements lang.Workspace {
         this.onDidSaveTextDocumentEmitter.fire(model);
     }
 
+    protected suppressedOpenIfDirty: MonacoEditorModel[] = [];
+
     protected openEditorIfDirty(model: MonacoEditorModel): void {
+        if (this.suppressedOpenIfDirty.indexOf(model) !== -1) {
+            return;
+        }
         if (model.dirty && MonacoEditor.findByDocument(this.editorManager, model).length === 0) {
             // create a new reference to make sure the model is not disposed before it is
             // acquired by the editor, thus losing the changes that made it dirty.
@@ -288,6 +294,18 @@ export class MonacoWorkspace implements lang.Workspace {
         }
     }
 
+    protected async suppressOpenIfDirty(model: MonacoEditorModel, cb: () => MaybePromise<void>): Promise<void> {
+        this.suppressedOpenIfDirty.push(model);
+        try {
+            await cb();
+        } finally {
+            const i = this.suppressedOpenIfDirty.indexOf(model);
+            if (i !== -1) {
+                this.suppressedOpenIfDirty.splice(i, 1);
+            }
+        }
+    }
+
     createFileSystemWatcher(globPattern: string, ignoreCreateEvents?: boolean, ignoreChangeEvents?: boolean, ignoreDeleteEvents?: boolean): lang.FileSystemWatcher {
         const disposables = new DisposableCollection();
         const onDidCreateEmitter = new lang.Emitter<Uri>();
@@ -331,6 +349,19 @@ export class MonacoWorkspace implements lang.Workspace {
         };
     }
 
+    applyBackgroundEdit(model: MonacoEditorModel, editOperations: monaco.editor.IIdentifiedSingleEditOperation[]): 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) {
+                await model.save();
+            }
+        });
+    }
+
     async applyEdit(changes: lang.WorkspaceEdit, options?: EditorOpenerOptions): Promise<boolean> {
         const workspaceEdit = this.p2m.asWorkspaceEdit(changes);
         await this.applyBulkEdit(workspaceEdit, options);

diff --git a/packages/plugin-ext/src/common/plugin-api-rpc.ts b/packages/plugin-ext/src/common/plugin-api-rpc.ts
@@ -811,7 +811,7 @@ export interface TextEditorsExt {
 }
 
 export interface SingleEditOperation {
-    range: Range;
+    range?: Range;
     text?: string;
     forceMoveMarkers?: boolean;
 }