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

Playback priority #840

Merged
merged 9 commits into from
Mar 15, 2017
Merged

Playback priority #840

merged 9 commits into from
Mar 15, 2017

Conversation

codealchemist
Copy link
Contributor

Added highest playback priority feature; added highest playback priority checkbox on preferences.
Pauses all active torrents when playback starts, giving all bandwidth to the streaming torrent.
Restores paused torrent when playback stops.
This is the easy way to solve #362.

@noamokman
Copy link
Contributor

I think we need to resume downloads when the torrent you are watching finished downloading.
It looks like it only resumes after you close the player, am I right?

@codealchemist
Copy link
Contributor Author

Good catch @noamokman!
Yeah, on this PR the paused downloads resume only after the player stops.
It will be better if it also resumes when the streaming torrent finishes downloading.
I'll look into it, thanks!

@codealchemist
Copy link
Contributor Author

@noamokman, now it resumes on wt-done, which seems to be working as expected.

@noamokman
Copy link
Contributor

Awesome! Great work 😄

@codealchemist
Copy link
Contributor Author

Thanks :)

@codealchemist
Copy link
Contributor Author

@feross if this is still needed I can spend some time fixing conflicts.
Thx.

@Nelrann
Copy link

Nelrann commented Mar 13, 2017

Exactly what I want
+1

@codealchemist nice work

@codealchemist
Copy link
Contributor Author

Thanks @Toinouu!
Still waiting for team approval before fixing conflicts and merge.
:)

@dcposch
Copy link
Contributor

dcposch commented Mar 13, 2017

@codealchemist Sorry we missed this. Reviewing today

@codealchemist
Copy link
Contributor Author

No worries @dcposch :)
Thanks for reviewing it!

Copy link
Contributor

@dcposch dcposch left a comment

Choose a reason for hiding this comment

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

This is a nice feature, thanks for making it!

I found one thing that looks like a bug and others that could be simplified.

Feel free to merge once you're confident that it's fixed.


// if torrent was paused add it to paused torrents collection
// we will use this collection to resume downloading when playback stops
if (wasPaused) this.state.saved.pausedTorrents.push(torrent.infoHash)
Copy link
Contributor

Choose a reason for hiding this comment

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

After this, the pausedTorrents array will contain all torrents which were paused before.

However, below, we use that same array to resume torrents. We want to resume the ones that were running before, right?

}

resumePausedTorrents () {
this.state.saved.pausedTorrents.map((infoHash) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, I'd consider renaming pausedTorrents to torrentsToResume.

There are two kinds of paused torrents:

  • Torrents which the user paused manually, which we should not resume
  • Torrents which were paused automatically as part of playback priority

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll use torrentsToResume, which is a lot clearer.
Thanks!

if (playSound) sound.play('ENABLE')
}

pauseTorrent (torrentSummary, playSound, filter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd definitely put a short comment here explaining what filter does.

label={'Highest Playback Priority'}
onCheck={this.handleHighestPlaybackPriorityChange}
/>
<p>Pauses all active torrents to allow playback to use all of the available bandwidth.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -103,18 +103,73 @@ module.exports = class TorrentListController {
}
}

pauseAll ({filter, excluded}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about making this

prioritizeTorrent (infohash) {

...the filter arg is always just active torrents. Don't know if we need the ability to pause torrents by arbitrary JSON filter. The excluded arg is always an array containing a single infohash: the torrent we're playing.

@codealchemist
Copy link
Contributor Author

Thanks a lot for your review @dcposch!
I'll work on it.
Cheers!

Copy link
Contributor Author

@codealchemist codealchemist left a comment

Choose a reason for hiding this comment

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

Almost there!
I'd like to do more testing before saying its OK to be merged.
Any help is really welcome :)

}

resumePausedTorrents () {
this.state.saved.pausedTorrents.map((infoHash) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll use torrentsToResume, which is a lot clearer.
Thanks!

@codealchemist codealchemist merged commit c2abb50 into master Mar 15, 2017
@codealchemist
Copy link
Contributor Author

Tests passing.
Manual test looks good.

@feross
Copy link
Member

feross commented Mar 15, 2017

🎉 🎉 🎉 🎉

@codealchemist codealchemist deleted the playback-priority branch March 21, 2017 01:09
@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.

5 participants