Skip to content

Fix #3397 #3398

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.

Sign up for GitHub

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

Merged
Rokt33r merged 3 commits into from
Feb 3, 2020
Merged

Fix #3397 #3398

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3c1448e
WIP: Fix #3397
hiiwave Dec 12, 2019
31227a5
fixup! WIP: Fix #3397
hiiwave Dec 17, 2019
5c0c820
fix: catch potential URIError threw from decodeURI
hiiwave Dec 17, 2019
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
12 changes: 11 additions & 1 deletion browser/components/MarkdownPreview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,7 +1109,17 @@ export default class MarkdownPreview extends React.Component {
}

// other case
shell.openExternal(href)
this.openExternal(href)
}

openExternal (href) {
try {
const success = shell.openExternal(href) || shell.openExternal(decodeURI(href))
Copy link
Contributor

@arcturus140 arcturus140 Dec 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

the Link should always be encoded on creation and always decoded for onclick. Otherwise it could cause problems with edge cases when a non-encoded file looks like encoded. For example, if you have two files in the same directory:

  • %E6%B8%AC%E8%A9%A6.txt
  • %25E6%25B8%25AC%25E8%25A9%25A6.txt

Copy link
Contributor Author

@hiiwave hiiwave Dec 19, 2019

Choose a reason for hiding this comment

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

As it seems the original behavior was fine under mac os, and the issue seems to be Windows-only, I made it conservative that it prioritized original behavior. If you can confirm that "always decoding" works fine under each os, I'm happy to change to that case.

Copy link
Contributor

@arcturus140 arcturus140 Dec 22, 2019

Choose a reason for hiding this comment

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

@hiiwave would you consider testing it under Windows with always decoding? I am currently testing it on Linux.

Copy link
Contributor Author

@hiiwave hiiwave Dec 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Always decoding behaves exactly as this implementation under Windows, because shell.openExternal(href) does not work thus it will always do shell.openExternal(decodeURI(href)

Copy link
Contributor

@arcturus140 arcturus140 Dec 22, 2019

Choose a reason for hiding this comment

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

I can confirm that "always decoding" works fine under Linux.

Copy link
Contributor

@arcturus140 arcturus140 Dec 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@hiiwave ok, I was assuming edge cases on Windows that make shell.openExternal(href) succeed. If it doesn't then it is ok.

If you create a storage location in the folder %20 both raw and decoded will fail because it must be encoded:

shell.openExternal(encodeURI(href)

works in this case. I don't know where href is stored but it must be encoded on creation.

Copy link
Contributor Author

@hiiwave hiiwave Dec 28, 2019

Choose a reason for hiding this comment

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

As #3397 (comment):

IMHO, users should not create a folder named %20 in any case. I don't think anyone will ever do it as well.

if (!success) console.error('failed to open url ' + href)
} catch (e) {
// URI Error threw from decodeURI
console.error(e)
}
}

render () {
Expand Down