debt - let editorgroup model use editors that are already in the model

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<IEditor | undefined> {
@@ -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);

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);

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
+}

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);