debt - make history suspend more explicit

src/vs/workbench/contrib/debug/test/browser/debugConfigurationManager.test.ts

@@ -20,8 +20,8 @@ import { UriIdentityService } from 'vs/platform/uriIdentity/common/uriIdentitySe
 import { ConfigurationManager } from 'vs/workbench/contrib/debug/browser/debugConfigurationManager';
 import { DebugConfigurationProviderTriggerKind, IAdapterManager, IConfig, IDebugAdapterExecutable, IDebugSession } from 'vs/workbench/contrib/debug/common/debug';
 import { IPreferencesService } from 'vs/workbench/services/preferences/common/preferences';
-import { TestHistoryService, TestQuickInputService } from 'vs/workbench/test/browser/workbenchTestServices';
-import { TestContextService, TestExtensionService, TestStorageService } from 'vs/workbench/test/common/workbenchTestServices';
+import { TestQuickInputService } from 'vs/workbench/test/browser/workbenchTestServices';
+import { TestHistoryService, TestContextService, TestExtensionService, TestStorageService } from 'vs/workbench/test/common/workbenchTestServices';
 
 suite('debugConfigurationManager', () => {
 	const configurationProviderType = 'custom-type';

src/vs/workbench/contrib/search/browser/quickTextSearch/textSearchQuickAccess.ts

@@ -125,12 +125,15 @@ export class TextSearchQuickAccess extends PickerQuickAccessProvider<ITextSearch
 				const itemMatch = item.match;
 				this.editorSequencer.queue(async () => {
 					// disable and re-enable history service so that we can ignore this history entry
-					this._historyService.shouldIgnoreActiveEditorChange = true;
-					await this._editorService.openEditor({
-						resource: itemMatch.parent().resource,
-						options: { preserveFocus: true, revealIfOpened: true, ignoreError: true, selection: itemMatch.range() }
-					});
-					this._historyService.shouldIgnoreActiveEditorChange = false;
+					const disposable = this._historyService.suspendTracking();
+					try {
+						await this._editorService.openEditor({
+							resource: itemMatch.parent().resource,
+							options: { preserveFocus: true, revealIfOpened: true, ignoreError: true, selection: itemMatch.range() }
+						});
+					} finally {
+						disposable.dispose();
+					}
 				});
 			}
 		}));

src/vs/workbench/services/history/browser/historyService.ts

@@ -12,7 +12,7 @@ import { IEditorService } from 'vs/workbench/services/editor/common/editorServic
 import { GoFilter, GoScope, IHistoryService } from 'vs/workbench/services/history/common/history';
 import { FileChangesEvent, IFileService, FileChangeType, FILES_EXCLUDE_CONFIG, FileOperationEvent, FileOperation } from 'vs/platform/files/common/files';
 import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace';
-import { dispose, Disposable, DisposableStore, IDisposable } from 'vs/base/common/lifecycle';
+import { dispose, Disposable, DisposableStore, IDisposable, toDisposable } from 'vs/base/common/lifecycle';
 import { IStorageService, StorageScope, StorageTarget } from 'vs/platform/storage/common/storage';
 import { Emitter, Event } from 'vs/base/common/event';
 import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
