From 5f72d181ad0dbe12485273bd60871d98f4b7d09c Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Mon, 16 Dec 2019 10:29:32 +0100 Subject: [PATCH] debt - let editorgroup model use editors that are already in the model --- .../browser/parts/editor/editorGroupView.ts | 21 +++-- src/vs/workbench/common/editor/editorGroup.ts | 84 +++++++++++-------- .../extensions/common/extensionsInput.ts | 6 +- .../test/common/editor/editorGroups.test.ts | 84 +++++++++++++------ 4 files changed, 129 insertions(+), 66 deletions(-) diff --git a/src/vs/workbench/browser/parts/editor/editorGroupView.ts b/src/vs/workbench/browser/parts/editor/editorGroupView.ts index 59bf3608a9f..b3cdfd1210c 100644 --- a/src/vs/workbench/browser/parts/editor/editorGroupView.ts +++ b/src/vs/workbench/browser/parts/editor/editorGroupView.ts @@ -878,11 +878,13 @@ export class EditorGroupView extends Themable implements IEditorGroupView { } } - // Update model - this._group.openEditor(editor, openEditorOptions); + // Update model and make sure to continue to use the editor we get from + // the model. It is possible that the editor was already opened and we + // want to ensure that we use the existing instance in that case. + const openedEditor = this._group.openEditor(editor, openEditorOptions); // Show editor - return this.doShowEditor(editor, !!openEditorOptions.active, options); + return this.doShowEditor(openedEditor, !!openEditorOptions.active, options); } private async doShowEditor(editor: EditorInput, active: boolean, options?: EditorOptions): Promise { @@ -1051,17 +1053,22 @@ export class EditorGroupView extends Themable implements IEditorGroupView { } } - private doMoveEditorInsideGroup(editor: EditorInput, moveOptions?: IMoveEditorOptions): void { + private doMoveEditorInsideGroup(candidate: EditorInput, moveOptions?: IMoveEditorOptions): void { const moveToIndex = moveOptions ? moveOptions.index : undefined; if (typeof moveToIndex !== 'number') { return; // do nothing if we move into same group without index } - const currentIndex = this._group.indexOf(editor); - if (currentIndex === moveToIndex) { - return; // do nothing if editor is already at the given index + const currentIndex = this._group.indexOf(candidate); + if (currentIndex === -1 || currentIndex === moveToIndex) { + return; // do nothing if editor unknown in model or is already at the given index } + // Update model and make sure to continue to use the editor we get from + // the model. It is possible that the editor was already opened and we + // want to ensure that we use the existing instance in that case. + const editor = this.group.getEditorByIndex(currentIndex)!; + // Update model this._group.moveEditor(editor, moveToIndex); this._group.pin(editor); diff --git a/src/vs/workbench/common/editor/editorGroup.ts b/src/vs/workbench/common/editor/editorGroup.ts index 27cee4afde0..701ba448bb5 100644 --- a/src/vs/workbench/common/editor/editorGroup.ts +++ b/src/vs/workbench/common/editor/editorGroup.ts @@ -152,18 +152,19 @@ export class EditorGroup extends Disposable { return this.matches(this.preview, editor); } - openEditor(editor: EditorInput, options?: IEditorOpenOptions): void { - const index = this.indexOf(editor); - + openEditor(candidate: EditorInput, options?: IEditorOpenOptions): EditorInput { const makePinned = options?.pinned; const makeActive = options?.active || !this.activeEditor || (!makePinned && this.matches(this.preview, this.activeEditor)); + const existingEditor = this.findEditor(candidate); + // New editor - if (index === -1) { - let targetIndex: number; + if (!existingEditor) { + const newEditor = candidate; const indexOfActive = this.indexOf(this.active); // Insert into specific position + let targetIndex: number; if (options && typeof options.index === 'number') { targetIndex = options.index; } @@ -194,7 +195,7 @@ export class EditorGroup extends Disposable { // Insert into our list of editors if pinned or we have no preview editor if (makePinned || !this.preview) { - this.splice(targetIndex, false, editor); + this.splice(targetIndex, false, newEditor); } // Handle preview @@ -207,22 +208,24 @@ export class EditorGroup extends Disposable { targetIndex--; // accomodate for the fact that the preview editor closes } - this.replaceEditor(this.preview, editor, targetIndex, !makeActive); + this.replaceEditor(this.preview, newEditor, targetIndex, !makeActive); } - this.preview = editor; + this.preview = newEditor; } // Listeners - this.registerEditorListeners(editor); + this.registerEditorListeners(newEditor); // Event - this._onDidEditorOpen.fire(editor); + this._onDidEditorOpen.fire(newEditor); // Handle active if (makeActive) { - this.setActive(editor); + this.setActive(newEditor); } + + return newEditor; } // Existing editor @@ -230,18 +233,20 @@ export class EditorGroup extends Disposable { // Pin it if (makePinned) { - this.pin(editor); + this.pin(existingEditor); } // Activate it if (makeActive) { - this.setActive(editor); + this.setActive(existingEditor); } // Respect index if (options && typeof options.index === 'number') { - this.moveEditor(editor, options.index); + this.moveEditor(existingEditor, options.index); } + + return existingEditor; } } @@ -287,8 +292,8 @@ export class EditorGroup extends Disposable { } } - closeEditor(editor: EditorInput, openNext = true): number | undefined { - const event = this.doCloseEditor(editor, openNext, false); + closeEditor(candidate: EditorInput, openNext = true): number | undefined { + const event = this.doCloseEditor(candidate, openNext, false); if (event) { this._onDidEditorClose.fire(event); @@ -299,12 +304,14 @@ export class EditorGroup extends Disposable { return undefined; } - private doCloseEditor(editor: EditorInput, openNext: boolean, replaced: boolean): EditorCloseEvent | null { - const index = this.indexOf(editor); + private doCloseEditor(candidate: EditorInput, openNext: boolean, replaced: boolean): EditorCloseEvent | null { + const index = this.indexOf(candidate); if (index === -1) { return null; // not found } + const editor = this.editors[index]; + // Active Editor closed if (openNext && this.matches(this.active, editor)) { @@ -377,12 +384,14 @@ export class EditorGroup extends Disposable { } } - moveEditor(editor: EditorInput, toIndex: number): void { - const index = this.indexOf(editor); + moveEditor(candidate: EditorInput, toIndex: number): void { + const index = this.indexOf(candidate); if (index < 0) { return; } + const editor = this.editors[index]; + // Move this.editors.splice(index, 1); this.editors.splice(toIndex, 0, editor); @@ -391,9 +400,9 @@ export class EditorGroup extends Disposable { this._onDidEditorMove.fire(editor); } - setActive(editor: EditorInput): void { - const index = this.indexOf(editor); - if (index === -1) { + setActive(candidate: EditorInput): void { + const editor = this.findEditor(candidate); + if (!editor) { return; // not found } @@ -410,9 +419,9 @@ export class EditorGroup extends Disposable { this._onDidEditorActivate.fire(editor); } - pin(editor: EditorInput): void { - const index = this.indexOf(editor); - if (index === -1) { + pin(candidate: EditorInput): void { + const editor = this.findEditor(candidate); + if (!editor) { return; // not found } @@ -427,9 +436,9 @@ export class EditorGroup extends Disposable { this._onDidEditorPin.fire(editor); } - unpin(editor: EditorInput): void { - const index = this.indexOf(editor); - if (index === -1) { + unpin(candidate: EditorInput): void { + const editor = this.findEditor(candidate); + if (!editor) { return; // not found } @@ -531,6 +540,15 @@ export class EditorGroup extends Disposable { return -1; } + private findEditor(candidate: IEditorInput | null): EditorInput | undefined { + const index = this.indexOf(candidate, this.editors); + if (index === -1) { + return undefined; + } + + return this.editors[index]; + } + contains(candidate: EditorInput, searchInSideBySideEditors?: boolean): boolean { for (const editor of this.editors) { if (this.matches(editor, candidate)) { @@ -547,10 +565,10 @@ export class EditorGroup extends Disposable { return false; } - private setMostRecentlyUsed(editor: EditorInput): void { - const index = this.indexOf(editor); - if (index === -1) { - return; // editor not found + private setMostRecentlyUsed(candidate: EditorInput): void { + const editor = this.findEditor(candidate); + if (!editor) { + return; // not found } const mruIndex = this.indexOf(editor, this.mru); diff --git a/src/vs/workbench/contrib/extensions/common/extensionsInput.ts b/src/vs/workbench/contrib/extensions/common/extensionsInput.ts index e9728fdb893..48daaa1d61e 100644 --- a/src/vs/workbench/contrib/extensions/common/extensionsInput.ts +++ b/src/vs/workbench/contrib/extensions/common/extensionsInput.ts @@ -28,6 +28,10 @@ export class ExtensionsInput extends EditorInput { } matches(other: unknown): boolean { + if (super.matches(other) === true) { + return true; + } + if (!(other instanceof ExtensionsInput)) { return false; } @@ -52,4 +56,4 @@ export class ExtensionsInput extends EditorInput { path: this.extension.identifier.id }); } -} \ No newline at end of file +} diff --git a/src/vs/workbench/test/common/editor/editorGroups.test.ts b/src/vs/workbench/test/common/editor/editorGroups.test.ts index aa3b4d2f7ce..061bb22b042 100644 --- a/src/vs/workbench/test/common/editor/editorGroups.test.ts +++ b/src/vs/workbench/test/common/editor/editorGroups.test.ts @@ -401,9 +401,9 @@ suite('Workbench editor groups', () => { const group = createGroup(); const events = groupListener(group); - const input1 = input(); - const input2 = input(); - const input3 = input(); + const input1 = input('1'); + const input2 = input('2'); + const input3 = input('3'); // Pinned and Active group.openEditor(input1, { pinned: true, active: true }); @@ -427,11 +427,33 @@ suite('Workbench editor groups', () => { assert.equal(events.opened[1], input2); assert.equal(events.opened[2], input3); + assert.equal(events.activated[0], input1); + assert.equal(events.activated[1], input2); + assert.equal(events.activated[2], input3); + const mru = group.getEditors(true); assert.equal(mru[0], input3); assert.equal(mru[1], input2); assert.equal(mru[2], input1); + // Add some tests where a matching input is used + // and verify that events carry the original input + const sameInput1 = input('1'); + group.openEditor(sameInput1, { pinned: true, active: true }); + assert.equal(events.activated[3], input1); + + group.unpin(sameInput1); + assert.equal(events.unpinned[0], input1); + + group.pin(sameInput1); + assert.equal(events.pinned[0], input1); + + group.moveEditor(sameInput1, 1); + assert.equal(events.moved[0], input1); + + group.closeEditor(sameInput1); + assert.equal(events.closed[0].editor, input1); + group.closeAllEditors(); assert.equal(events.closed.length, 3); @@ -930,24 +952,36 @@ suite('Workbench editor groups', () => { const group = createGroup(); // [] -> /index.html/ - let indexHtml = input('index.html'); - group.openEditor(indexHtml); + const indexHtml = input('index.html'); + let openedEditor = group.openEditor(indexHtml); + assert.equal(openedEditor, indexHtml); + assert.equal(group.activeEditor, indexHtml); + assert.equal(group.previewEditor, indexHtml); + assert.equal(group.getEditors()[0], indexHtml); + assert.equal(group.count, 1); + + // /index.html/ -> /index.html/ + const sameIndexHtml = input('index.html'); + openedEditor = group.openEditor(sameIndexHtml); + assert.equal(openedEditor, indexHtml); assert.equal(group.activeEditor, indexHtml); assert.equal(group.previewEditor, indexHtml); assert.equal(group.getEditors()[0], indexHtml); assert.equal(group.count, 1); // /index.html/ -> /style.css/ - let styleCss = input('style.css'); - group.openEditor(styleCss); + const styleCss = input('style.css'); + openedEditor = group.openEditor(styleCss); + assert.equal(openedEditor, styleCss); assert.equal(group.activeEditor, styleCss); assert.equal(group.previewEditor, styleCss); assert.equal(group.getEditors()[0], styleCss); assert.equal(group.count, 1); // /style.css/ -> [/style.css/, test.js] - let testJs = input('test.js'); - group.openEditor(testJs, { active: true, pinned: true }); + const testJs = input('test.js'); + openedEditor = group.openEditor(testJs, { active: true, pinned: true }); + assert.equal(openedEditor, testJs); assert.equal(group.previewEditor, styleCss); assert.equal(group.activeEditor, testJs); assert.equal(group.isPreview(styleCss), true); @@ -957,28 +991,28 @@ suite('Workbench editor groups', () => { assert.equal(group.count, 2); // [/style.css/, test.js] -> [test.js, /index.html/] - indexHtml = input('index.html'); - group.openEditor(indexHtml, { active: true }); - assert.equal(group.activeEditor, indexHtml); - assert.equal(group.previewEditor, indexHtml); - assert.equal(group.isPreview(indexHtml), true); + const indexHtml2 = input('index.html'); + group.openEditor(indexHtml2, { active: true }); + assert.equal(group.activeEditor, indexHtml2); + assert.equal(group.previewEditor, indexHtml2); + assert.equal(group.isPreview(indexHtml2), true); assert.equal(group.isPinned(testJs), true); assert.equal(group.getEditors()[0], testJs); - assert.equal(group.getEditors()[1], indexHtml); + assert.equal(group.getEditors()[1], indexHtml2); assert.equal(group.count, 2); // make test.js active - testJs = input('test.js'); - group.setActive(testJs); + const testJs2 = input('test.js'); + group.setActive(testJs2); assert.equal(group.activeEditor, testJs); - assert.equal(group.isActive(testJs), true); + assert.equal(group.isActive(testJs2), true); assert.equal(group.count, 2); // [test.js, /indexHtml/] -> [test.js, index.html] - indexHtml = input('index.html'); - group.pin(indexHtml); - assert.equal(group.isPinned(indexHtml), true); - assert.equal(group.isPreview(indexHtml), false); + const indexHtml3 = input('index.html'); + group.pin(indexHtml3); + assert.equal(group.isPinned(indexHtml3), true); + assert.equal(group.isPreview(indexHtml3), false); assert.equal(group.activeEditor, testJs); // [test.js, index.html] -> [test.js, file.ts, index.html] @@ -1006,9 +1040,9 @@ suite('Workbench editor groups', () => { assert.ok(group.getEditors()[2].matches(indexHtml)); // make index.html active - indexHtml = input('index.html'); - group.setActive(indexHtml); - assert.equal(group.activeEditor, indexHtml); + const indexHtml4 = input('index.html'); + group.setActive(indexHtml4); + assert.equal(group.activeEditor, indexHtml2); // [test.js, /other.ts/, index.html] -> [test.js, /other.ts/] group.closeEditor(indexHtml); -- GitLab