Add playlists feature #871

Goldob · 2016-09-01T18:46:10Z

Open multi-file torrents as playlists. Basically #642 after rebase.
Solves #123 and #839.

* replaces `pickFileToPlay` feature * when reopening player, restores the most recently viewed file

dcposch · 2016-09-01T21:48:39Z

src/renderer/lib/cast.js

@@ -96,7 +96,7 @@ function chromecastPlayer () {

  function open () {
    var torrentSummary = state.saved.torrents.find((x) => x.infoHash === state.playing.infoHash)
-    ret.device.play(state.server.networkURL, {
+    ret.device.play(state.server.networkURL + '/' + state.playing.fileIndex, {


I'd put this into its own function to reduce code duplication. (Notice the two lines that look just like this one further down)

dcposch · 2016-09-01T22:04:03Z

@Goldob thanks for doing this! Looks great

I think it can be simplified a bit

There's a bit of code duplication. Nothing serious
I don't know if we need state.playlist. The state object is already pretty enormous, and state.playing already contains the current infohash + file index we're playing. I think that's enough state to implement skip-forward, skip-back, and automatically playing the next file after one finishes.
I don't know if we want to add Repeat and Shuffle.

Here's why:

Autoplay is great for listening to an album or audiobook, but Shuffle inside a single torrent isn't that useful, right? Usually people want to shuffle across a whole collection.
We want to keep the app minimalist
We want users (and reviewers) to think of us as a great torrent app, not as an OK media library. Adding a Shuffle and Repeat button starts to feel like ITunes / Media Player / etc.

The new UI (menu buttons plus player icons for skipping forward and back) looks great to me.

dcposch · 2016-09-01T22:05:30Z

Tempted to merge this now and make a follow up PR to avoid conflicts with #863

CC @feross @mathiasvr

dcposch · 2016-09-01T22:06:43Z

src/renderer/lib/playlist.js

+  this._shuffled = false
+}
+
+// =============================================================================


I wouldn't add ascii banners like this. We don't have them anywhere else in the code

mathiasvr · 2016-09-01T22:22:35Z

Just tested it, and I get this error when it auto skips to next track:

undefined:1 Uncaught (in promise) DOMException: The play() request was interrupted by a call to pause().

This is probably a Chrome issue as discussed in #599. Not sure if it's serious though.

mathiasvr · 2016-09-01T22:29:01Z

src/renderer/lib/playlist.js

+    .filter((object) => TorrentPlayer.isPlayable(object.file))
+    .sort(function (a, b) {
+      if (a.file.name < b.file.name) return -1
+      if (b.file.name < a.file.name) return 1


Do we want to sort the files by name? I think preserving the original order makes sense.

Files in torrent list are displayed alpabetically, so I think it's reasonable to keep that order. Also, when we stick to file indices, the sequence appears totally random.

Do you have an example? All torrents I've checked is ordered reasonably.

I just checked and you are right. I had problem with one particular torrent and that's why I assumed it would be the case everywhere.

That said, getting rid of file sorting would let for yet another simplification of the code.

Anyway, for the sake of consistency, I think the alphabetical order should go either both in playlists and torrent list or none of them.

@Goldob @mathiasvr I agree, I think keeping the original order is probably a good idea.

@feross what was the reason for sorting torrents alphabetically?

I guess sort by name is okay.
I agree that it should be consistent, maybe it would be easier to sort the files initially and stick with that order everywhere else?

Goldob · 2016-09-01T23:05:51Z

Thank you very much for the feedback! I'll introduce the suggested changes.

The shuffle and repeat options are here only because I implemented them in the original branch (before rebase). I'll revert the commit and you'll be able to just skip it when merging.

@mathiasvr, similar error appears sometimes if you change tracks too fast. It is caused by the Chrome issue, just like you said. In case of autoplaying my guess is that it tries to replay the file after reaching the end, but before it is able to do that, new media object replaces it.

This reverts commit 9284122.

dcposch · 2016-09-02T02:18:39Z

@Goldob looks great. I have a few nitpicks still but they're really small and easy to fix later. 🚢 🚢 🚢

Goldob · 2016-09-02T06:19:17Z

Awesome to see that merged. One remark from me, before actually releasing this it would be nice to add a line to renderer/lib/migrations.js to remove torrentSummary.defaultPlayFileIndex, because we don't use it anymore.

dcposch · 2016-09-02T21:14:03Z

@Goldob fixed b8effff

Goldob added 4 commits September 1, 2016 20:37

Open multi-file torrents as playlists

b8f6f9f

Add repeat and shuffle options to the player

9284122

Autoplay first file in torrent

964aeda

* replaces `pickFileToPlay` feature * when reopening player, restores the most recently viewed file

Add playlist navigation buttons to Windows thumbar

9bcf8d4

dcposch reviewed Sep 1, 2016
View reviewed changes

mathiasvr reviewed Sep 1, 2016
View reviewed changes

Goldob added 3 commits September 2, 2016 01:08

Remove repeat and shuffle options

706484e

This reverts commit 9284122.

Play files in order they appear in torrent

7aa7875

Clean up playlists code

17b31f0

dcposch merged commit 3073230 into webtorrent:master Sep 2, 2016

Goldob deleted the playlists branch September 2, 2016 06:13

Goldob mentioned this pull request Sep 2, 2016

Restore playback state when reopening player #877

Merged

Goldob mentioned this pull request Sep 6, 2016

VLC Playback rework #906

Closed

This was referenced Sep 6, 2016

Turn torrent into 'playlist': automatically start playing next item #123

Closed

play first video file #751

Closed

lock bot locked as resolved and limited conversation to collaborators May 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add playlists feature #871

Add playlists feature #871

Goldob commented Sep 1, 2016 •

edited

Loading

dcposch Sep 1, 2016 •

edited

Loading

dcposch commented Sep 1, 2016

dcposch commented Sep 1, 2016

dcposch Sep 1, 2016

mathiasvr commented Sep 1, 2016

mathiasvr Sep 1, 2016

Goldob Sep 1, 2016

mathiasvr Sep 1, 2016

Goldob Sep 1, 2016 •

edited by dcposch

Loading

dcposch Sep 1, 2016

mathiasvr Sep 1, 2016

Goldob commented Sep 1, 2016

dcposch commented Sep 2, 2016

Goldob commented Sep 2, 2016

dcposch commented Sep 2, 2016

Add playlists feature #871

Add playlists feature #871

Conversation

Goldob commented Sep 1, 2016 • edited Loading

dcposch Sep 1, 2016 • edited Loading

Choose a reason for hiding this comment

dcposch commented Sep 1, 2016

dcposch commented Sep 1, 2016

dcposch Sep 1, 2016

Choose a reason for hiding this comment

mathiasvr commented Sep 1, 2016

mathiasvr Sep 1, 2016

Choose a reason for hiding this comment

Goldob Sep 1, 2016

Choose a reason for hiding this comment

mathiasvr Sep 1, 2016

Choose a reason for hiding this comment

Goldob Sep 1, 2016 • edited by dcposch Loading

Choose a reason for hiding this comment

dcposch Sep 1, 2016

Choose a reason for hiding this comment

mathiasvr Sep 1, 2016

Choose a reason for hiding this comment

Goldob commented Sep 1, 2016

dcposch commented Sep 2, 2016

Goldob commented Sep 2, 2016

dcposch commented Sep 2, 2016

Goldob commented Sep 1, 2016 •

edited

Loading

dcposch Sep 1, 2016 •

edited

Loading

Goldob Sep 1, 2016 •

edited by dcposch

Loading