Skip to content

Fixes image name including # fail to render#84334

Merged
mjbvz merged 2 commits intomicrosoft:masterfrom
jeanp413:fix-84197
Nov 13, 2019
Merged

Fixes image name including # fail to render#84334
mjbvz merged 2 commits intomicrosoft:masterfrom
jeanp413:fix-84197

Conversation

@jeanp413
Copy link
Contributor

@jeanp413 jeanp413 commented Nov 9, 2019

This PR fixes #84197

mjbvz
mjbvz reviewed Nov 11, 2019
View reviewed changes
src/vs/workbench/contrib/webview/common/resourceLoader.ts Outdated
// Modern vscode-resources uris put the scheme of the requested resource as the authority
if (requestUri.authority) {
return URI.parse(requestUri.authority + ':' + requestUri.path);
return URI.parse(requestUri.authority + ':' + encodeURIComponent(requestUri.path).replace(/%2F/g, '/'));
Copy link
Collaborator

@mjbvz mjbvz Nov 11, 2019

Choose a reason for hiding this comment

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

Which operating system did you test this on. The change looks good but I'm not sure if it will work on windows

Copy link
Contributor Author

@jeanp413 jeanp413 Nov 11, 2019

Choose a reason for hiding this comment

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

Ubuntu 18.04

Copy link
Collaborator

@mjbvz mjbvz Nov 13, 2019

Choose a reason for hiding this comment

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

What happens for windows style paths though (ones that use \)?

Copy link
Contributor Author

@jeanp413 jeanp413 Nov 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't have a windows VM at hand to be able to build from sources and test it but looking at the uri.ts file it seems that \ characters are converted to / before constructing the uri object

vscode/src/vs/base/common/uri.ts

Lines 306 to 308 in ab147f8

if (isWindows) {
path = path.replace(/\\/g, _slash);
}

and converted back again with the fsPath getter

so the requestUri.path doesn't have any \ character

@mjbvz mjbvz added this to the November 2019 milestone Nov 11, 2019
@mjbvz mjbvz merged commit ef4d7e4 into microsoft:master Nov 13, 2019
@jeanp413 jeanp413 deleted the fix-84197 branch November 13, 2019 09:22
@jeanp413
Copy link
Contributor Author

jeanp413 commented Nov 18, 2019

@mjbvz I tested this today and seems that #84667 broke this again.
I tested both PRs separately and they fix both issues (#84197 and #83165) so you need to revert one of them.
The issue is that URI.parse is called twice here and here

  • Fix opening image with '%' in the filename #84667 fixes it by encoding the uri twice in the extension so a filename #.png will be encoded as "vscode-resource://file///home/jeanpierre/Desktop/vstest/%2523.png?version=1574091136161" (notice %23 is encoded as %2523)
  • This PR fixes just by encoding the path again before calling URI.parse the second time

@onequid onequid mentioned this pull request Nov 21, 2019
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mjbvz mjbvz mjbvz left review comments

Assignees

@mjbvz mjbvz

Labels

None yet

Projects

None yet

Milestone

November 2019

Development

Successfully merging this pull request may close these issues.

image name include # cause fail render

2 participants

@jeanp413 @mjbvz