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

Upload folders via D&D in the browser workbench #97347

Merged
merged 2 commits into from
May 13, 2020

Conversation

steven166
Copy link
Contributor

@steven166 steven166 commented May 9, 2020

This PR fixes #97592

Upload recursive folders via drag and drop in the file explorer window for the Browser workbench.

@msftclas
Copy link

msftclas commented May 9, 2020

CLA assistant check
All CLA requirements met.

@isidorn
Copy link
Contributor

isidorn commented May 11, 2020

@steven166 thanks for this pr, however did you test this change out? If yes for what cases did you test it?

@steven166
Copy link
Contributor Author

@isidorn, I tested it in combination with code-server. The scenario that I've tested was dragging a large folder with several subfolders and files from my computer (mac) into the file explorer of vscode for Chrome and Firefox.

@isidorn
Copy link
Contributor

isidorn commented May 11, 2020

Adding @bpasero for Web DnD

@isidorn isidorn requested review from isidorn and bpasero May 11, 2020 14:09
@bpasero
Copy link
Member

bpasero commented May 11, 2020

I am against a change that only works in Chrome, given the webkitGetAsEntry thing. If this cannot be implemented across browsers, then I suggest to start a web standards discussion on this and get it into the spec 👍

PS: this seem unrelated to #164

@steven166
Copy link
Contributor Author

@bpasero, it also works in Firefox. But not sure how it works in IE/Edge and if they should be supported as they are switching also to the webkit engine.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Indeed, this works in Firefox, Safari and even Edge, nice. Can we get a separate issue to talk about what you fix because the linked one is unrelated. What you solve is the folder upload for web and I think we have no issue for that yet.

@bpasero
Copy link
Member

bpasero commented May 12, 2020

One smaller issue that I found is that we seem to try to open the folder in the editor which shows up as error. Maybe we can ensure we only open a file and not a folder.

@steven166
Copy link
Contributor Author

I've created a new issue for it #97592.

Good idea to add a check to open only files. Will add it.

I noticed a performance issue with the upload mechanism. While uploading a lot of files or some small large files, you can't interact anymore with the backend and ping's start to fail. I think the cause of this is that the stream is fully occupied and there is no room left anymore.

I think this is not that great for the user experience. It is out of the scope of this pr, but maybe some kind of quality of service for the stream to the backend would be helpful here. What do you think?

@bpasero
Copy link
Member

bpasero commented May 12, 2020

Well I think the file service supports everything to write as stream, but the question is: do browsers support a streamed interface for the file contents to read from?

@steven166
Copy link
Contributor Author

Ah I see, the FileReader should be implemented slightly different to read in chunks.

@steven166
Copy link
Contributor Author

steven166 commented May 12, 2020

Added the FileReadableStream class which reads a file in chunks of 1kb and added a progress notification for the upload. I didn't hit the performance issue anymore after this change.

@bpasero
Copy link
Member

bpasero commented May 12, 2020

Thanks for the investment. Would it make sense to decouple the two things into separate PRs? I feel today already we are not using streams when you upload file(s) with the potential of blocking the tab, but given this is a relatively rare operation, I think we can keep it this way even for the folder upload for the moment.

And in a second PR, look into converting this into a stream based approach? I am also asking because my time is limited for being able to review all the changes and I think we can move forward with your previous non-stream-based solution and discuss the stream based approach independently. I also noticed you added a lot of progress notifications, but I wonder if that is a bit of overkill...

@steven166
Copy link
Contributor Author

Agree, I can understand that. I've reverted my branch for now.

@isidorn
Copy link
Contributor

isidorn commented May 13, 2020

@steven166 thanks a lot for the PR. Merging in. I will write a test plan item so we cover this in the endgame week.

@isidorn isidorn merged commit e9d4c97 into microsoft:master May 13, 2020
@isidorn isidorn added this to the May 2020 milestone May 13, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jun 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web File explorer d&d drop folder with nested files
4 participants