Skip to content
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

Refresh Images In Markdown Preview On Change #114083

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,6 @@ export class RenderDocument implements Command {
) { }

public async execute(document: SkinnyTextDocument | string): Promise<string> {
return this.engine.render(document);
return (await (this.engine.render(document))).html;
}
}
52 changes: 48 additions & 4 deletions extensions/markdown-language-features/src/features/preview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ import { isMarkdownFile } from '../util/file';
import { normalizeResource, WebviewResourceProvider } from '../util/resources';
import { getVisibleLine, TopmostLineMonitor } from '../util/topmostLineMonitor';
import { MarkdownPreviewConfigurationManager } from './previewConfig';
import { MarkdownContentProvider } from './previewContentProvider';
import { MarkdownContentProvider, MarkdownContentProviderOutput } from './previewContentProvider';
import { MarkdownEngine } from '../markdownEngine';
import { urlToFileUri } from '../util/url';

const localize = nls.loadMessageBundle();

Expand Down Expand Up @@ -118,6 +119,20 @@ class MarkdownPreview extends Disposable implements WebviewResourceProvider {
private _disposed: boolean = false;
private imageInfo: { readonly id: string, readonly width: number, readonly height: number; }[] = [];

private currentCacheKeyOffset = 0;
/**
* Cache keys must not be reused during the entire lifetime of the preview.
* Even if images are removed from the document, their cache key must
* not be reset - the image could be added later again.
* In theory, images could be added, removed, edited and added again.
* In this implementation, images that are removed from the document aren't
* tracked anymore for performance reasons.
* If they are added again, they are considered unchanged.
* This behavior can be changed easily.
*/
private readonly _imageCacheKeysBySrc = new Map</* src: */ string, string>();
private readonly _fileWatchersBySrc = new Map</* src: */ string, vscode.FileSystemWatcher>();

constructor(
webview: vscode.WebviewPanel,
resource: vscode.Uri,
Expand Down Expand Up @@ -208,6 +223,9 @@ class MarkdownPreview extends Disposable implements WebviewResourceProvider {
super.dispose();
this._disposed = true;
clearTimeout(this.throttleTimer);
for (const entry of this._fileWatchersBySrc.values()) {
entry.dispose();
}
}

public get resource(): vscode.Uri {
Expand Down Expand Up @@ -303,7 +321,8 @@ class MarkdownPreview extends Disposable implements WebviewResourceProvider {
}

this.currentVersion = pendingVersion;
const content = await this._contentProvider.provideTextDocumentContent(document, this, this._previewConfigurations, this.line, this.state);
const content = await this._contentProvider.provideTextDocumentContent(document, this,
this._previewConfigurations, this.line, this._imageCacheKeysBySrc, this.state);

// Another call to `doUpdate` may have happened.
// Make sure we are still updating for the correct document
Expand Down Expand Up @@ -360,7 +379,7 @@ class MarkdownPreview extends Disposable implements WebviewResourceProvider {
this._webviewPanel.webview.html = this._contentProvider.provideFileNotFoundContent(this._resource);
}

private setContent(html: string): void {
private setContent(content: MarkdownContentProviderOutput): void {
if (this._disposed) {
return;
}
Expand All @@ -371,7 +390,32 @@ class MarkdownPreview extends Disposable implements WebviewResourceProvider {
this._webviewPanel.iconPath = this.iconPath;
this._webviewPanel.webview.options = this.getWebviewOptions();

this._webviewPanel.webview.html = html;
this._webviewPanel.webview.html = content.html;

const srcs = new Set(content.containingImages.map(img => img.src));

// Delete stale file watchers.
for (const [src, watcher] of [...this._fileWatchersBySrc]) {
if (!srcs.has(src)) {
watcher.dispose();
this._fileWatchersBySrc.delete(src);
}
}

// Create new file watchers.
const root = vscode.Uri.joinPath(this._resource, '../');
for (const src of srcs) {
const uri = urlToFileUri(src, root);
if (uri && !this._fileWatchersBySrc.has(src)) {
const watcher = vscode.workspace.createFileSystemWatcher(uri.fsPath);
watcher.onDidChange(() => {
this.currentCacheKeyOffset++;
this._imageCacheKeysBySrc.set(src, `${this.currentCacheKeyOffset}`);
this.refresh();
mjbvz marked this conversation as resolved.
Show resolved Hide resolved
});
this._fileWatchersBySrc.set(src, watcher);
}
}
}

private getWebviewOptions(): vscode.WebviewOptions {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ function escapeAttribute(value: string | vscode.Uri): string {
return value.toString().replace(/"/g, '&quot;');
}

export interface MarkdownContentProviderOutput {
html: string;
containingImages: { src: string }[];
}


export class MarkdownContentProvider {
constructor(
private readonly engine: MarkdownEngine,
Expand All @@ -48,13 +54,25 @@ export class MarkdownContentProvider {
private readonly logger: Logger
) { }

/**
*
* @param markdownDocument
hediet marked this conversation as resolved.
Show resolved Hide resolved
* @param resourceProvider
* @param previewConfigurations
* @param initialLine
* @param imageCacheKeyBySrc Each image is identified by its "src".
* Two images with the same src but different cache keys must not share the
* same cache entry.
* @param state
*/
public async provideTextDocumentContent(
markdownDocument: vscode.TextDocument,
resourceProvider: WebviewResourceProvider,
previewConfigurations: MarkdownPreviewConfigurationManager,
initialLine: number | undefined = undefined,
imageCacheKeyBySrc: ReadonlyMap</* src: */ string, string>,
state?: any
): Promise<string> {
): Promise<MarkdownContentProviderOutput> {
const sourceUri = markdownDocument.uri;
const config = previewConfigurations.loadAndCacheConfiguration(sourceUri);
const initialData = {
Expand All @@ -74,8 +92,8 @@ export class MarkdownContentProvider {
const nonce = new Date().getTime() + '' + new Date().getMilliseconds();
const csp = this.getCsp(resourceProvider, sourceUri, nonce);

const body = await this.engine.render(markdownDocument);
return `<!DOCTYPE html>
const body = await this.engine.render(markdownDocument, { imageCacheKeyBySrc });
const html = `<!DOCTYPE html>
<html style="${escapeAttribute(this.getSettingsOverrideStyles(config))}">
<head>
<meta http-equiv="Content-type" content="text/html;charset=UTF-8">
Expand All @@ -89,11 +107,15 @@ export class MarkdownContentProvider {
<base href="${resourceProvider.asWebviewUri(markdownDocument.uri)}">
</head>
<body class="vscode-body ${config.scrollBeyondLastLine ? 'scrollBeyondLastLine' : ''} ${config.wordWrap ? 'wordWrap' : ''} ${config.markEditorSelection ? 'showEditorSelection' : ''}">
${body}
${body.html}
<div class="code-line" data-line="${markdownDocument.lineCount}"></div>
${this.getScripts(resourceProvider, nonce)}
</body>
</html>`;
return {
html,
containingImages: body.containingImages,
};
}

public provideFileNotFoundContent(
Expand Down
54 changes: 49 additions & 5 deletions extensions/markdown-language-features/src/markdownEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { Slugifier } from './slugify';
import { SkinnyTextDocument } from './tableOfContentsProvider';
import { hash } from './util/hash';
import { isOfScheme, MarkdownFileExtensions, Schemes } from './util/links';
import { tryAppendQueryArgToUrl } from './util/url';

const UNICODE_NEWLINE_REGEX = /\u2028|\u2029/g;

Expand Down Expand Up @@ -54,6 +55,25 @@ class TokenCache {
}
}

export interface RenderOutput {
html: string;
containingImages: { src: string }[];
}

export interface RenderContext {
/**
* Each image is identified by its "src".
* Two images with the same src but different cache keys must not share the
* same cache entry.
*/
imageCacheKeyBySrc?: ReadonlyMap</* src: */ string, string>;
}

interface RenderEnv {
containingImages: { src: string }[];
imageCacheKeyBySrc: ReadonlyMap</* src: */ string, string>;
}

export class MarkdownEngine {
private md?: Promise<MarkdownIt>;

Expand Down Expand Up @@ -141,18 +161,28 @@ export class MarkdownEngine {
return engine.parse(text.replace(UNICODE_NEWLINE_REGEX, ''), {});
}

public async render(input: SkinnyTextDocument | string): Promise<string> {
public async render(input: SkinnyTextDocument | string, context: RenderContext = {}): Promise<RenderOutput> {
const config = this.getConfig(typeof input === 'string' ? undefined : input.uri);
const engine = await this.getEngine(config);

const tokens = typeof input === 'string'
? this.tokenizeString(input, engine)
: this.tokenizeDocument(input, config, engine);

return engine.renderer.render(tokens, {
const env: RenderEnv = {
containingImages: [],
imageCacheKeyBySrc: context.imageCacheKeyBySrc || new Map()
};

const html = engine.renderer.render(tokens, {
...(engine as any).options,
...config
}, {});
}, env);

return {
html,
containingImages: env.containingImages
};
}

public async parse(document: SkinnyTextDocument): Promise<Token[]> {
Expand Down Expand Up @@ -192,12 +222,26 @@ export class MarkdownEngine {

private addImageStabilizer(md: any): void {
const original = md.renderer.rules.image;
md.renderer.rules.image = (tokens: any, idx: number, options: any, env: any, self: any) => {
md.renderer.rules.image = (tokens: any, idx: number, options: any, env: RenderEnv, self: any) => {
const token = tokens[idx];
token.attrJoin('class', 'loading');

const src = token.attrGet('src');
// `src-origin` makes this operation idempotent.
// Idempotent operations with side effects can be applied on cached values
// without worrying about changing the cache.
const src = token.attrGet('src-origin') || token.attrGet('src');
if (src) {
env.containingImages.push({ src });
const cacheKey = env.imageCacheKeyBySrc.get(src);

// Don't change `src` if no cache key is set!
// The cache key might alter the server response when the
// image is hosted online.
if (cacheKey !== undefined) {
token.attrSet('src-origin', src);
token.attrSet('src', tryAppendQueryArgToUrl(src, 'cacheKey', cacheKey));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you are using the query string make sure we don't used cached image. I wonder if VS Code should be doing this automatically though by looking at etags or similar for the resource?

Have you debugged to see why the images are being cached in the first place? Are vscode-webview-resource request perhaps not setting the correct cache headers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very good idea. I changed the approach and got rid of the cache keys. Instead, responses to vscode-webview-resource requests have an etag and a cache-control header now. Also, if-none-match is implemented.

Basically it is a pull approach now - on every possible change, all resources with an etag are re-requested. However, responses are cached.
Before it was a push approach - only if an image actually changed, it got re-requested.

The advantages of this vs cache keys are:

  • More natural code
  • Does work for arbitrary webviews
  • A manual preview refresh will reload resources from virtual file systems if they have an etag.

The disadvantages are:

  • Does only refresh images if the webview is reloaded (I guess reloading the entire markdown preview webview is never going to be problematic, so it is fine)
  • Resources with etags are not cached anymore with this change. Repeated access to such resources will lead to repeated requests. Most of them will be answered with 304 (not modified) though. If computing the etag is slow, this is going to be problematic.

}

const imgHash = hash(src);
token.attrSet('id', `image-hash-${imgHash}`);
}
Expand Down
42 changes: 40 additions & 2 deletions extensions/markdown-language-features/src/test/engine.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,50 @@ suite('markdown.engine', () => {
test('Renders a document', async () => {
const doc = new InMemoryDocument(testFileName, input);
const engine = createNewMarkdownEngine();
assert.strictEqual(await engine.render(doc), output);
assert.strictEqual((await engine.render(doc)).html, output);
});

test('Renders a string', async () => {
const engine = createNewMarkdownEngine();
assert.strictEqual(await engine.render(input), output);
assert.strictEqual((await engine.render(input)).html, output);
});
});

suite('image-caching', () => {
const input = '![](img.png) [](no-img.png) ![](http://example.org/img.png) ![](img.png) ![](./img2.png)';

test('Extracts all images', async () => {
const engine = createNewMarkdownEngine();
assert.deepStrictEqual((await engine.render(input)), {
html: '<p data-line="0" class="code-line">'
+ '<img src="img.png" alt="" class="loading" id="image-hash--754511435"> '
mjbvz marked this conversation as resolved.
Show resolved Hide resolved
+ '<a href="no-img.png" data-href="no-img.png"></a> '
+ '<img src="http://example.org/img.png" alt="" class="loading" id="image-hash--1903814170"> '
+ '<img src="img.png" alt="" class="loading" id="image-hash--754511435"> '
+ '<img src="./img2.png" alt="" class="loading" id="image-hash-265238964">'
+ '</p>\n'
,
containingImages: [{ src: 'img.png' }, { src: 'http://example.org/img.png' }, { src: 'img.png' }, { src: './img2.png' }],
});
});

test('Cache-Keys are considered', async () => {
const engine = createNewMarkdownEngine();
const imageCacheKeyBySrc = new Map<string, string>();
imageCacheKeyBySrc.set('img.png', '1');
imageCacheKeyBySrc.set('./img2.png', '2');

assert.deepStrictEqual((await engine.render(input, { imageCacheKeyBySrc })), {
html: '<p data-line="0" class="code-line">'
+ '<img src="img.png?cacheKey=1" alt="" class="loading" src-origin="img.png" id="image-hash--754511435"> '
+ '<a href="no-img.png" data-href="no-img.png"></a> '
+ '<img src="http://example.org/img.png" alt="" class="loading" id="image-hash--1903814170"> '
+ '<img src="img.png?cacheKey=1" alt="" class="loading" src-origin="img.png" id="image-hash--754511435"> '
+ '<img src="./img2.png?cacheKey=2" alt="" class="loading" src-origin="./img2.png" id="image-hash-265238964">'
+ '</p>\n'
,
containingImages: [{ src: 'img.png' }, { src: 'http://example.org/img.png' }, { src: 'img.png' }, { src: './img2.png' }],
});
});
});
});
39 changes: 39 additions & 0 deletions extensions/markdown-language-features/src/util/url.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import * as vscode from 'vscode';
import { URL } from 'url';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import will fail in browsers. I think you can instead what I do here:

declare const URL: typeof import('url').URL;

This will let you use the globally defined URL object that ships in both browsers and node

Copy link
Member Author

@hediet hediet Jan 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I changed that. Are the tests in markdown-language-features run in both node and browser environments?
Edit: I don't think so, otherwise CI would have detected that this import will crash.

I used to think even when VS Code runs in the browser, extensions have to be run inside a node environment.


/**
* Tries to convert an url into a vscode uri and returns undefined if this is not possible.
* `url` can be absolute or relative.
*/
export function urlToFileUri(url: string, base: vscode.Uri): vscode.Uri | undefined {
try {
// `vscode.Uri.joinPath` cannot be used, since it understands
// `src` as path, not as relative url. This is problematic for query args.
const parsedUrl = new URL(url, base.toString());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've had lots of problems in the past around encoding/decoding of special characters in uris. Make sure to add tests for cases where the uri has percent encoded characters

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some tests with percent characters. I don't know the spec though and what pitfalls there are.

const uri = vscode.Uri.parse(parsedUrl.toString());
return uri.scheme === 'file' ? uri : undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may be on a virtual file system, so we probably should not restrict this to 'file' uris. Instead you can use isWritableFilesystem to see if a given scheme is known to VS Code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But createFileSystemWatcher seems to work on file paths only, doesn't it?
I generalized the function, but changed the consumer to this:

const uri = urlToUri(src, root);
if (uri && uri.scheme === 'file' && !this._fileWatchersBySrc.has(src)) {
    const watcher = vscode.workspace.createFileSystemWatcher(uri.fsPath);
    // ...
}

} catch (e) {
// Don't crash if `URL` cannot parse `src`.
return undefined;
}
}

/**
* Tries to append `?name=arg` or `&name=arg` to `url`.
* If `url` is malformed (and it cannot be decided whether to use `&` or `?`),
* `url` is returned as is.
*/
export function tryAppendQueryArgToUrl(url: string, name: string, arg: string): string {
try {
const parsedUrl = new URL(url, 'file://');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, we may be on a virtual file system so generally we can't assume that we are on a file://.

I think it's fine if you want to restrict the first version of the feature to file uris, but if that's the case, make sure the function names make this clear

Copy link
Member Author

@hediet hediet Jan 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is now obsolete.
Anyways, as long as the file system watcher does not support virtual file systems (i.e. accepts vscode.Uris instead of the file path based glob), the file:// protocol is the only one that can be supported afaik.

const joinChar = (parsedUrl.search === '') ? '?' : '&';
return `${url}${joinChar}${name}=${arg}`;
} catch (e) {
return url;
}
}