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

Check for missing download path #776

Merged
merged 3 commits into from
Aug 14, 2016
Merged

Check for missing download path #776

merged 3 commits into from
Aug 14, 2016

Conversation

dcposch
Copy link
Contributor

@dcposch dcposch commented Aug 13, 2016

Also handle the case where the user cancels out of Save Torrent File As

Fixes #646.

@@ -274,6 +274,7 @@ function saveTorrentFileAs (torrentSummary) {
]
}
electron.remote.dialog.showSaveDialog(electron.remote.getCurrentWindow(), opts, function (savePath) {
if (!savePath) return // They clicked Cancel
Copy link
Member

Choose a reason for hiding this comment

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

good catch!

@feross
Copy link
Member

feross commented Aug 13, 2016

This is good. One concern though: this only shows the user a message when the download folder is gone, but it doesn't prevent the active torrents from being added to the torrent engine, right? That means that webtorrent will make new directories, right? That's not what we want.

We should probably wait for the check to confirm the directory exists before adding to the torrent engine.

But, come to think of it, there's a bigger issue.

The default download path in Preferences is not necessarily the same as the torrents. The preference could have been changed to an external drive after several torrents were already started in ~/Downloads. In that case, the torrents that live in ~/Downloads should show up in the list and be usable by the user.

What do you think about changing this PR fs.stat the torrentSummary.path before calling webtorrent.add()? Torrents whose locations do not exist anymore could still show up in the list, but with warning text in their status line (below the name). This is what Transmission does.

As for the warning about the default download path being missing, I think that can stay. But I don't think it shouldn't prevent rendering of the entire torrent list when it's missing. What about just showing the warning at the top of the list?

@dcposch
Copy link
Contributor Author

dcposch commented Aug 14, 2016

Ya, I just wanted to do the easy part first.

I can a per-torrent check similar to the one Transmission has.

@dcposch
Copy link
Contributor Author

dcposch commented Aug 14, 2016

gifff

@dcposch
Copy link
Contributor Author

dcposch commented Aug 14, 2016

@feross fixed. New error message

screen shot 2016-08-13 at 8 37 56 pm

@dcposch dcposch force-pushed the dc/missing-path branch 4 times, most recently from 26cdac8 to 85f6b6e Compare August 14, 2016 04:50
@feross feross merged commit d57bfb8 into master Aug 14, 2016
@feross feross deleted the dc/missing-path branch August 14, 2016 23:10
@feross
Copy link
Member

feross commented Aug 14, 2016

LGTM -- we can work on improving the design of this and honing the copy in a future PR.

@feross feross mentioned this pull request Aug 14, 2016
@dcposch
Copy link
Contributor Author

dcposch commented Aug 15, 2016

screen shot 2016-08-13 at 9 22 17 pm
screen shot 2016-08-13 at 8 55 43 pm

@feross
Copy link
Member

feross commented Aug 20, 2016

Released in WebTorrent Desktop v0.11!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
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.

2 participants