substance / texture

A visual editor for research.
MIT License
1k stars 85 forks source link

Improve 'Download File' in desktop app. #1132

Closed obuchtala closed 5 years ago

obuchtala commented 5 years ago

ATM this opens a link in the same window. This makes the app unusable afterwards. We should discuss if we need this feature for the desktop app at all, and if so, then we need an explicit mechanism opening a file dialog similar to 'Save as'.

obuchtala commented 5 years ago

IMO, we should implement the file-dialog mechanism. Even if we decided to disable the save file command in electron, we would probably want something for a future DAR browser.

obuchtala commented 5 years ago

@Integral what was the reason for disabling this in electron, in first place? https://github.com/substance/texture/blob/master/src/article/editor/DownloadSupplementaryFileTool.js#L12

Integral commented 5 years ago

@oliver---- Can I ask you to provide more details regarding application unusability after downloading supplementary file? Is there any error, if so what build did you used, in what OS? I see that file got downloaded in macOS High Sierra and the app is working afterwards. Still there is a big problem that you have no chance to understand that file got downloaded. True, I introduced this exception for electron since with new window after downloading you'll see a blank window:

screen shot 2019-02-03 at 14 52 44

I think it is rather annoying and confusing. I believe we should try to find some general approach for downloads in desktop app, let's discuss it.

obuchtala commented 5 years ago

Without opening a new tab, the main window gets blank. We must not leave the main location in the app. texture-download-in-electron

A new tab, at least leaves the main window intact.

obuchtala commented 5 years ago

Also, the user would expect to be asked for a download location.

Integral commented 5 years ago

@oliver---- thanks for gif! Can I ask you what branch did you use? Perhaps you can attach a dar file? Because I don't have this problem with mac os: I not even sure that new window will solve the problem for you. So as a first step I think we should try to understand what's going on. I also found a problem with attached image as supplementary file. It's not downloading, but opening: screen shot 2019-02-03 at 20 11 42

obuchtala commented 5 years ago

Yes, opening tab is not a solution, but doing in the same tab is worse.

I am on master. Also for me it's opening the file. The one from the capture was very large and it finished after gif ended.

As I have described in the issue description, we need something different: a file save dialog. Similar as our 'Save as'. And instead of opening the tab we need to read data from that url and pipe it through to file write.

Integral commented 5 years ago

So we are ok with current implementation for browser (except images)?

obuchtala commented 5 years ago

I guess yes. User could still use browser's save as for images.

obuchtala commented 5 years ago

Showing a file dialog, and then piping data like this: https://stackoverflow.com/questions/14552638/read-remote-file-with-node-js-http-get

Integral commented 5 years ago

@oliver---- I think we can force saving the image via download attribute of tag. I think that would be better rather than showing it... What do you think? I will try to implement save as in separate PR.

obuchtala commented 5 years ago

If that is possible, it's better. Would that work for electron, too?

Integral commented 5 years ago

I will investigate ;)

Integral commented 5 years ago

Re: download attribute. It will instructs browser and electron to download a URL instead of navigating to it, so the user will be prompted to save it as a local file, but only for same-origin URLs. In other words: it will not work for remote images.

obuchtala commented 5 years ago

In the browser it did download remote files with the target=_blank, though

Integral commented 5 years ago

yes, but not images

obuchtala commented 5 years ago

Ok, then let's collect a list of cases and options to summarise what we can do in which situation.

Integral commented 5 years ago

To my surprise I don't see a lot of options for image downloading in client, I only found this technique. Which is a bit too much in my taste and a bad sign for solving this problem. I mean if there is a move towards image download restriction maybe we should consider that and rather show image with some additional warning. So a list of cases is following:

  1. Browser version 1.1. Local image (working fine with new page and download attribute) 1.2. Remote image (going wild with canvas or show image or show image with warning [a div with 100% max-height and max-width and warning on top of it]) 1.3. Anything else (working fine with new page)
  2. App version I believe we can do anything through file dialog an piping GET result to a file.
obuchtala commented 5 years ago

Ok, thanks! Then let's do the following:

obuchtala commented 5 years ago

Maybe we will extend the dar-server protocol in future to be able to retrieve a download url from the server, then this can be proxied if needed. But we will do this when we know more about integration use-cases.

Integral commented 5 years ago

@oliver---- Is it correct for Browser use case: we are opening any file in new tab with @download (we just know that it will not work only for remote images and for now don't want to display warnings)?

obuchtala commented 5 years ago

I suggested to do in browser target=_blank for remote, because there is no better way.

Integral commented 5 years ago

Ok. Got it. Priority?

obuchtala commented 5 years ago

Rather sooner than later, because currently it is kind of broken.

Integral commented 5 years ago

Ok, I will try. Unfortunately it's not super trivial for app.

Integral commented 5 years ago

@oliver---- may I ask you to try #1134 in app? looks like electron have a special api for our case

Integral commented 5 years ago

For me it works exactly as we discussed, but curious about original case of yours. How it will look like?

obuchtala commented 5 years ago

It is working - almost :) It crashes on 'Cancel'.

Integral commented 5 years ago

@oliver---- done!

obuchtala commented 5 years ago

Done via #1134