Skip to content

Fix opening image with '%' in the filename - #84667

Merged
Matt Bierner (mjbvz) merged 2 commits into
microsoft:masterfrom
onequid:83165-cannot_open_file_with_percent_mark
Nov 16, 2019
Merged

Fix opening image with '%' in the filename#84667
Matt Bierner (mjbvz) merged 2 commits into
microsoft:masterfrom
onequid:83165-cannot_open_file_with_percent_mark

Conversation

@onequid

@onequid onequid commented Nov 13, 2019

Copy link
Copy Markdown

This PR fixes #83165 opening image with '%' in the filename
Please let me know how to write tests for this if it is required as I don't have a clue.
I have no idea in what situation scheme would be as 'git' or 'data' neither, please give advice and I would try it out

@msftclas

Microsoft Contribution License Agreements (msftclas) commented Nov 13, 2019

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@onequid

onequid commented Nov 13, 2019

Copy link
Copy Markdown
Author

By the way, do you any conventions for encodeURI and decodeURI thing?
I am a bit concerned if there would be any other situations that '%' not handled propriately, as I saw functions calling decodeURIComponent() themselves and another function calling decodeURIComponent(), where the url is decoded twice in a row.

@mjbvz

Copy link
Copy Markdown
Contributor

I believe this was originally added to handle file names with spaces in the name. Can you please confirm images with names like a b.png still load after this change?

@@ -223,15 +223,15 @@ class Preview extends Disposable {
private getResourcePath(webviewEditor: vscode.WebviewPanel, resource: vscode.Uri, version: string) {
switch (resource.scheme) {
case 'data':

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

What user operation or situation would run codes in the case 'data'?
I guess that the case 'git' part would be fine. Not sure about the case 'data' section.

@onequid

onequid commented Nov 14, 2019

Copy link
Copy Markdown
Author

I see, I didn't think of the spaces.
And I just tried and glad to say that it worked good.
screenshot

I am still concerned about another case section.
Could you please share some more of your knowledge?
I add a review comment just in case I did not make it clear.

@mjbvz Matt Bierner (mjbvz) added this to the November 2019 milestone Nov 16, 2019
@mjbvz
Matt Bierner (mjbvz) merged commit ba19fe0 into microsoft:master Nov 16, 2019
@mjbvz

Copy link
Copy Markdown
Contributor

Thanks! This will be in the next insiders build and is scheduled to be shipped with VS Code 1.41

@onequid

onequid commented Nov 16, 2019

Copy link
Copy Markdown
Author

Glad to know that! My pleasure! Thanks!

@onequid
onequid deleted the 83165-cannot_open_file_with_percent_mark branch November 16, 2019 02:35
@mjbvz

Copy link
Copy Markdown
Contributor

This broke loading of images with spaces in the name. Given that these are more common than images with %, I'm reverting this change

Matt Bierner (mjbvz) pushed a commit that referenced this pull request Nov 20, 2019
Partially revert #84667

Also seems to fix #85190
@onequid

onequid commented Nov 21, 2019

Copy link
Copy Markdown
Author

I assume that both PR (this and #84334 ) would locate the same cause, and only one of them should be merged. It is more about which to merge.

Merging this PR solely won't break anything as far as I tested, in my defence.
Regards

@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An Error occured while loading the image

3 participants