@@ -47,9 +47,7 @@ export class HistoryService extends Disposable implements IHistoryService {
 
 	private readonly editorHelper = this.instantiationService.createInstance(EditorHelper);
 
-	// Can be set to temporarily ignore messages from the editor service that indicate a new active editor.
-	// Used for ignoring some editors for history.
-	public shouldIgnoreActiveEditorChange: boolean = false;
+
 
 	constructor(
 		@IEditorService private readonly editorService: EditorServiceImpl,
@@ -75,21 +73,28 @@ export class HistoryService extends Disposable implements IHistoryService {
 		}
 	}
 
+	private trackingSuspended = false;
+	suspendTracking(): IDisposable {
+		this.trackingSuspended = true;
+
+		return toDisposable(() => this.trackingSuspended = false);
+	}
+
 	private registerListeners(): void {
 
 		// Mouse back/forward support
 		this.registerMouseNavigationListener();
 
 		// Editor changes
 		this._register(this.editorService.onDidActiveEditorChange((e) => {
-			if (!this.shouldIgnoreActiveEditorChange) {
+			if (!this.trackingSuspended) {
 				this.onDidActiveEditorChange();
 			}
 		}));
 		this._register(this.editorService.onDidOpenEditorFail(event => this.remove(event.editor)));
 		this._register(this.editorService.onDidCloseEditor(event => this.onDidCloseEditor(event)));
 		this._register(this.editorService.onDidMostRecentlyActiveEditorsChange(() => {
-			if (!this.shouldIgnoreActiveEditorChange) {
+			if (!this.trackingSuspended) {
 				this.handleEditorEventInRecentEditorsStack();
 			}
 		}));
@@ -190,11 +195,10 @@ export class HistoryService extends Disposable implements IHistoryService {
 		// is having a selection concept.
 		if (isEditorPaneWithSelection(activeEditorPane)) {
 			this.activeEditorListeners.add(activeEditorPane.onDidChangeSelection(e => {
-				if (!this.shouldIgnoreActiveEditorChange) {
+				if (!this.trackingSuspended) {
 					this.handleActiveEditorSelectionChangeEvent(activeEditorGroup, activeEditorPane, e);
 				}
-			}
-			));
+			}));
 		}
 
 		// Context keys

src/vs/workbench/services/history/common/history.ts

@@ -8,6 +8,7 @@ import { IResourceEditorInput } from 'vs/platform/editor/common/editor';
 import { GroupIdentifier } from 'vs/workbench/common/editor';
 import { EditorInput } from 'vs/workbench/common/editor/editorInput';
 import { URI } from 'vs/base/common/uri';
+import { IDisposable } from 'vs/base/common/lifecycle';
 
 export const IHistoryService = createDecorator<IHistoryService>('historyService');
 
@@ -60,7 +61,6 @@ export const enum GoScope {
 export interface IHistoryService {
 
 	readonly _serviceBrand: undefined;
-	shouldIgnoreActiveEditorChange: boolean;
 
 	/**
 	 * Navigate forwards in editor navigation history.
@@ -138,4 +138,9 @@ export interface IHistoryService {
 	 * Clear list of recently opened editors.
 	 */
 	clearRecentlyOpened(): void;
+
+	/**
+	 * Temporarily suspend tracking of editor events for the history.
+	 */
+	suspendTracking(): IDisposable;
 }

src/vs/workbench/services/history/test/browser/historyService.test.ts

@@ -807,5 +807,143 @@ suite('HistoryService', function () {
 		return workbenchTeardown(instantiationService);
 	});
 
+	test('suspend should suspend editor changes- skip two editors and continue (single group)', async () => {
+		const [part, historyService, editorService, , instantiationService] = await createServices();
+
+		const input1 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar1'), TEST_EDITOR_INPUT_ID));
+		const input2 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar2'), TEST_EDITOR_INPUT_ID));
+		const input3 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar3'), TEST_EDITOR_INPUT_ID));
+		const input4 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar4'), TEST_EDITOR_INPUT_ID));
+		const input5 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar5'), TEST_EDITOR_INPUT_ID));
+
+		let editorChangePromise = Event.toPromise(editorService.onDidActiveEditorChange);
+		await part.activeGroup.openEditor(input1, { pinned: true });
+		assert.strictEqual(part.activeGroup.activeEditor, input1);
+		await editorChangePromise;
+
+		const disposable = historyService.suspendTracking();
+
+		// wait on two editor changes before disposing
+		editorChangePromise = Event.toPromise(editorService.onDidActiveEditorChange)
+			.then(() => Event.toPromise(editorService.onDidActiveEditorChange));
+
+		await part.activeGroup.openEditor(input2, { pinned: true });
+		assert.strictEqual(part.activeGroup.activeEditor, input2);
+		await part.activeGroup.openEditor(input3, { pinned: true });
+		assert.strictEqual(part.activeGroup.activeEditor, input3);
+
+		await editorChangePromise;
+		disposable.dispose();
+
+		await part.activeGroup.openEditor(input4, { pinned: true });
+		assert.strictEqual(part.activeGroup.activeEditor, input4);
+		await part.activeGroup.openEditor(input5, { pinned: true });
+		assert.strictEqual(part.activeGroup.activeEditor, input5);
+
+		// stack should be [input1, input4, input5]
+		await historyService.goBack();
+		assert.strictEqual(part.activeGroup.activeEditor, input4);
+		await historyService.goBack();
+		assert.strictEqual(part.activeGroup.activeEditor, input1);
+		await historyService.goBack();
+		assert.strictEqual(part.activeGroup.activeEditor, input1);
+
+		await historyService.goForward();
+		assert.strictEqual(part.activeGroup.activeEditor, input4);
+		await historyService.goForward();
+		assert.strictEqual(part.activeGroup.activeEditor, input5);
+
+		return workbenchTeardown(instantiationService);
+	});
+
+	test('suspend should suspend editor changes- skip two editors and continue (multi group)', async () => {
+		const [part, historyService, editorService, , instantiationService] = await createServices();
+		const rootGroup = part.activeGroup;
+
+		const input1 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar1'), TEST_EDITOR_INPUT_ID));
+		const input2 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar2'), TEST_EDITOR_INPUT_ID));
+		const input3 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar3'), TEST_EDITOR_INPUT_ID));
+		const input4 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar4'), TEST_EDITOR_INPUT_ID));
+		const input5 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar5'), TEST_EDITOR_INPUT_ID));
+
+		const sideGroup = part.addGroup(rootGroup, GroupDirection.RIGHT);
+
+		let editorChangePromise = Event.toPromise(editorService.onDidActiveEditorChange);
+		await rootGroup.openEditor(input1, { pinned: true });
+		await editorChangePromise;
+
+		const disposable = historyService.suspendTracking();
+		editorChangePromise = Event.toPromise(editorService.onDidActiveEditorChange)
+			.then(() => Event.toPromise(editorService.onDidActiveEditorChange));
+		await sideGroup.openEditor(input2, { pinned: true });
+		await rootGroup.openEditor(input3, { pinned: true });
+		await editorChangePromise;
+		disposable.dispose();
+
+		await sideGroup.openEditor(input4, { pinned: true });
+		await rootGroup.openEditor(input5, { pinned: true });
+
+		// stack should be [input1, input4, input5]
+		await historyService.goBack();
+		assert.strictEqual(part.activeGroup.activeEditor, input4);
+		assert.strictEqual(part.activeGroup, sideGroup);
+		assert.strictEqual(rootGroup.activeEditor, input5);
+
+		await historyService.goBack();
+		assert.strictEqual(part.activeGroup.activeEditor, input1);
+		assert.strictEqual(part.activeGroup, rootGroup);
+		assert.strictEqual(sideGroup.activeEditor, input4);
+
+		await historyService.goBack();
+		assert.strictEqual(part.activeGroup.activeEditor, input1);
+
+		await historyService.goForward();
+		assert.strictEqual(part.activeGroup.activeEditor, input4);
+		await historyService.goForward();
+		assert.strictEqual(part.activeGroup.activeEditor, input5);
+
+		return workbenchTeardown(instantiationService);
+	});
+
+	test('suspend should suspend editor changes - interleaved skips', async () => {
+		const [part, historyService, editorService, , instantiationService] = await createServices();
+
+		const input1 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar1'), TEST_EDITOR_INPUT_ID));
+		const input2 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar2'), TEST_EDITOR_INPUT_ID));
+		const input3 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar3'), TEST_EDITOR_INPUT_ID));
+		const input4 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar4'), TEST_EDITOR_INPUT_ID));
+		const input5 = disposables.add(new TestFileEditorInput(URI.parse('foo://bar5'), TEST_EDITOR_INPUT_ID));
+
+		let editorChangePromise = Event.toPromise(editorService.onDidActiveEditorChange);
+		await part.activeGroup.openEditor(input1, { pinned: true });
+		await editorChangePromise;
+
+		let disposable = historyService.suspendTracking();
+		editorChangePromise = Event.toPromise(editorService.onDidActiveEditorChange);
+		await part.activeGroup.openEditor(input2, { pinned: true });
+		await editorChangePromise;
+		disposable.dispose();
+
+		await part.activeGroup.openEditor(input3, { pinned: true });
+
+		disposable = historyService.suspendTracking();
+		editorChangePromise = Event.toPromise(editorService.onDidActiveEditorChange);
+		await part.activeGroup.openEditor(input4, { pinned: true });
+		await editorChangePromise;
+		disposable.dispose();
+
+		await part.activeGroup.openEditor(input5, { pinned: true });
+
+		// stack should be [input1, input3, input5]
+		await historyService.goBack();
+		assert.strictEqual(part.activeGroup.activeEditor, input3);
+		await historyService.goBack();
+		assert.strictEqual(part.activeGroup.activeEditor, input1);
+		await historyService.goBack();
+		assert.strictEqual(part.activeGroup.activeEditor, input1);
+
+		return workbenchTeardown(instantiationService);
+	});
+
 	ensureNoDisposablesAreLeakedInTestSuite();
 });

src/vs/workbench/test/browser/workbenchTestServices.ts

@@ -98,7 +98,7 @@ import { IInputBox, IInputOptions, IPickOptions, IQuickInputButton, IQuickInputS
 import { QuickInputService } from 'vs/workbench/services/quickinput/browser/quickInputService';
 import { IListService } from 'vs/platform/list/browser/listService';
 import { win32, posix } from 'vs/base/common/path';
-import { TestContextService, TestStorageService, TestTextResourcePropertiesService, TestExtensionService, TestProductService, createFileStat, TestLoggerService, TestWorkspaceTrustManagementService, TestWorkspaceTrustRequestService, TestMarkerService } from 'vs/workbench/test/common/workbenchTestServices';
+import { TestContextService, TestStorageService, TestTextResourcePropertiesService, TestExtensionService, TestProductService, createFileStat, TestLoggerService, TestWorkspaceTrustManagementService, TestWorkspaceTrustRequestService, TestMarkerService, TestHistoryService } from 'vs/workbench/test/common/workbenchTestServices';
 import { IView, ViewContainer, ViewContainerLocation } from 'vs/workbench/common/views';
 import { IViewsService } from 'vs/workbench/services/views/common/viewsService';
 import { IPaneComposite } from 'vs/workbench/common/panecomposite';
@@ -545,28 +545,6 @@ export class TestMenuService implements IMenuService {
 	}
 }
 
-export class TestHistoryService implements IHistoryService {
-
-	declare readonly _serviceBrand: undefined;
-	declare shouldIgnoreActiveEditorChange: boolean;
-
-	constructor(private root?: URI) { }
-
-	async reopenLastClosedEditor(): Promise<void> { }
-	async goForward(): Promise<void> { }
-	async goBack(): Promise<void> { }
-	async goPrevious(): Promise<void> { }
-	async goLast(): Promise<void> { }
-	removeFromHistory(_input: EditorInput | IResourceEditorInput): void { }
-	clear(): void { }
-	clearRecentlyOpened(): void { }
-	getHistory(): readonly (EditorInput | IResourceEditorInput)[] { return []; }
-	async openNextRecentlyUsedEditor(group?: GroupIdentifier): Promise<void> { }
-	async openPreviouslyUsedEditor(group?: GroupIdentifier): Promise<void> { }
-	getLastActiveWorkspaceRoot(_schemeFilter: string): URI | undefined { return this.root; }
-	getLastActiveFile(_schemeFilter: string): URI | undefined { return undefined; }
-}
-
 export class TestFileDialogService implements IFileDialogService {
 
 	declare readonly _serviceBrand: undefined;

src/vs/workbench/test/common/workbenchTestServices.ts

@@ -149,7 +149,6 @@ export class TestStorageService extends InMemoryStorageService {
 export class TestHistoryService implements IHistoryService {
 
 	declare readonly _serviceBrand: undefined;
-	declare shouldIgnoreActiveEditorChange: boolean;
 
 	constructor(private root?: URI) { }
 
@@ -166,6 +165,7 @@ export class TestHistoryService implements IHistoryService {
 	async openPreviouslyUsedEditor(group?: GroupIdentifier): Promise<void> { }
 	getLastActiveWorkspaceRoot(_schemeFilter: string): URI | undefined { return this.root; }
 	getLastActiveFile(_schemeFilter: string): URI | undefined { return undefined; }
+	suspendTracking() { return Disposable.None; }
 }
 
 export class TestWorkingCopy extends Disposable implements IWorkingCopy {