Upload folders via D&D in the browser workbench by steven166 · Pull Request #97347 · microsoft/vscode
This PR fixes #97592
Upload recursive folders via drag and drop in the file explorer window for the Browser workbench.
@steven166 thanks for this pr, however did you test this change out? If yes for what cases did you test it?
@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.
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
@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.
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.
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.
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?
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?
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.
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 thanks a lot for the PR. Merging in. I will write a test plan item so we cover this in the endgame week.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters