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: drop multiple files on chrome #1049

Merged
merged 7 commits into from
May 30, 2019
Merged

fix: drop multiple files on chrome #1049

merged 7 commits into from
May 30, 2019

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented May 28, 2019

Fixes #1045 by using my fork of datatransfer-files-promise. I didn't publish it because I was hoping the original creator would accept my PR, although @grabantot doesn't seem very active on GitHub.

License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias requested a review from olizilla May 28, 2019 08:58
@hacdias
Copy link
Member Author

hacdias commented May 28, 2019

@olizilla why I haven't done the tests for this: #1047 (comment)

@olizilla
Copy link
Member

@hacdias 👍 you could publish it under your npm namespace like @hacdias/datatransfer-files-promise in the meantime.

@olizilla
Copy link
Member

So it'd be rad to add tests for the datatransfer-files-promise...it's a function that takes an array-ish DataTransferItemList, where each entry may be a single file or a tree of files, and returns a flat array of objects with a filePath property, that encodes the path in the tree where it came from.

We'd need to do some mocking... each item in the input array needs a .webkit​GetAsEntry() function that returns an entry with either .isFile or .isDirectory set to true and and entry.name. Where we set .isFile to true, we expect to just see filePath in the output object set to entry.name. Where isDirectroy is true, we need to provide an entry.createReader function that returns an object with a .getEntries function that returns an array of entries... from which we can mock out a tree structure of entries.

It's a bit of work, but it'd be really nice.

@olizilla
Copy link
Member

For example we should test what happens when a brower use getAsEntry instead of webkitGetAsEntry as per

This function is implemented as webkitGetAsEntry() in non-WebKit browsers including Firefox at this time; it may be renamed to simply getAsEntry() in the future, so you should code defensively, looking for both.

https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItem/webkitGetAsEntry

@hacdias
Copy link
Member Author

hacdias commented May 28, 2019

@olizilla yeah, thanks. I'm aware of the structure, I've already started them 😃

hacdias added 2 commits May 29, 2019 07:59
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented May 29, 2019

@olizilla just released it as a package. The tests will go on #1047 here.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented May 29, 2019

@olizilla I'm hitting an infinite loop on the directory tests and I don't know why. I know the loop is happening on the module (datatransfer-files-promise, both with mine and the original version) but it must be something in the structure I'm creating that's causing it.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented May 29, 2019

@olizilla found the issue. Ready for review 😃

Copy link
Member

@olizilla olizilla left a comment

Choose a reason for hiding this comment

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

Good stuff! Some minor tweaks to do.

More significantly, now that this code can handle the case where the user drops mutliple directories, i notice that I can't actually drop multiple directories in practice due to an error around the logic of counting how many directories we expect go-ipfs will make... can you take a look...

https://github.com/ipfs-shipyard/ipfs-webui/blob/master/src/lib/count-dirs.test.js#L32-L42

src/lib/dnd-backend.test.js Outdated Show resolved Hide resolved
src/lib/dnd-backend.test.js Outdated Show resolved Hide resolved
hacdias added 2 commits May 29, 2019 16:47
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented May 29, 2019

@olizilla just updated the tests. One question about https://github.com/ipfs-shipyard/ipfs-webui/blob/master/src/lib/count-dirs.test.js#L32-L42. Why is it testing against 8? Is that the wrong part?

@olizilla
Copy link
Member

@hacdias it assumes ipfs will make a dir for the root /, perhaps that assumption should change.

@olizilla
Copy link
Member

@hacdias does that make sense?

@hacdias
Copy link
Member Author

hacdias commented May 30, 2019

@olizilla yes, it does. Could we merge this PR and I'll take a look at count-dirs on another one?

@olizilla olizilla merged commit 5678fa1 into master May 30, 2019
@olizilla olizilla deleted the fix/drop-files branch May 30, 2019 11:30
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.

Dropping multiple files only adds one on Chrome
2 participants