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: decode urlencoded spaces in asset path before opening #2279

Merged
merged 3 commits into from
Jan 29, 2022

Conversation

hikchoi
Copy link
Contributor

@hikchoi hikchoi commented Jan 26, 2022

fix: decode urlencoded spaces in asset path before opening

This PR:

  • fixes an issue with asset paths not opening correctly with the system's default app when there is a urlencoded space (%20) in the filename.

Manual testing

  • Since this is not straightforward to integ-test, I'm doing the following:
  1. open dendron.ref.assets in test-workspace
  2. open preview
  3. click on link to asset with space
  4. confirm opening in default app

Pull Request Checklist

If this is your first time submitting a pull request to Dendron, copy and paste the full Dendron Review Checklist into this request and check off each item as necessary.

This template contains the short checklist which is used by the Dendron core team.

Testing

Docs

  • [~] if your change reflects documentation changes, also submit a PR to dendron-site and mention the doc PR link in your current PR

Analytics

  • [~] if you are adding analytics related changes, make sure the Telemetry docs are updated

@hikchoi
Copy link
Contributor Author

hikchoi commented Jan 26, 2022

confirmed manual testing in

  • mac
  • windows
  • linux

@hikchoi
Copy link
Contributor Author

hikchoi commented Jan 26, 2022

related: #2228

@hikchoi
Copy link
Contributor Author

hikchoi commented Jan 26, 2022

madge reports no additional circ dependencies

@jonathanyeung jonathanyeung self-requested a review January 27, 2022 11:34
Copy link
Member

@kevinslin kevinslin left a comment

Choose a reason for hiding this comment

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

can we have a test for this?

@hikchoi
Copy link
Contributor Author

hikchoi commented Jan 27, 2022

can we have a test for this?

Do you have any suggestions on how to write an integ test for this?

The behavior itself is triggered from the webview, which the plugin-core does listens to. the end result we want to verify is that a system default app has been used to open an asset file.

I can test if the path that we get at the end would be properly decoded, but if there's anything else that I'm blanking on here let me know.

nevermind. We already have some existing tests. I'll add more to that

Copy link
Member

@kevinslin kevinslin left a comment

Choose a reason for hiding this comment

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

this looks good. can you rename ShowPreview.test.ts to PreviewLinkHandler.test.ts to make it more obvious for the next person?

@kevinslin kevinslin merged commit c60743d into master Jan 29, 2022
@kevinslin kevinslin deleted the fix/file-links-with-spaces branch January 29, 2022 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants