Markdown Static Preview Scrolling Fixes #123727

andreamah · 2021-05-12T23:14:01Z

This PR addresses two bugs with scrolling in markdown static preview:

When toggling between the text editor and custom (static preview) editor, this should ensure that the scrolling position stays persistent. It currently loads the editor at the top regardless of where the user scrolled last.
When opening a static preview in one tab and a text editor for the same file in another column, scrolling the static preview scrolls the text editor, but scrolling the text editor does not scroll the static preview.

Steps to test:
Issue 1:

Open a markdown file that is long enough to need scrolling and scroll down.
Use the command palette to do View - Reopen Editor With... > Markdown Preview (Experimental)
- The resulting preview should be where you previously scrolled to in the text editor. Scroll on this page again.
Use the command palate to switch back to the text editor. It should load at the point where the static preview left off.

Issue 2:

Open a markdown file as a static preview (View - Reopen Editor With... > Markdown Preview (Experimental) in command palette).
Open the same file in another column, but in the regular text editor.
Scroll both sides. Both sides should scroll along with one another.

This PR addresses #84520

…ll-state

mjbvz

Looks like the right direction but I think the implementation could be cleaned up a little. Let me know if you have any questions about the comments

mjbvz · 2021-05-13T01:20:17Z

extensions/markdown-language-features/src/features/previewManager.ts

