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

Add 'Upload Files...' menu entries #4717

Merged
merged 3 commits into from
Mar 29, 2019
Merged

Add 'Upload Files...' menu entries #4717

merged 3 commits into from
Mar 29, 2019

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Mar 26, 2019

fix #4088

TODO:

  • fix handling of global selection by the navigator: it does not clean it up on disposal
  • relocate & group download & upload in the main menu
  • relocate & group download & upload in the navigator context menu
  • reveal a folder in the navigator after upload
  • open a CQ for formidable - it's MIT and does not have 3rd party dependencies, we can wait till the CQ process for prod dependencies is clear
  • sign off a commit properly
  • uploading to the same folder twice should work

upload

@@ -57,29 +76,20 @@ export class FileDownloadCommandContribution implements CommandContribution {
return this.isDownloadEnabled(uris);
}

protected getUris(uri: Object | undefined): URI[] {
Copy link
Member Author

Choose a reason for hiding this comment

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

@kittaakos i've removed it, seems to be just left overs which were never used

@akosyakov akosyakov changed the title WIP Add 'Upload Files...' menu entries Add 'Upload Files...' menu entries Mar 27, 2019
@akosyakov
Copy link
Member Author

@jankeromnes @jgbradley1 Could you try it out?

@akosyakov
Copy link
Member Author

akosyakov commented Mar 27, 2019

@vzhukovskii There was a bug in the navigator and plugin tree views that the global selection was not cleaned properly when they are disposed. I've introduced TreeWidgetSelection to deal with that in TreeWidget. One can enable it via globalSelection props. Could you please have a look that it does not break menu items for tree view? I will do it as well.

@akosyakov akosyakov force-pushed the upload-files branch 2 times, most recently from 5c826ee to 37d07db Compare March 27, 2019 11:06
@vzhukovs
Copy link
Contributor

@akosyakov thanks! Will try to check it asap.

@vzhukovs
Copy link
Contributor

Looks like it doesn't break existed logic on plugin side.

@jgbradley1
Copy link
Contributor

jgbradley1 commented Mar 28, 2019

Nice. I like it! One minor UI issue. If a user has a directory open (i.e. by clicking on the drop down arrow) in the workspace and uploads files to it, they will not actually see the file structure updated with the new file names. You must first close the folder and reopen it. In the screenshot above, open the script directory and then upload files to it. I think you'll see what I am describing.

@vzhukovskii There was a bug in the navigator and plugin tree views that the global selection was not cleaned properly when they are disposed. I've introduced TreeWidgetSelection to deal with that in TreeWidget. One can enable it via globalSelection props. Could you please have a look that it does not break menu items for tree view? I will do it as well.

Is this the same bug you were talking about?

Solution: Make a call to refresh the file tree after the upload process is complete.

This issue also exist when you delete a file. You have to close the folder and open it back up again to see the changes occur.

@jgbradley1
Copy link
Contributor

jgbradley1 commented Mar 28, 2019

This is not a necessary feature right now, but we should think about some way to communicate upload progress on a status bar or on the blue bar at the bottom of the page.

@akosyakov
Copy link
Member Author

This is not a necessary feature right now, but we should think about some way to communicate upload progress on a status bar or on the blue bar at the bottom of the page.

It is on our list to allow blueish progress bar for different top-level widgets similar what you see in VS Code or Chrome while loading the page. It is even more important for cloud solution than for VS Code, since connection delay and issues is something which we need to deal with.

@akosyakov
Copy link
Member Author

If a user has a directory open (i.e. by clicking on the drop down arrow) in the workspace and uploads files to it, they will not actually see the file structure updated with the new file names. You must first close the folder and reopen it.

It is fs watching issue. I will have a look into it. A library which we use has troubles with symlinks and does not report some events for real paths :( Refreshing for each operation is work around, it should happen automatically based on fs events.

@akosyakov
Copy link
Member Author

akosyakov commented Mar 28, 2019

@jgbradley1 could you try again, i've added a fix for fs watching with symlinks, cc @elaihau

@AlexTugarev
Copy link
Contributor

Some random notes while testing this:

  • if possible we should add a filter for all pseudo files, e.g. .app on osx, it's not a file but a folder, which is not supported, and the effect is, that it doesn't bark, but blocking the next try to upload a file for quite some time.
  • maxFileSize exceeded is not reported
  • files of the same name are overridden silently
    • maybe it would makes to write to orig_filename.tmp, and mv to the then computed filename.

@akosyakov
Copy link
Member Author

akosyakov commented Mar 28, 2019

i think we can wait till someone opens bugs for pseudo files and silent override. What would be good default maxFileSize?

Copy link
Contributor

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

This works nicely! 🎉

@akosyakov akosyakov force-pushed the upload-files branch 4 times, most recently from 32b299f to 9f5cd55 Compare March 28, 2019 14:16
@akosyakov
Copy link
Member Author

@AlexTugarev I've added proper error handling and uploadMaxFileSize env variable, by default it is 2048MB. Could you check again?

@jgbradley1 i've also added upload progress reporting :)

@elaihau
Copy link
Contributor

elaihau commented Mar 28, 2019

Nice feature. i love it.
I tried uploading different versions of the same file to the same location, and found the newer version replaces the older - that's what i expected. would it be helpful if theia warns the user the older version would be overwritten ?

@akosyakov
Copy link
Member Author

akosyakov commented Mar 28, 2019

I tried uploading different versions of the same file to the same location, and found the newer version replaces the older - that's what i expected. would it be helpful if theia warns the user the older version would be overwritten ?

It is not easy to implement: this part of code cannot prompt to a user, since it does not go through web socket connection). We can only fail the whole request.

@jgbradley1
Copy link
Contributor

jgbradley1 commented Mar 28, 2019

@akosyakov This is great. +1 on the progress bar. Something is wrong with the max file size. It's off by an order of magnitude. I cannot upload any file > 2 MB and they all fail with the Payload too large message (tested on Gitpod). Files < 2 MB upload successfully. The math in the code looks correct however.

2048 * 1024 * 1024 (bytes) = 2 GB

@akosyakov
Copy link
Member Author

akosyakov commented Mar 29, 2019

@jgbradley1 good catch! thank you for checking

Updated: it turned out to be a proxy settings issue in Gitpod

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov
Copy link
Member Author

@AlexTugarev @jgbradley1 I've fixed and retested locally handling of exceeded payload. It should be fine now. Good to merge?

@akosyakov
Copy link
Member Author

merging, @AlexTugarev said that he is fine with changes in off-line conversation

@akosyakov akosyakov merged commit 9b6234e into master Mar 29, 2019
@akosyakov akosyakov deleted the upload-files branch March 29, 2019 10:11
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.

Add a File menu entry to upload files
5 participants