LBRYCommunity/lbry-desktop

Fork 0

Expanded Playback and List controls #6921

Merged

saltrafael merged 22 commits from playback-controls into master

2021-09-02 22:05:32 +02:00

saltrafael commented

2021-08-20 17:45:20 +02:00

(Migrated from github.com)

🟢 Available for testing on https://salt.odysee.com

What is the new behavior?

Don't show countdown on a list (Closes #6321)
Added "Replay" option on autoplay (#2921)
Added Loop option on Lists
Added Shuffle option on Lists
Added link to "View List" popup option, so can be opened on a new tab
Clicking on the title of a floating player will take you back to the list being played
Play Next/Previous (with shortcuts SHIFT+N SHIFT+P):
Separate control for autoplay next (Closes #2902)

Other information

PR Type & Checklist

PR Type...

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting)
Refactoring (no functional changes)
Documentation changes
Other - Please describe:

PR Checklist...

Please check all that apply to this PR using "x":

I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
I added a line describing my change to CHANGELOG.md
I have checked that this PR does not introduce a breaking change
This PR introduces breaking changes and I have provided a detailed explanation below

this is a long one
🚀 the best is yet to come

🟢 Available for testing on https://salt.odysee.com  ## What is the new behavior? - Don't show countdown on a list (Closes #6321) - Added "Replay" option on autoplay (#2921) ![image](https://user-images.githubusercontent.com/76502841/130255310-983351b3-a2cd-458e-9c0f-a9498723bf03.png) - Added Loop option on Lists - Added Shuffle option on Lists - Added link to "View List" popup option, so can be opened on a new tab - Clicking on the title of a floating player will take you back to the list being played - Play Next/Previous (with shortcuts SHIFT+N SHIFT+P): ![Peek 2021-08-20 12-05](https://user-images.githubusercontent.com/76502841/130253915-096e9eb8-8dd8-4308-aa00-c0e491711fa3.gif) - Separate control for autoplay next (Closes #2902) ![Peek 2021-09-02 08-28](https://user-images.githubusercontent.com/76502841/131836021-566c4684-42c8-4290-90e4-f78af161c006.gif) ![image](https://user-images.githubusercontent.com/76502841/131836092-098a6856-69da-4400-8af6-1f95371a4796.png) ## Other information  ## PR Type & Checklist <details><summary>PR Type...</summary> What kind of change does this PR introduce? - [X] Bugfix - [X] Feature - [X] Code style update (formatting) - [ ] Refactoring (no functional changes) - [ ] Documentation changes - [ ] Other - Please describe: </details>  <details><summary>PR Checklist...</summary>  Please check all that apply to this PR using "x": - [X] I have checked that this PR is not a duplicate of an existing PR (open, closed or merged) - [ ] I added a line describing my change to CHANGELOG.md - [ ] I have checked that this PR does not introduce a breaking change - [ ] This PR introduces breaking changes and I have provided a detailed explanation below </details> this is a long one 🚀 the best is yet to come

🚀 4

jessopb commented

2021-08-23 03:29:13 +02:00

(Migrated from github.com)

This is huge.
I think copy list is its own thing.
modalCollectionAdd could conditionally return a simple collectionCopy component instead of the collectionAdd with the checkboxes etc.

This is huge. I think copy list is its own thing. modalCollectionAdd could conditionally return a simple collectionCopy component instead of the collectionAdd with the checkboxes etc.

jessopb commented

2021-08-23 03:37:54 +02:00

(Migrated from github.com)

Not sure about adding lists to sidebare in this way. Will discuss at planning. I'd be interested to see if we can have an accordion for followign, tags, and lists where only one is open?

Not sure about adding lists to sidebare *in this way*. Will discuss at planning. I'd be interested to see if we can have an accordion for followign, tags, and lists where only one is open?

jessopb commented

2021-08-23 03:56:57 +02:00

(Migrated from github.com)

It seems like coming up with a a shuffled next url can be done in the index.js of the a component, especially interacting and excluding based on content/history reducer. I'm not sure you need any work in lbry-redux for this.

jessopb commented

2021-08-23 03:57:28 +02:00

(Migrated from github.com)

I don't quite understand the case for selecting the previous url from collection with shuffle.

I don't quite understand the case for selecting the previous url from collection *with shuffle*.

jessopb (Migrated from github.com) reviewed 2021-08-23 03:59:59 +02:00

ui/redux/actions/content.js

jessopb (Migrated from github.com) commented

2021-08-23 03:16:08 +02:00

Is it worth filtering out at least the top 5 in content/history reducer? Or even the top <collection_size>?

jessopb commented

2021-08-23 17:22:37 +02:00

(Migrated from github.com)

At the meeting we thought we needed to brainstorm the design for sidebar playlists more - take it out for now?

saltrafael commented

2021-08-24 16:17:48 +02:00

(Migrated from github.com)

✔️ Separated Copy List
✔️ Took out Sidebar Lists

It seems like coming up with a a shuffled next url can be done in the index.js of the a component, especially interacting and excluding based on content/history reducer. I'm not sure you need any work in lbry-redux for this.

IMO on redux it avoids repetition of the same code every time you want to select a shuffled list

I don't quite understand the case for selecting the previous url from collection with shuffle.

going back on previously played shuffled videos, or else the ordering would be lost

✔️ Separated Copy List ✔️ Took out Sidebar Lists > It seems like coming up with a a shuffled next url can be done in the index.js of the a component, especially interacting and excluding based on content/history reducer. I'm not sure you need any work in lbry-redux for this. IMO on redux it avoids repetition of the same code every time you want to select a shuffled list > I don't quite understand the case for selecting the previous url from collection _with shuffle_. going back on previously played shuffled videos, or else the ordering would be lost

👍 1

jessopb commented

2021-08-24 18:17:47 +02:00

(Migrated from github.com)

Ah, I see. You're right.
How about just selecting the values right in the selector like is done here:
d016d8057b/src/redux/selectors/publish.js (L43)
state => state.content.shuffledUris,
state => state.content.shouldLoop,

It seems like a lot of params to be passing to the selector, and breaks the pattern of the selector name spelling out its requirements. I think the above might be cleaner.
Other than that, I think this is really close!

Ah, I see. You're right. How about just selecting the values right in the selector like is done here: https://github.com/lbryio/lbry-redux/blob/d016d8057be1c676db4ad0d21bdbcacd39a678eb/src/redux/selectors/publish.js#L43 state => state.content.shuffledUris, state => state.content.shouldLoop, It seems like a lot of params to be passing to the selector, and breaks the pattern of the selector name spelling out its requirements. I think the above might be cleaner. Other than that, I think this is really close!

saltrafael commented

2021-08-25 15:53:37 +02:00

(Migrated from github.com)

How about just selecting the values right in the selector like is done here:

✔️ Done, and wow this does look way cleaner
➕ Also just added "separate control for autoplay next"

> How about just selecting the values right in the selector like is done here: ✔️ Done, and wow this does look way cleaner ➕ Also just added "separate control for autoplay next"

tzarebczan commented

2021-08-25 21:26:54 +02:00

(Migrated from github.com)

Some other feedback/bugs:

mobile view doesn't show the autoplay/ < > buttons. I can click that area and it reacts, so the buttons are there, just not shown.
Loop = on goes into a crazy looping call where it locks up the whole browser by going back and forth between the playlist items. Seems related to view position being saved at the end (a rebase will fix this?)
With 1 item in playlist + loop on, i'd expect it to keep looping (doesn't)
Don't show count down timer if playlist mode
if autoplay is off in settings and you're in playlist mode, we don't show the autoplay next button - should this assume autoplay next is on, or should we just always show the button (I know youtube hide it).
if you have autoplay off + playing a video + then go to settings to enable autoplay, it crashes at : autoplayButton.removeClass('vjs-button--autoplay-next');
```
  at videojs.jsx:670
  at oc (react-dom.production.min.js:211)```  
