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 new Transfers menu to allow pause all and resume all torrents #1043

Merged
merged 3 commits into from
Oct 24, 2016

Conversation

karanjthakkar
Copy link
Contributor

Fixes #1027

@karanjthakkar
Copy link
Contributor Author

@feross Let me know what you think of this :)

if (torrentSummary.status === 'downloading') {
torrentSummary.status = 'paused'
ipcRenderer.send('wt-stop-torrenting', torrentSummary.infoHash)
sound.play('DISABLE')
Copy link
Member

Choose a reason for hiding this comment

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

Could sound.play be moved outside the forEach loop? This could get funky if there is a large number of torrents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I can do that.

@karanjthakkar
Copy link
Contributor Author

@Flet Done!

@karanjthakkar
Copy link
Contributor Author

@Flet Bump! Is there anything stopping this from getting merged? Would love to do it so that this gets merged :)

@@ -128,6 +128,26 @@ module.exports = class TorrentListController {
}
}

pauseAllTorrents () {
this.state.saved.torrents.forEach((torrentSummary) => {
if (torrentSummary.status === 'downloading') {
Copy link
Member

@Flet Flet Oct 24, 2016

Choose a reason for hiding this comment

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

I noticed that if a torrent is seeding it is not paused... Does it make sense to pause torrents that are seeding too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Flet Yes, I think it does. 🤔 I did not think about it. Thanks for bringing it up! Should I add it in the implementation? I can do it in a few minutes if you're around.

@Flet
Copy link
Member

Flet commented Oct 24, 2016

I think @feross and @dcposch are taking a little break from this repo :)

I think updating "pause all" to include "Seeding" torrents makes sense.

Happy to review and merge once this is done!

@karanjthakkar
Copy link
Contributor Author

@Flet Oh, I understand. I'll update the PR to include seeding torrents too.

@karanjthakkar
Copy link
Contributor Author

karanjthakkar commented Oct 24, 2016

I've added the condition for checking seeding torrents while pausing. However since the old torrent state wasn't saved, while resuming I have to set all the torrents to a downloading state. They're then automatically moved to a seeding state. Is that cool?

@Flet
Copy link
Member

Flet commented Oct 24, 2016

Yeah, I think its OK.
It the spirit of keeping things simple, its a straightforward way to do it without adding separate "pause" states (and more menu items).

We can always make it more articulate later if folks feel pain.

@Flet Flet merged commit ed0f05a into webtorrent:master Oct 24, 2016
@karanjthakkar karanjthakkar deleted the transfers-menu branch October 24, 2016 18:45
@feross
Copy link
Member

feross commented Jan 23, 2017

LGTM!

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
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.

3 participants