The Wayback Machine - https://web.archive.org/web/20230307184955/https://github.com/microsoft/vscode/pull/116649
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

Handle normalized absolute file paths on markdown preview link click #116649

Merged
merged 2 commits into from Feb 19, 2021
Merged

Handle normalized absolute file paths on markdown preview link click #116649

merged 2 commits into from Feb 19, 2021

Conversation

habibkarim
Copy link
Contributor

This PR fixes #115812

@@ -439,6 +439,9 @@ class MarkdownPreview extends Disposable implements WebviewResourceProvider {
if (hrefPath[0] !== '/') {
// Fix #93691, use this.resource.fsPath instead of this.resource.path
hrefPath = path.join(path.dirname(this.resource.fsPath), hrefPath);
} else {
// Handle any normalized file paths
hrefPath = hrefPath.replace('file///', '');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After investigation, I noticed that this issue was caused inadvertently by https://github.com/microsoft/vscode/pull/114553/files where the links were normalised to display images. This fix handles both absolute links and file:/// links

Copy link
Contributor

Choose a reason for hiding this comment

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

On windows, I think the file path will look like file://c/path/to/file. Try parsing the file path using vscode.Uri.parse and then getting the .fsPath from it instead. I think that should be safer but haven't tested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I've made the change The latest update now handles file:/ links as well. I've tested this change both on Windows and Mac for multiple paths.

Windows:
Windows

Mac:
Mac

@joaomoreno joaomoreno changed the base branch from master to main February 15, 2021 08:51
@@ -439,6 +439,9 @@ class MarkdownPreview extends Disposable implements WebviewResourceProvider {
if (hrefPath[0] !== '/') {
// Fix #93691, use this.resource.fsPath instead of this.resource.path
hrefPath = path.join(path.dirname(this.resource.fsPath), hrefPath);
} else {
// Handle any normalized file paths
hrefPath = hrefPath.replace('file///', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

On windows, I think the file path will look like file://c/path/to/file. Try parsing the file path using vscode.Uri.parse and then getting the .fsPath from it instead. I think that should be safer but haven't tested

@mjbvz mjbvz merged commit 9f08368 into microsoft:main Feb 19, 2021
1 check passed
@mjbvz
Copy link
Contributor

mjbvz commented Feb 19, 2021

Thanks! Will be in the next insiders build and is scheduled for VS Code 1.54

@mjbvz mjbvz added this to the February 2021 milestone Feb 19, 2021
@habibkarim habibkarim deleted the habibkarim/115812_markdown_links_no_longer_work branch February 19, 2021 17:33
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
2 participants