Fix #3397 by hiiwave · Pull Request #3398 · BoostIO/BoostNote-Legacy

hiiwave · 2019-12-12T18:51:39Z

Description

This is trying to fix #3397.

(Update: Done)
~~This is working in progress. Since I have little experience building electron app, I haven't tried if this solution really works.~~

~~I can try to build it and confirm later.~~

Issue fixed

#3397

Type of changes

⚫ Bug fix (Change that fixed an issue)
⚪ Breaking change (Change that can cause existing functionality to change)
⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
⚪ Feature (Change that adds new functionality)
⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

⚫ My code follows the project code style
⚪ I have written test for my code and it has been tested -> I think it's untestable
⚫ All existing tests have been passed
⚫ I have attached a screenshot/video to visualize my change if possible -> Please see Cannot open attachment on storage with path containing non-English letter #3397 (comment)

hiiwave · 2019-12-16T16:09:02Z

(Update: Solved)
~~(Asking for help on project building)~~

Hi,

I tried to run the tests for this project (before editing any code) on Windows but failed. As I am not familiar to npm, can anyone advises what happened and how I could fix this here?
The steps to reproduce the problem are as follows:

git clone https://github.com/BoostIO/Boostnote.git
cd Boostnote
npm i
npm run test

It then shows the following log / error:

> boost@0.13.0 test C:\Users\hiiwa_000\resources\Boostnote
> npm run ava && npm run jest


> boost@0.13.0 ava C:\Users\hiiwa_000\resources\Boostnote
> cross-env NODE_ENV=test ava --serial

C:\Users\hiiwa_000\resources\Boostnote\tests\helpers\setup-browser-env.js:29
window.localStorage = {
                    ^

TypeError: Cannot set property localStorage of #<Window> which has only a getter
    at Object.<anonymous> (C:/Users/hiiwa_000/resources/Boostnote/tests/helpers/setup-browser-env.js:22:1)
    at Module._compile (module.js:652:30)
    at loader (C:\Users\hiiwa_000\resources\Boostnote\node_modules\babel-register\lib\node.js:144:5)
    at Object.require.extensions.(anonymous function) [as .js] (C:\Users\hiiwa_000\resources\Boostnote\node_modules\babel-register\lib\node.js:154:7)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at forEach.x (C:\Users\hiiwa_000\resources\Boostnote\node_modules\ava\lib\test-worker.js:34:3)

...

  32 exceptions

  × tests\dataApi\copyFile-test.js exited with a non-zero exit code: 1

...

All the tests failed with the exactly same reason.

I also tried npm run compile before npm run test, but it still does not work. If I run npm run start or npm run dev, I can get electron open, but the app freezes with the spinner spinning forever.

I guessed it's a basic setup issue but I have no idea how to go on. Appreciate for any help.

Flexo013 · 2019-12-16T20:49:49Z

I think you might need to use yarn, but I'm not sure about that. @Rokt33r do you know what can help?

ZeroX-DG · 2019-12-17T02:03:02Z

You can always check out the job log in the travis build to find out what command was executed for running test:
https://travis-ci.org/BoostIO/Boostnote/builds/624286760

hiiwave · 2019-12-17T14:47:33Z

Thanks for helping. I have finished this PR and checked that it fixed #3397 under Windows. Awaiting review.

arcturus140 · 2019-12-18T23:08:11Z

browser/components/MarkdownPreview.js

+
+  openExternal (href) {
+    try {
+      const success = shell.openExternal(href) || shell.openExternal(decodeURI(href))


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

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.

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

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)

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

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

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.

ZeroX-DG · 2020-02-01T01:19:43Z

@Rokt33r can you confirm this work on Windows as well?

Rokt33r · 2020-02-03T10:58:40Z

Will do.

Rokt33r · 2020-02-03T11:33:23Z

Confirmed it works. @hiiwave Thanks for the contribution!

WIP: Fix BoostIO#3397

3c1448e

Flexo013 added the awaiting review ❇️ Pull request is awaiting a review. label Dec 14, 2019

fixup! WIP: Fix BoostIO#3397

31227a5

hiiwave changed the title ~~WIP: Fix #3397~~ Fix #3397 Dec 17, 2019

fix: catch potential URIError threw from decodeURI

5c0c820

hiiwave mentioned this pull request Dec 18, 2019

Cannot open attachment on storage with path containing non-English letter #3397

Closed

arcturus140 reviewed Dec 18, 2019

View reviewed changes

ZeroX-DG added needs extra review 🔎 Pull request requires review from an additional reviewer. and removed awaiting review ❇️ Pull request is awaiting a review. labels Feb 1, 2020

ZeroX-DG requested a review from Rokt33r February 1, 2020 01:20

Rokt33r removed the needs extra review 🔎 Pull request requires review from an additional reviewer. label Feb 3, 2020

Rokt33r added this to the v0.15.0 milestone Feb 3, 2020

Rokt33r merged commit 051ce9e into BoostIO:master Feb 3, 2020

Uh oh!

Conversation

hiiwave commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue fixed

Type of changes

Checklist:

Uh oh!

hiiwave commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Flexo013 commented Dec 16, 2019

Uh oh!

ZeroX-DG commented Dec 17, 2019

Uh oh!

hiiwave commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arcturus140 Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiiwave Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

arcturus140 Dec 22, 2019

Choose a reason for hiding this comment

Uh oh!

hiiwave Dec 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcturus140 Dec 22, 2019

Choose a reason for hiding this comment

Uh oh!

arcturus140 Dec 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiiwave Dec 28, 2019

Choose a reason for hiding this comment

Uh oh!

ZeroX-DG commented Feb 1, 2020

Uh oh!

Rokt33r commented Feb 3, 2020

Uh oh!

Rokt33r commented Feb 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hiiwave commented Dec 12, 2019 •

edited

Loading

hiiwave commented Dec 16, 2019 •

edited

Loading

hiiwave commented Dec 17, 2019 •

edited

Loading

arcturus140 Dec 18, 2019 •

edited

Loading

hiiwave Dec 22, 2019 •

edited

Loading

arcturus140 Dec 22, 2019 •

edited

Loading