debt - some multi-tabs select cleanup

src/vs/workbench/browser/parts/editor/multiEditorTabsControl.ts

@@ -889,14 +889,15 @@ export class MultiEditorTabsControl extends EditorTabsControl {
 			const editor = this.tabsModel.getEditorByIndex(tabIndex);
 			if (editor) {
 				if (e.shiftKey) {
-					let anchor;
+					let anchor: EditorInput;
 					if (this.lastSingleSelectSelectedEditor && this.tabsModel.isSelected(this.lastSingleSelectSelectedEditor)) {
 						// The last selected editor is the anchor
 						anchor = this.lastSingleSelectSelectedEditor;
 					} else {
 						// The active editor is the anchor
-						this.lastSingleSelectSelectedEditor = this.groupView.activeEditor!;
-						anchor = this.groupView.activeEditor!;
+						const activeEditor = assertIsDefined(this.groupView.activeEditor);
+						this.lastSingleSelectSelectedEditor = activeEditor;
+						anchor = activeEditor;
 					}
 					await this.selectEditorsBetween(editor, anchor);
 				} else if ((e.ctrlKey && !isMacintosh) || (e.metaKey && isMacintosh)) {
@@ -1292,7 +1293,7 @@ export class MultiEditorTabsControl extends EditorTabsControl {
 			throw new BugIndicatingError();
 		}
 
-		const selection = this.groupView.selectedEditors;
+		let selection = this.groupView.selectedEditors;
 
 		// Unselect editors on other side of anchor in relation to the target
 		let currentIndex = anchorIndex;
@@ -1308,7 +1309,7 @@ export class MultiEditorTabsControl extends EditorTabsControl {
 				break;
 			}
 
-			selection.filter(editor => !editor.matches(currentEditor));
+			selection = selection.filter(editor => !editor.matches(currentEditor));
 		}
 
 		// Select editors between anchor and target
@@ -1334,7 +1335,7 @@ export class MultiEditorTabsControl extends EditorTabsControl {
 			return;
 		}
 
-		let newActiveEditor = this.groupView.activeEditor!;
+		let newActiveEditor = assertIsDefined(this.groupView.activeEditor);
 
 		// If active editor is bing unselected then find the most recently opened selected editor
 		// that is not the editor being unselected
@@ -1355,7 +1356,8 @@ export class MultiEditorTabsControl extends EditorTabsControl {
 
 	private async unselectAllEditors(): Promise<void> {
 		if (this.groupView.selectedEditors.length > 1) {
-			await this.groupView.setSelection(this.groupView.activeEditor!, []);
+			const activeEditor = assertIsDefined(this.groupView.activeEditor);
+			await this.groupView.setSelection(activeEditor, []);
 		}
 	}
 
@@ -1643,7 +1645,6 @@ export class MultiEditorTabsControl extends EditorTabsControl {
 	}
 
 	private doRedrawTabActive(isGroupActive: boolean, allowBorderTop: boolean, editor: EditorInput, tabContainer: HTMLElement, tabActionBar: ActionBar): void {
-
 		const isActive = this.tabsModel.isActive(editor);
 		const isSelected = this.tabsModel.isSelected(editor);
 
@@ -1657,7 +1658,7 @@ export class MultiEditorTabsControl extends EditorTabsControl {
 		if (isActive) {
 			const activeTabBorderColorBottom = this.getColor(isGroupActive ? TAB_ACTIVE_BORDER : TAB_UNFOCUSED_ACTIVE_BORDER);
 			tabContainer.classList.toggle('tab-border-bottom', !!activeTabBorderColorBottom);
-			tabContainer.style.setProperty('--tab-border-bottom-color', activeTabBorderColorBottom?.toString() ?? '');
+			tabContainer.style.setProperty('--tab-border-bottom-color', activeTabBorderColorBottom ?? '');
 		}
 
 		// Set border TOP if theme defined color
@@ -2219,30 +2220,30 @@ export class MultiEditorTabsControl extends EditorTabsControl {
 		else if (this.editorTransfer.hasData(DraggedEditorIdentifier.prototype)) {
 			const data = this.editorTransfer.getData(DraggedEditorIdentifier.prototype);
 			if (Array.isArray(data) && data.length > 0) {
-				const sourceGroup = data.length ? this.editorPartsView.getGroup(data[0].identifier.groupId) : undefined;
-				const isLocalMove = sourceGroup === this.groupView;
-
-				// Keep the same order when moving / copying editors within the same group
-				for (const de of data) {
-					const editor = de.identifier.editor;
-
-					// Only allow moving/copying from a single group source
-					if (!sourceGroup || sourceGroup.id !== de.identifier.groupId) {
-						continue;
-					}
-
-					const sourceEditorIndex = sourceGroup.getIndexOfEditor(editor);
-					if (isLocalMove && sourceEditorIndex < targetEditorIndex) {
-						targetEditorIndex--;
-					}
-
-					if (this.isMoveOperation(e, de.identifier.groupId, editor)) {
-						sourceGroup.moveEditor(editor, this.groupView, { ...options, index: targetEditorIndex });
-					} else {
-						sourceGroup.copyEditor(editor, this.groupView, { ...options, index: targetEditorIndex });
+				const sourceGroup = this.editorPartsView.getGroup(data[0].identifier.groupId);
+				if (sourceGroup) {
+					for (const de of data) {
+						const editor = de.identifier.editor;
+
+						// Only allow moving/copying from a single group source
+						if (sourceGroup.id !== de.identifier.groupId) {
+							continue;
+						}
+
+						// Keep the same order when moving / copying editors within the same group
+						const sourceEditorIndex = sourceGroup.getIndexOfEditor(editor);
+						if (sourceGroup === this.groupView && sourceEditorIndex < targetEditorIndex) {
+							targetEditorIndex--;
+						}
+
+						if (this.isMoveOperation(e, de.identifier.groupId, editor)) {
+							sourceGroup.moveEditor(editor, this.groupView, { ...options, index: targetEditorIndex });
+						} else {
+							sourceGroup.copyEditor(editor, this.groupView, { ...options, index: targetEditorIndex });
+						}
+
+						targetEditorIndex++;
 					}
-
-					targetEditorIndex++;
 				}
 			}
 

src/vs/workbench/browser/parts/editor/multiRowEditorTabsControl.ts

@@ -199,7 +199,7 @@ export class MultiRowEditorControl extends Disposable implements IEditorTabsCont
 		return this.stickyEditorTabsControl.getHeight() + this.unstickyEditorTabsControl.getHeight();
 	}
 
-	public override dispose(): void {
+	override dispose(): void {
 		this.parent.classList.toggle('two-tab-bars', false);
 
 		super.dispose();

src/vs/workbench/common/editor/editorGroupModel.ts

@@ -220,9 +220,9 @@ export class EditorGroupModel extends Disposable implements IEditorGroupModel {
 
 	private locked = false;
 
-	private selection: EditorInput[] = [];					// editors in selected state, first one is active
+	private selection: EditorInput[] = [];					// editors in selected state, first one is active TODO align this with transient editors and use a Set<EditorInput> instead
 
-	private set active(editor: EditorInput | null) {
+	private set active(editor: EditorInput | null) { // TODO: this misses an event when selection changes and should be a method?
 		this.selection = editor ? [editor] : [];
 	}
 	private get active(): EditorInput | null {
@@ -413,7 +413,7 @@ export class EditorGroupModel extends Disposable implements IEditorGroupModel {
 			};
 			this._onDidModelChange.fire(event);
 
-			// Handle active & selection
+			// Handle active & selection TODO why do we call this when `makeActive` is false?
 			this.setSelection(makeActive ? newEditor : this.activeEditor, options?.inactiveSelection ?? []);
 
 			return {
@@ -434,7 +434,7 @@ export class EditorGroupModel extends Disposable implements IEditorGroupModel {
 				this.doPin(existingEditor, existingEditorIndex);
 			}
 
-			// Activate / select it
+			// Activate / select it TODO why do we call this when `makeActive` is false?
 			this.setSelection(makeActive ? existingEditor : this.activeEditor, options?.inactiveSelection ?? []);
 
 			// Respect index
@@ -570,22 +570,22 @@ export class EditorGroupModel extends Disposable implements IEditorGroupModel {
 					}
 				}
 
-				const newSelection = this.selection.filter(selected => !selected.matches(newActive) && !selected.matches(editor));
-				this.doSetSelection(newActive, this.editors.indexOf(newActive), newSelection);
+				const newInactiveSelection = this.selection.filter(selected => !selected.matches(newActive) && !selected.matches(editor));
+				this.doSetSelection(newActive, this.editors.indexOf(newActive), newInactiveSelection);
 			}
 
 			// One Editor
 			else {
-				this.active = null;
+				this.active = null; // TODO this potentially changes selection without a related event. and is it expected to change the selection?
 			}
 		}
 
 		// Remove from selection
 		else if (!isActiveEditor) {
 			const wasSelected = !!this.selection.find(selected => this.matches(selected, editor));
 			if (wasSelected) {
-				const newSelection = this.selection.filter(selected => !selected.matches(editor));
-				this.doSetSelection(this.activeEditor!, this.indexOf(this.activeEditor), newSelection);
+				const newInactiveSelection = this.selection.filter(selected => !selected.matches(editor));
+				this.doSetSelection(this.activeEditor!, this.indexOf(this.activeEditor), newInactiveSelection);
 			}
 		}
 
@@ -692,11 +692,11 @@ export class EditorGroupModel extends Disposable implements IEditorGroupModel {
 
 	private doSetActive(editor: EditorInput, editorIndex: number): void {
 		if (this.matches(this.active, editor)) {
-			this.selection = [editor];
+			this.selection = [editor]; // TODO this potentially changes selection without a related event. and is it expected to change the selection?
 			return; // already active
 		}
 
-		this.active = editor;
+		this.active = editor; // TODO this potentially changes selection without a related event. and is it expected to change the selection?
 
 		// Bring to front in MRU list
 		const mruIndex = this.indexOf(editor, this.mru);
@@ -712,14 +712,17 @@ export class EditorGroupModel extends Disposable implements IEditorGroupModel {
 		this._onDidModelChange.fire(event);
 	}
 
-	public get selectedEditors(): EditorInput[] {
+	get selectedEditors(): EditorInput[] {
 		// Return selected editors in sequential order
-		return this.editors.filter(editor => this.isSelected(editor));
+		return this.editors.filter(editor => this.isSelected(editor)); // TODO I would have assumed `this.selection` to be in sequential order
 	}
 
-	isSelected(editor: EditorInput | number): boolean {
-		if (typeof editor === 'number') {
-			editor = this.editors[editor];
+	isSelected(editorOrIndex: EditorInput | number): boolean { // TODO align with how isTransient() works
+		let editor: EditorInput;
+		if (typeof editorOrIndex === 'number') {
+			editor = this.editors[editorOrIndex];
+		} else {
+			editor = editorOrIndex;
 		}
 
 		return !!this.selection.find(selectedEditor => this.matches(selectedEditor, editor));
@@ -728,7 +731,7 @@ export class EditorGroupModel extends Disposable implements IEditorGroupModel {
 	setSelection(activeSelectedEditor: EditorInput, inactiveSelectedEditors: EditorInput[]): void {
 		const res = this.findEditor(activeSelectedEditor);
 		if (!res) {
-			return;
+			return; // not found
 		}
 
 		const [newActiveEditor, newActiveEditorIndex] = res;
@@ -739,14 +742,14 @@ export class EditorGroupModel extends Disposable implements IEditorGroupModel {
 	private doSetSelection(newActiveEditor: EditorInput, activeEditorIndex: number, inactiveSelectedEditors: EditorInput[]): void {
 		this.doSetActive(newActiveEditor, activeEditorIndex);
 
-		for (const candidate of inactiveSelectedEditors) {
+		for (const candidate of inactiveSelectedEditors) { // TODO should this move up to the public API? the idea is that we do not trust editor inputs from public API but internal is fine
 			const editor = this.findEditor(candidate)?.[0];
 			if (editor && !this.isSelected(editor)) {
 				this.selection.push(editor);
 			}
 		}
 
-		// Event
+		// Event TODO what if selection did not change?
 		const event: IGroupModelChangeEvent = {
 			kind: GroupModelChangeKind.EDITORS_SELECTION,
 		};

src/vs/workbench/common/editor/filteredEditorGroupModel.ts

@@ -42,7 +42,7 @@ abstract class FilteredEditorGroupModel extends Disposable implements IReadonlyE
 	isTransient(editorOrIndex: EditorInput | number): boolean { return this.model.isTransient(editorOrIndex); }
 	isSticky(editorOrIndex: EditorInput | number): boolean { return this.model.isSticky(editorOrIndex); }
 	isActive(editor: EditorInput | IUntypedEditorInput): boolean { return this.model.isActive(editor); }
-	isSelected(editor: EditorInput | number): boolean { return this.model.isSelected(editor); }
+	isSelected(editorOrIndex: EditorInput | number): boolean { return this.model.isSelected(editorOrIndex); }
 
 	isFirst(editor: EditorInput): boolean {
 		return this.model.isFirst(editor, this.getEditors(EditorsOrder.SEQUENTIAL));