@@ -160,14 +160,18 @@ export class MarkdownPreviewManager extends Disposable implements vscode.Webview
 		document: vscode.TextDocument,
 		webview: vscode.WebviewPanel
 	): Promise<void> {
+		const lineNumber = this._topmostLineMonitor.previousMDTextEditor?.document.uri.toString() === document.uri.toString() ? this._topmostLineMonitor.previousMDTextEditor?.visibleRanges[0].start.line : undefined;


Instead of exposing previousMDTextEditor, can you hide this behind a method that gets the line for a given uri?

previousMDTextEditor changed to an internal map with public functions for access.

mjbvz · 2021-05-13T01:20:58Z

extensions/markdown-language-features/src/util/topmostLineMonitor.ts

+
+			// When at a markdown file, apply existing scroll settings from static preview if applicable.
+			// Also save reference to text editor for line number reference later
+			if (textEditor && isMarkdownFile(textEditor.document!)) {


[nit] Avoid using ! where possible

Should be fixed now.

mjbvz · 2021-05-13T01:21:31Z

extensions/markdown-language-features/src/util/topmostLineMonitor.ts

+
+				if (this.previousStaticEditorInfo.uri?.toString() === textEditor.document.uri.toString()) {
+					const line = this.previousStaticEditorInfo.line ? this.previousStaticEditorInfo.line : 0;
+					scrollEditorToLine(line, textEditor);


Should the TopMostLineMonitor being scrolling editors or can this live somewhere else?

Moved to MarkdownPreviewManager

mjbvz · 2021-05-13T01:23:03Z

extensions/markdown-language-features/src/util/topmostLineMonitor.ts

 export class TopmostLineMonitor extends Disposable {

 	private readonly pendingUpdates = new Map<string, number>();
 	private readonly throttle = 50;
+	public previousMDTextEditor: vscode.TextEditor | undefined;


Are there cases where you can more than one markdown file that we need to track the scroll position of? Try seeing if you can hit this using multiple editors and editor groups with different files

Changed the previous text editors to be indexed via string URI so that multiple editor instances can be accessed. This might get populated if the user has a lot of markdown files open (?)

mjbvz · 2021-05-13T01:25:05Z

extensions/markdown-language-features/src/features/previewManager.ts

@@ -220,6 +224,11 @@ export class MarkdownPreviewManager extends Disposable implements vscode.Webview
 			this._staticPreviews.delete(preview);
 		});

+		// Continuously update the scroll info in case user changes the editor.
+		preview.onScroll((scrollInfo) => {
+			this._topmostLineMonitor.previousStaticEditorInfo = scrollInfo;


Should this live in the preview itself?

Yup, accidentally forgot that I passed an instance of the topmost line monitor into the preview. Preview should now do this.

mjbvz · 2021-05-13T01:25:42Z

extensions/markdown-language-features/src/util/topmostLineMonitor.ts

@@ -7,13 +7,21 @@ import * as vscode from 'vscode';
 import { Disposable } from '../util/dispose';
 import { isMarkdownFile } from './file';

+export interface LastScrollLocation {
+	readonly line: number;
+	readonly uri: vscode.Uri | undefined;


What are the cases where uri can be undefined? Is this something we'd expect to happen?

Originally allowed for undefined for initialization; removed undefined.

andreamah · 2021-05-13T18:45:23Z

I noticed another bug where all other open text editors for a markdown file disappear when one instance enters static preview. It seems that this bug isn't caused by this PR's changes because it also occurs on main. I will try to look into it for this PR also.

…ll-state

andreamah · 2021-05-13T20:42:24Z

I noticed another bug where all other open text editors for a markdown file disappear when one instance enters static preview. It seems that this bug isn't caused by this PR's changes because it also occurs on main. I will try to look into it for this PR also.

When running the debugger, it seems that the other text editors are removed prior to entering the resolveCustomTextEditor function in the preview manager. Thus, I feel like it might be an issue outside of the extension code that is being focused on in this PR?

…ll-state

andreamah · 2021-05-19T15:46:02Z

I noticed another bug where all other open text editors for a markdown file disappear when one instance enters static preview. It seems that this bug isn't caused by this PR's changes because it also occurs on main. I will try to look into it for this PR also.

When running the debugger, it seems that the other text editors are removed prior to entering the resolveCustomTextEditor function in the preview manager. Thus, I feel like it might be an issue outside of the extension code that is being focused on in this PR?

Regarding this, I created #124134

mjbvz · 2021-05-19T17:46:41Z

extensions/markdown-language-features/src/util/topmostLineMonitor.ts

+
+	public setPreviousMDTextEditorLine(editor: vscode.TextEditor) {
+		const uri = editor.document.uri;
+		this.previousMDTextEditors.set(uri.toString(), editor);


The TextEditor object becomes invalid as soon as that editor is closed, so it's kind of dangerous to keep a reference to it around

Instead you can use window.visibleTextEditors to get all current editors

I think we need to keep information about closed text editors to get the scroll info when we switch editors. Since keeping text editors won't work, we can just keep the last scroll location as with the static preview scroll locations.

mjbvz · 2021-05-19T17:47:12Z

extensions/markdown-language-features/src/util/topmostLineMonitor.ts

+		this.previousMDTextEditors.set(uri.toString(), editor);
+	}
+
+	public getPreviousMDTextEditorLineByUri(resource: vscode.Uri) {


[nit] Add an explicit return types

mjbvz · 2021-05-19T17:48:25Z

extensions/markdown-language-features/src/util/topmostLineMonitor.ts

+				this.setPreviousMDTextEditorLine(textEditor);
+			}
+
+			this.isPrevEditorCustom = (textEditor === undefined);


isPrevCustomEditor will up being true when you do anything that causes there not to be an active text editor, such as switching to an extension page or maximizing the panel

Ok, will link isPrevCustomEditor to PreviewManager._activePreview == undefined

mjbvz · 2021-05-19T17:49:55Z

extensions/markdown-language-features/src/util/topmostLineMonitor.ts

+		return editor?.visibleRanges[0].start.line;
+	}
+
+	public setPreviousStaticEditorLine(scrollLocation: LastScrollLocation) {


Is this used anywhere?

used in preview.ts.

mjbvz · 2021-05-19T17:50:24Z

extensions/markdown-language-features/src/util/topmostLineMonitor.ts

+		this.previousStaticEditorInfo.set(scrollLocation.uri.toString(), scrollLocation);
+	}
+
+	public getPreviousStaticEditorLineByUri(resource: vscode.Uri) {


Is this used outside of this class?

nope, will change to private

mjbvz · 2021-05-19T17:54:50Z

extensions/markdown-language-features/src/util/topmostLineMonitor.ts

+				if (this.isPrevEditorCustom) {
+					const line = this.getPreviousStaticEditorLineByUri(textEditor.document.uri);
+					if (line) {
+						this._onEditorNeedsScrolling.fire({ line: line, editor: textEditor });


Would it make sense to have the previewManager instead monitor onDidChangeActiveTextEditor and then ask the TopMostLineMonitor if it need to scroll somewhere?

Sure, that would also functionally work. I can make that change.

mjbvz · 2021-05-19T17:55:54Z

extensions/markdown-language-features/src/features/preview.ts

@@ -284,6 +286,8 @@ class MarkdownPreview extends Disposable implements WebviewResourceProvider {
 		});
 	}

+


[nit] Remove extra spaces

…ll-state

andreamah · 2021-05-19T23:03:35Z

extensions/markdown-language-features/src/util/topmostLineMonitor.ts

 export class TopmostLineMonitor extends Disposable {

 	private readonly pendingUpdates = new Map<string, number>();
 	private readonly throttle = 50;
+	private previousEditorInfo = new Map<string, LastScrollLocation>();


Unlike the previous commits, line numbers from static previews and text editors are both tracked in this object. I believe that they were tracked in different objects before because text editors used the reference to the editor object to find line number, but both editors now update using the same way.

…ll-state

andreamah added 2 commits May 12, 2021 16:59

scroll state persists while toggling between static preview and text

1895018

Merge branch 'main' into dev/t-andreamah/markdown-static-preview-scro…

ee4510b

…ll-state

andreamah requested a review from mjbvz May 12, 2021 23:14

andreamah added this to the May 2021 milestone May 12, 2021

fixing issue with static scroll alongside text editor

d289456

andreamah changed the title ~~Initial Markdown Static Preview State~~ Markdown Static Preview Scrolling Fixes May 12, 2021

mjbvz reviewed May 13, 2021

View reviewed changes

egamma assigned mjbvz May 13, 2021

andreamah added 2 commits May 13, 2021 12:30

applied PR feedback

3dec3b3

addressed ! nit

9731995

Merge branch 'main' into dev/t-andreamah/markdown-static-preview-scro…

8a2157f

…ll-state

andreamah added 2 commits May 13, 2021 14:45

Merge branch 'main' into dev/t-andreamah/markdown-static-preview-scro…

0099c58

…ll-state

cleanup

9449877

andreamah mentioned this pull request May 14, 2021

Static Markdown Preview - Toggle Between Text Editor and Static Preview [WIP] #123924

Closed

mjbvz requested changes May 19, 2021

View reviewed changes

andreamah added 4 commits May 19, 2021 12:15

Merge branch 'main' into dev/t-andreamah/markdown-static-preview-scro…

44d135e

…ll-state

pr feedback and combining prevEditor info for preview and non-preview

9ee1906

removing unecessary spaces

41606da

removing more unecessary whitespace

f594bb4

andreamah commented May 19, 2021

View reviewed changes

Merge branch 'main' into dev/t-andreamah/markdown-static-preview-scro…

4fbd548

…ll-state

mjbvz approved these changes May 20, 2021

View reviewed changes

Merge branch 'main' into dev/t-andreamah/markdown-static-preview-scro…

18c2549

…ll-state

andreamah enabled auto-merge May 20, 2021 16:00

andreamah merged commit ab5df44 into main May 20, 2021

andreamah deleted the dev/t-andreamah/markdown-static-preview-scroll-state branch May 20, 2021 16:03

andreamah mentioned this pull request Jun 14, 2021

Fix #126178 #126285

Merged

andreamah mentioned this pull request Jun 14, 2021

Fix #126178 (Recovery) #126293

Merged

github-actions bot locked and limited conversation to collaborators Jul 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Markdown Static Preview Scrolling Fixes #123727

Markdown Static Preview Scrolling Fixes #123727

andreamah commented May 12, 2021 •

edited

mjbvz left a comment

mjbvz May 13, 2021

andreamah May 13, 2021

mjbvz May 13, 2021

andreamah May 13, 2021

mjbvz May 13, 2021

andreamah May 13, 2021

mjbvz May 13, 2021

andreamah May 13, 2021

mjbvz May 13, 2021

andreamah May 13, 2021

mjbvz May 13, 2021

andreamah May 13, 2021

andreamah commented May 13, 2021

andreamah commented May 13, 2021

andreamah commented May 19, 2021

mjbvz May 19, 2021

andreamah May 19, 2021

mjbvz May 19, 2021

mjbvz May 19, 2021

andreamah May 19, 2021

mjbvz May 19, 2021

andreamah May 19, 2021 •

edited

mjbvz May 19, 2021

andreamah May 19, 2021

mjbvz May 19, 2021

andreamah May 19, 2021

mjbvz May 19, 2021

andreamah May 19, 2021

		@@ -284,6 +286,8 @@ class MarkdownPreview extends Disposable implements WebviewResourceProvider {
		});
		}

Markdown Static Preview Scrolling Fixes #123727

Markdown Static Preview Scrolling Fixes #123727

Conversation

andreamah commented May 12, 2021 • edited

mjbvz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreamah commented May 13, 2021

andreamah commented May 13, 2021

andreamah commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreamah May 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreamah commented May 12, 2021 •

edited

andreamah May 19, 2021 •

edited