```
on playlist view page, we have a "shuffle play" option, but no regular play. Clicking the first item goes to regular play. Add another button for "play all" or remove shuffle play (they can turn it on in the next page)?
the settings option will only control autoplay on load (adjust text), and autoplay next will be controlled from the player. We can consider adding it settings also.

Some other feedback/bugs: - [x] mobile view doesn't show the autoplay/ < > buttons. I can click that area and it reacts, so the buttons are there, just not shown. - [x] Loop = on goes into a crazy looping call where it locks up the whole browser by going back and forth between the playlist items. Seems related to view position being saved at the end (a rebase will fix this?) - [x] With 1 item in playlist + loop on, i'd expect it to keep looping (doesn't) - [x] Don't show count down timer if playlist mode - [ ] if autoplay is off in settings and you're in playlist mode, we don't show the autoplay next button - should this assume autoplay next is on, or should we just always show the button (I know youtube hide it). - [x] if you have autoplay off + playing a video + then go to settings to enable autoplay, it crashes at : autoplayButton.removeClass('vjs-button--autoplay-next'); ```instrument.ts:129 TypeError: Cannot read property 'removeClass' of undefined at videojs.jsx:670 at oc (react-dom.production.min.js:211)``` - [x] on playlist view page, we have a "shuffle play" option, but no regular play. Clicking the first item goes to regular play. Add another button for "play all" or remove shuffle play (they can turn it on in the next page)? - [ ] the settings option will only control autoplay on load (adjust text), and autoplay next will be controlled from the player. We can consider adding it settings also.

🚀 1

infinite-persistence (Migrated from github.com) reviewed 2021-08-26 11:39:23 +02:00

ui/component/viewers/videoViewer/internal/play-next.js

infinite-persistence (Migrated from github.com) commented

2021-08-26 11:39:22 +02:00

No need to localize internal name, otherwise it'll change based on language. I think.

saltrafael commented

2021-08-31 19:48:37 +02:00

(Migrated from github.com)

if autoplay is off in settings and you're in playlist mode, we don't show the autoplay next button - should this assume autoplay next is on, or should we just always show the button (I know youtube hide it).

the settings option will only control autoplay on load (adjust text), and autoplay next will be controlled from the player. We can consider adding it settings also.

These are up for discussion

if you have autoplay off + playing a video + then go to settings to enable autoplay, it crashes at : autoplayButton.removeClass('vjs-button--autoplay-next');
  at videojs.jsx:670
  at oc (react-dom.production.min.js:211)```  

~~This is the last one missing, I didn't manage to reproduce it @tzarebczan~~
Nevermind! fixed

> * [ ] if autoplay is off in settings and you're in playlist mode, we don't show the autoplay next button - should this assume autoplay next is on, or should we just always show the button (I know youtube hide it). > * [ ] the settings option will only control autoplay on load (adjust text), and autoplay next will be controlled from the player. We can consider adding it settings also. These are up for discussion > * [x] if you have autoplay off + playing a video + then go to settings to enable autoplay, it crashes at : autoplayButton.removeClass('vjs-button--autoplay-next'); > ``` > at videojs.jsx:670 > at oc (react-dom.production.min.js:211)``` > ``` ~~This is the last one missing, I didn't manage to reproduce it @tzarebczan~~ Nevermind! fixed