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

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

Merged
merged 2 commits into from Nov 16, 2019
Merged

Conversation

@onequid
Copy link
Contributor

onequid commented Nov 13, 2019

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

This comment has been minimized.

Copy link

msftclas commented Nov 13, 2019

CLA assistant check
All CLA requirements met.

@onequid

This comment has been minimized.

Copy link
Contributor Author

onequid commented Nov 13, 2019

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

This comment has been minimized.

Copy link
Contributor

mjbvz commented Nov 14, 2019

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':

This comment has been minimized.

Copy link
@onequid

onequid Nov 14, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Contributor Author

onequid commented Nov 14, 2019

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 mjbvz added this to the November 2019 milestone Nov 16, 2019
@mjbvz mjbvz merged commit ba19fe0 into microsoft:master Nov 16, 2019
@mjbvz

This comment has been minimized.

Copy link
Contributor

mjbvz commented Nov 16, 2019

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

@onequid

This comment has been minimized.

Copy link
Contributor Author

onequid commented Nov 16, 2019

Glad to know that! My pleasure! Thanks!

@onequid onequid deleted the onequid:83165-cannot_open_file_with_percent_mark branch Nov 16, 2019
@mjbvz

This comment has been minimized.

Copy link
Contributor

mjbvz commented Nov 20, 2019

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

mjbvz added a commit that referenced this pull request Nov 20, 2019
Partially revert #84667

Also seems to fix #85190
@onequid

This comment has been minimized.

Copy link
Contributor Author

onequid commented Nov 21, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.