text editors - cleanup view state handling

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

@@ -9,7 +9,7 @@ import { isObject, isArray, assertIsDefined, withUndefinedAsNull, withNullAsUnde
 import { IDiffEditor, isDiffEditor } from 'vs/editor/browser/editorBrowser';
 import { IDiffEditorOptions, IEditorOptions as ICodeEditorOptions } from 'vs/editor/common/config/editorOptions';
 import { BaseTextEditor, IEditorConfiguration } from 'vs/workbench/browser/parts/editor/textEditor';
-import { TEXT_DIFF_EDITOR_ID, IEditorFactoryRegistry, EditorExtensions, ITextDiffEditorPane, IEditorOpenContext, EditorInputCapabilities, isEditorInput } from 'vs/workbench/common/editor';
+import { TEXT_DIFF_EDITOR_ID, IEditorFactoryRegistry, EditorExtensions, ITextDiffEditorPane, IEditorOpenContext, EditorInputCapabilities, isEditorInput, isTextEditorViewState } from 'vs/workbench/common/editor';
 import { EditorInput } from 'vs/workbench/common/editor/editorInput';
 import { applyTextEditorOptions } from 'vs/workbench/common/editor/editorOptions';
 import { DiffEditorInput } from 'vs/workbench/common/editor/diffEditorInput';
@@ -140,18 +140,18 @@ export class TextDiffEditor extends BaseTextEditor<IDiffEditorViewState> impleme
 			const resolvedDiffEditorModel = resolvedModel as TextDiffEditorModel;
 			diffEditor.setModel(withUndefinedAsNull(resolvedDiffEditorModel.textDiffEditorModel));
 
-			/// Apply options to editor if any
+			// Restore view state (unless provided by options)
+			let hasPreviousViewState = false;
+			if (!isTextEditorViewState(options?.viewState)) {
+				hasPreviousViewState = this.restoreTextDiffEditorViewState(input, options, context, diffEditor);
+			}
+
+			// Apply options to editor if any
 			let optionsGotApplied = false;
 			if (options) {
 				optionsGotApplied = applyTextEditorOptions(options, diffEditor, ScrollType.Immediate);
 			}
 
-			// Otherwise restore View State unless disabled via settings
-			let hasPreviousViewState = false;
-			if (!optionsGotApplied) {
-				hasPreviousViewState = this.restoreTextDiffEditorViewState(input, context, diffEditor);
-			}
-
 			// Diff navigator
 			this.diffNavigator = new DiffNavigator(diffEditor, {
 				alwaysRevealFirst: !optionsGotApplied && !hasPreviousViewState // only reveal first change if we had no options or viewstate
@@ -179,10 +179,14 @@ export class TextDiffEditor extends BaseTextEditor<IDiffEditorViewState> impleme
 		}
 	}
 
-	private restoreTextDiffEditorViewState(editor: DiffEditorInput, context: IEditorOpenContext, control: IDiffEditor): boolean {
-		const viewState = this.loadEditorViewState(editor, context);
-		if (viewState) {
-			control.restoreViewState(viewState);
+	private restoreTextDiffEditorViewState(editor: DiffEditorInput, options: ITextEditorOptions | undefined, context: IEditorOpenContext, control: IDiffEditor): boolean {
+		const editorViewState = this.loadEditorViewState(editor, context);
+		if (editorViewState) {
+			if (options?.selection && editorViewState.modified) {
+				editorViewState.modified.cursorState = []; // prevent duplicate selections via options
+			}
+
+			control.restoreViewState(editorViewState);
 
 			return true;
 		}

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

@@ -6,7 +6,7 @@
 import { localize } from 'vs/nls';
 import { assertIsDefined, withNullAsUndefined } from 'vs/base/common/types';
 import { ICodeEditor, getCodeEditor, IPasteEvent } from 'vs/editor/browser/editorBrowser';
-import { IEditorOpenContext } from 'vs/workbench/common/editor';
+import { IEditorOpenContext, isTextEditorViewState } from 'vs/workbench/common/editor';
 import { EditorInput } from 'vs/workbench/common/editor/editorInput';
 import { applyTextEditorOptions } from 'vs/workbench/common/editor/editorOptions';
 import { AbstractTextResourceEditorInput, TextResourceEditorInput } from 'vs/workbench/common/editor/textResourceEditorInput';
@@ -77,15 +77,21 @@ export class AbstractTextResourceEditor extends BaseTextEditor<ICodeEditorViewSt
 		const textEditorModel = resolvedModel.textEditorModel;
 		textEditor.setModel(textEditorModel);
 
-		// Apply options to editor if any
-		let optionsGotApplied = false;
-		if (options) {
-			optionsGotApplied = applyTextEditorOptions(options, textEditor, ScrollType.Immediate);
+		// Restore view state (unless provided by options)
+		if (!isTextEditorViewState(options?.viewState)) {
+			const editorViewState = this.loadEditorViewState(input, context);
+			if (editorViewState) {
+				if (options?.selection) {
+					editorViewState.cursorState = []; // prevent duplicate selections via options
+				}
+
+				textEditor.restoreViewState(editorViewState);
+			}
 		}
 
-		// Otherwise restore View State unless disabled via settings
-		if (!optionsGotApplied) {
-			this.restoreTextResourceEditorViewState(input, context, textEditor);
+		// Apply options to editor if any
+		if (options) {
+			applyTextEditorOptions(options, textEditor, ScrollType.Immediate);
 		}
 
 		// Since the resolved model provides information about being readonly
@@ -96,13 +102,6 @@ export class AbstractTextResourceEditor extends BaseTextEditor<ICodeEditorViewSt
 		textEditor.updateOptions({ readOnly: resolvedModel.isReadonly() });
 	}
 
-	private restoreTextResourceEditorViewState(editor: AbstractTextResourceEditorInput, context: IEditorOpenContext, control: IEditor) {
-		const viewState = this.loadEditorViewState(editor, context);
-		if (viewState) {
-			control.restoreViewState(viewState);
-		}
-	}
-
 	/**
 	 * Reveals the last line of this editor if it has a model set.
 	 */

src/vs/workbench/common/editor.ts

@@ -8,7 +8,7 @@ import { Event } from 'vs/base/common/event';
 import { assertIsDefined } from 'vs/base/common/types';
 import { URI } from 'vs/base/common/uri';
 import { Disposable, IDisposable, toDisposable } from 'vs/base/common/lifecycle';
-import { IDiffEditor } from 'vs/editor/common/editorCommon';
+import { ICodeEditorViewState, IDiffEditor, IDiffEditorViewState, IEditorViewState } from 'vs/editor/common/editorCommon';
 import { IEditorOptions, ITextEditorOptions, IResourceEditorInput, ITextResourceEditorInput, IBaseTextResourceEditorInput, IBaseUntypedEditorInput } from 'vs/platform/editor/common/editor';
 import type { EditorInput } from 'vs/workbench/common/editor/editorInput';
 import { IInstantiationService, IConstructorSignature, ServicesAccessor, BrandedService } from 'vs/platform/instantiation/common/instantiation';
@@ -1325,3 +1325,19 @@ export const enum EditorsOrder {
 	 */
 	SEQUENTIAL
 }
+
+export function isTextEditorViewState(candidate: unknown): candidate is IEditorViewState {
+	const viewState = candidate as IEditorViewState | undefined;
+	if (!viewState) {
+		return false;
+	}
+
+	const diffEditorViewState = viewState as IDiffEditorViewState;
+	if (diffEditorViewState.modified) {
+		return isTextEditorViewState(diffEditorViewState.modified);
+	}
+
+	const codeEditorViewState = viewState as ICodeEditorViewState;
+
+	return !!(codeEditorViewState.contributionsState && codeEditorViewState.viewState && Array.isArray(codeEditorViewState.cursorState));
+}

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

@@ -4,15 +4,16 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { IRange } from 'vs/editor/common/core/range';
-import { ICodeEditorViewState, IDiffEditorViewState, IEditor, IEditorViewState, ScrollType } from 'vs/editor/common/editorCommon';
+import { ICodeEditorViewState, IDiffEditorViewState, IEditor, ScrollType } from 'vs/editor/common/editorCommon';
 import { ITextEditorOptions, TextEditorSelectionRevealType, TextEditorSelectionSource } from 'vs/platform/editor/common/editor';
+import { isTextEditorViewState } from 'vs/workbench/common/editor';
 
 export function applyTextEditorOptions(options: ITextEditorOptions, editor: IEditor, scrollType: ScrollType): boolean {
 	let applied = false;
 
 	// Restore view state if any
 	const viewState = massageEditorViewState(options);
-	if (viewState) {
+	if (isTextEditorViewState(viewState)) {
 		editor.restoreViewState(viewState);
 
 		applied = true;
@@ -50,11 +51,11 @@ export function applyTextEditorOptions(options: ITextEditorOptions, editor: IEdi
 	return applied;
 }
 
-function massageEditorViewState(options: ITextEditorOptions): IEditorViewState | undefined {
+function massageEditorViewState(options: ITextEditorOptions): object | undefined {
 
 	// Without a selection or view state, just return immediately
 	if (!options.selection || !options.viewState) {
-		return options.viewState as IEditorViewState | undefined;
+		return options.viewState;
 	}
 
 	// Diff editor: since we have an explicit selection, clear the

src/vs/workbench/contrib/files/browser/editors/textFileEditor.ts

@@ -10,7 +10,7 @@ import { toAction } from 'vs/base/common/actions';
 import { VIEWLET_ID, TEXT_FILE_EDITOR_ID } from 'vs/workbench/contrib/files/common/files';
 import { ITextFileService, TextFileOperationError, TextFileOperationResult } from 'vs/workbench/services/textfile/common/textfiles';
 import { BaseTextEditor } from 'vs/workbench/browser/parts/editor/textEditor';
-import { IEditorOpenContext, EditorInputCapabilities } from 'vs/workbench/common/editor';
+import { IEditorOpenContext, EditorInputCapabilities, isTextEditorViewState } from 'vs/workbench/common/editor';
 import { EditorInput } from 'vs/workbench/common/editor/editorInput';
 import { applyTextEditorOptions } from 'vs/workbench/common/editor/editorOptions';
 import { BinaryEditorModel } from 'vs/workbench/common/editor/binaryEditorModel';
@@ -140,16 +140,11 @@ export class TextFileEditor extends BaseTextEditor<ICodeEditorViewState> {
 			textEditor.setModel(textFileModel.textEditorModel);
 
 			// Restore view state (unless provided by options)
-			if (!options?.viewState) {
+			if (!isTextEditorViewState(options?.viewState)) {
 				const editorViewState = this.loadEditorViewState(input, context);
 				if (editorViewState) {
 					if (options?.selection) {
-						// If we have a selection, make sure to not
-						// restore any selection from the view state
-						// to ensure the right selection change event
-						// is fired and we avoid changing selections
-						// twice.
-						editorViewState.cursorState = [];
+						editorViewState.cursorState = []; // prevent duplicate selections via options
 					}
 
 					textEditor.restoreViewState(editorViewState);

src/vs/workbench/services/editor/browser/codeEditorService.ts

@@ -57,7 +57,7 @@ export class CodeEditorService extends AbstractCodeEditorService {
 			input.resource &&															// we need a request resource to compare with
 			source === activeTextEditorControl.getModifiedEditor() && 					// we need the source of this request to be the modified side of the diff editor
 			activeTextEditorControl.getModel() &&										// we need a target model to compare with
-			isEqual(input.resource, activeTextEditorControl.getModel()!.modified.uri) 	// we need the input resources to match with modified side
+			isEqual(input.resource, activeTextEditorControl.getModel()?.modified.uri) 	// we need the input resources to match with modified side
 		) {
 			const targetEditor = activeTextEditorControl.getModifiedEditor();
 

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

@@ -4,7 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import * as assert from 'assert';
-import { EditorResourceAccessor, SideBySideEditor, EditorInputWithPreferredResource, EditorInputCapabilities, isEditorIdentifier, IResourceDiffEditorInput, IUntitledTextResourceEditorInput, isResourceEditorInput, isUntitledResourceEditorInput, isResourceDiffEditorInput, isEditorInputWithOptionsAndGroup, EditorInputWithOptions, isEditorInputWithOptions, isEditorInput, EditorInputWithOptionsAndGroup, isResourceSideBySideEditorInput, IResourceSideBySideEditorInput } from 'vs/workbench/common/editor';
+import { EditorResourceAccessor, SideBySideEditor, EditorInputWithPreferredResource, EditorInputCapabilities, isEditorIdentifier, IResourceDiffEditorInput, IUntitledTextResourceEditorInput, isResourceEditorInput, isUntitledResourceEditorInput, isResourceDiffEditorInput, isEditorInputWithOptionsAndGroup, EditorInputWithOptions, isEditorInputWithOptions, isEditorInput, EditorInputWithOptionsAndGroup, isResourceSideBySideEditorInput, IResourceSideBySideEditorInput, isTextEditorViewState } from 'vs/workbench/common/editor';
 import { DiffEditorInput } from 'vs/workbench/common/editor/diffEditorInput';
 import { URI } from 'vs/base/common/uri';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
@@ -20,6 +20,8 @@ import { EditorService } from 'vs/workbench/services/editor/browser/editorServic
 import { IEditorService } from 'vs/workbench/services/editor/common/editorService';
 import { SideBySideEditorInput } from 'vs/workbench/common/editor/sideBySideEditorInput';
 import { EditorResolution, IResourceEditorInput } from 'vs/platform/editor/common/editor';
+import { ICodeEditorViewState, IDiffEditorViewState } from 'vs/editor/common/editorCommon';
+import { Position } from 'vs/editor/common/core/position';
 
 suite('Workbench editor utils', () => {
 
@@ -364,6 +366,30 @@ suite('Workbench editor utils', () => {
 		assert.strictEqual(isEditorInputWithOptionsAndGroup(editorInputWithOptionsAndGroup), true);
 	});
 
+	test('isTextEditorViewState', () => {
+		assert.strictEqual(isTextEditorViewState(undefined), false);
+		assert.strictEqual(isTextEditorViewState({}), false);
+
+		const codeEditorViewState: ICodeEditorViewState = {
+			contributionsState: {},
+			cursorState: [],
+			viewState: {
+				scrollLeft: 0,
+				firstPosition: new Position(1, 1),
+				firstPositionDeltaTop: 1
+			}
+		};
+
+		assert.strictEqual(isTextEditorViewState(codeEditorViewState), true);
+
+		const diffEditorViewState: IDiffEditorViewState = {
+			original: codeEditorViewState,
+			modified: codeEditorViewState
+		};
+
+		assert.strictEqual(isTextEditorViewState(diffEditorViewState), true);
+	});
+
 	test('whenEditorClosed (single editor)', async function () {
 		return testWhenEditorClosed(false, false, toResource.call(this, '/path/index.txt'));
 	});