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 a saveAs option to prompt a dialog instead of downloading immediately #4

Merged
merged 2 commits into from
Jul 21, 2016

Conversation

jwheare
Copy link
Contributor

@jwheare jwheare commented Jul 20, 2016

Added to allow working around #3, and also nice to have the option.

@sindresorhus
Copy link
Owner

Why not just do a proper workaround and append a number to the file name? That's pretty easy. As a user I don't want to be bothered with a save dialog. I just want to have the file saved.

@jwheare
Copy link
Contributor Author

jwheare commented Jul 20, 2016

Well I'd want both really. Sometimes I want to save the file to a specific location. This is in fact what Chrome has in the context menu. There's also the shortcut to hold down option to download immediately (which is what I use when I don't want to be bothered).

Having electron support the incrementing number is the best solution really. Just appending a number can get ugly if it happens more than once, you'd want an intelligently incrementing number really.

Besides, more work than I want to put into this now.

@jwheare
Copy link
Contributor Author

jwheare commented Jul 20, 2016

Travis failure looks unrelated to my changes. Possibly due to an xo update.

@sindresorhus
Copy link
Owner

Just appending a number can get ugly if it happens more than once, you'd want an intelligently incrementing number really.

With the Electron fix, I agree, but as a quick workaround, you can just append a number each time. I find it pretty edge-casy to have many identically named files in Downloads anyways.


Show a "Save As…" dialog instead of downloading immediately.

Useful if you're concerned about overriding existing files. See [issue #3](https://github.com/sindresorhus/electron-dl/issues/3)
Copy link
Owner

Choose a reason for hiding this comment

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

I agree this can be a useful feature, but I don't like it as a workaround. I'd rather add it for the right reasons.

Can you remove this and I'll add a proper workaround after this is merged.

@sindresorhus sindresorhus merged commit 936d006 into sindresorhus:master Jul 21, 2016
@sindresorhus
Copy link
Owner

👌

@sindresorhus
Copy link
Owner

sindresorhus commented Jul 21, 2016

I'll work on a fix to the overwriting problem tonight.

@jwheare jwheare deleted the saveas branch July 21, 2016 16:49
@zoubin
Copy link

zoubin commented Jul 23, 2016

@jwheare Thanks for adding this feature. It's really helpful.

@sindresorhus
If the saveAs option is turned on, app.dock.downloadFinished should not be called with the old file path, which is inside the Download directory, because the new file path may lie outside.

Is it possible to get the new file path synchronously in the renderer process?
Right now I have to run the code in the main process to synchronously get the save file path and call item.setSavePath(filePath) in the listener callback.

@sindresorhus
Copy link
Owner

If the saveAs option is turned on, app.dock.downloadFinished should not be called with the old file path, which is inside the Download directory, because the new file path may lie outside.

That's why Electron needs a getSavePath() method: electron/electron#6551 (comment)

Is it possible to get the new file path synchronously in the renderer process?

Get it from where? I'm not sure how this question is relevant to this module.

@zoubin
Copy link

zoubin commented Jul 23, 2016

Is it possible to get the new file path synchronously in the renderer process?

I was going to use this module in the renderer process, with the saveAs option on.
So I need to get the save path and pass it to app.dock.downloadFinished(newFilePath) in the will-download callback. But I failed to find a way to do that.

The getSavepath() is just what I'm looking for.
Thanks for your response and let me know that!

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.

3 participants