-
Notifications
You must be signed in to change notification settings - Fork 29k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Markdown Static Preview Scrolling Fixes #123727
Changes from 3 commits
1895018
ee4510b
d289456
3dec3b3
9731995
8a2157f
0099c58
9449877
44d135e
9ee1906
41606da
f594bb4
4fbd548
18c2549
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
const preview = StaticMarkdownPreview.revive( | ||
document.uri, | ||
webview, | ||
this._contentProvider, | ||
this._previewConfigurations, | ||
this._topmostLineMonitor, | ||
this._logger, | ||
this._contributions, | ||
this._engine); | ||
this._engine, | ||
lineNumber | ||
); | ||
this.registerStaticPreview(preview); | ||
} | ||
|
||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this live in the preview itself? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, accidentally forgot that I passed an instance of the topmost line monitor into the preview. Preview should now do this. |
||
}); | ||
|
||
this.trackActive(preview); | ||
return preview; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are the cases where uri can be undefined? Is this something we'd expect to happen? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Originally allowed for undefined for initialization; removed undefined. |
||
} | ||
|
||
export class TopmostLineMonitor extends Disposable { | ||
|
||
private readonly pendingUpdates = new Map<string, number>(); | ||
private readonly throttle = 50; | ||
public previousMDTextEditor: vscode.TextEditor | undefined; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 (?) |
||
public previousStaticEditorInfo: LastScrollLocation = { line: 0, uri: undefined }; | ||
|
||
constructor() { | ||
super(); | ||
this.previousMDTextEditor = vscode.window.activeTextEditor; | ||
this._register(vscode.window.onDidChangeTextEditorVisibleRanges(event => { | ||
if (isMarkdownFile(event.textEditor.document)) { | ||
const line = getVisibleLine(event.textEditor); | ||
|
@@ -22,6 +30,21 @@ export class TopmostLineMonitor extends Disposable { | |
} | ||
} | ||
})); | ||
|
||
this._register(vscode.window.onDidChangeActiveTextEditor(textEditor => { | ||
|
||
// 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!)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] Avoid using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be fixed now. |
||
|
||
if (this.previousStaticEditorInfo.uri?.toString() === textEditor.document.uri.toString()) { | ||
const line = this.previousStaticEditorInfo.line ? this.previousStaticEditorInfo.line : 0; | ||
scrollEditorToLine(line, textEditor); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the TopMostLineMonitor being scrolling editors or can this live somewhere else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to |
||
} | ||
|
||
this.previousMDTextEditor = textEditor; | ||
} | ||
})); | ||
} | ||
|
||
private readonly _onChanged = this._register(new vscode.EventEmitter<{ readonly resource: vscode.Uri, readonly line: number }>()); | ||
|
@@ -68,3 +91,18 @@ export function getVisibleLine( | |
const progress = firstVisiblePosition.character / (line.text.length + 2); | ||
return lineNumber + progress; | ||
} | ||
/** | ||
* Change the top-most visible line of `editor` to be at `line` | ||
*/ | ||
export function scrollEditorToLine( | ||
line: number, | ||
editor: vscode.TextEditor | ||
) { | ||
const sourceLine = Math.floor(line); | ||
const fraction = line - sourceLine; | ||
const text = editor.document.lineAt(sourceLine).text; | ||
const start = Math.floor(fraction * text.length); | ||
editor.revealRange( | ||
new vscode.Range(sourceLine, start, sourceLine + 1, 0), | ||
vscode.TextEditorRevealType.AtTop); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of exposing
previousMDTextEditor
, can you hide this behind a method that gets the line for a given uri?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previousMDTextEditor
changed to an internal map with public functions for access.