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.

Sign up for GitHub

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

Jump to bottom

Expanded Playback and List controls #6921

Merged
merged 22 commits into from Sep 2, 2021
Merged

Expanded Playback and List controls #6921

merged 22 commits into from Sep 2, 2021

Conversation

rafael-xmr
Copy link
Collaborator

@rafael-xmr rafael-xmr commented Aug 20, 2021 •
edited

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

What is the new behavior?

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

@rafael-xmr rafael-xmr mentioned this pull request Aug 20, 2021
Merged
@jessopb
Copy link
Member

jessopb commented Aug 23, 2021

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
Copy link
Member

jessopb commented Aug 23, 2021 •
edited

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
Copy link
Member

jessopb commented Aug 23, 2021

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
Copy link
Member

jessopb commented Aug 23, 2021

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

jessopb
jessopb reviewed Aug 23, 2021
View reviewed changes
ui/redux/actions/content.js
return (dispatch: Dispatch, getState: () => any) => {
if (shuffle) {
const state = getState();
const urls = makeSelectUrlsForCollectionId(collectionId)(state);

const newUrls = urls
let newUrls = urls
Copy link
Member

Choose a reason for hiding this comment

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

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

@jessopb
Copy link
Member

jessopb commented Aug 23, 2021

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

@rafael-xmr rafael-xmr force-pushed the playback-controls branch 3 times, most recently from 613bdd4 to 67859f6 Compare August 24, 2021 14:13
@rafael-xmr
Copy link
Collaborator Author

✔️ 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

@jessopb
Copy link
Member

jessopb commented Aug 24, 2021 •
edited

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!

@rafael-xmr
Copy link
Collaborator Author

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
Copy link
Contributor

tzarebczan commented Aug 25, 2021 •
edited by rafael-xmr

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.

@rafael-xmr rafael-xmr force-pushed the playback-controls branch 2 times, most recently from b189f43 to 63e2508 Compare August 31, 2021 16:17
@rafael-xmr
Copy link
Collaborator Author

rafael-xmr commented Aug 31, 2021 •
edited by tzarebczan

  • 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

@rafael-xmr rafael-xmr force-pushed the playback-controls branch 6 times, most recently from 869e8ff to 9d3a4c7 Compare September 2, 2021 13:11
@tzarebczan tzarebczan mentioned this pull request Sep 2, 2021
Open
@tzarebczan tzarebczan merged commit 64cbd4a into master Sep 2, 2021
@tzarebczan tzarebczan deleted the playback-controls branch September 2, 2021 20:05
rafael-xmr pushed a commit that referenced this pull request Sep 7, 2021
Squashed commit of the following:
6287a61 
commit b2ec92e
Author: saltrafael <a657ee92-7eb9-46eb-97d2-6735799df838@aleeas.com>
Date:   Tue Sep 7 07:19:54 2021 -0300

    Better handle paid content on playlists

commit 7291a2f
Author: saltrafael <a657ee92-7eb9-46eb-97d2-6735799df838@aleeas.com>
Date:   Mon Sep 6 09:21:21 2021 -0300

    Fix and improve code

commit 802b473
Author: saltrafael <a657ee92-7eb9-46eb-97d2-6735799df838@aleeas.com>
Date:   Tue Sep 7 07:31:31 2021 -0300

    Add snack bar notification

commit 124aa90
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Tue Sep 7 16:03:05 2021 +0800

    Fix Duration widget error-message logic

    - It didn't remove the error msg when the input is cleared.
    - It didn't update to the latest error msg if one already existed.

commit 6a5ea5d
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Tue Sep 7 16:10:40 2021 +0800

    Vertically center 'icon--help' (redo)

    A better fix with extra specificity, since the main issue was with form-fields only. The previous fix was too wide and broke other areas with column flex.

commit a6ece46
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Tue Sep 7 14:40:59 2021 +0800

    Vertically center 'icon--help'

    Reasons:
    - Better symmetry.
    - When used on a form-field label, the off-centeredness causes an extra offset at the top. Since the icon goes away when there is an input error, the form-field shifts a bit. (An alternative is to use a smaller icon, but I think size 16 is the best for legibility).

commit a227e6a
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Tue Sep 7 10:25:38 2021 +0800

    i18n + vjs i18n fixes

    - Reverted #7004 as it ended up preventing the rest of the components from being localized when there is an error. Replaced that with a `setLabel` that simply checks if the component exists before setting the label.

    - Fix "Autoplay Next On" not localized on initial load -- it was only localized when the setting changes. It is unfortunate that we need to also set the label in an additional `useEffect` instead of just using vjs events, but so be it.

commit aceb8b8
Author: saltrafael <76502841+saltrafael@users.noreply.github.com>
Date:   Mon Sep 6 14:33:53 2021 -0300

    Fix autoplay not saving (#7028)

    * Fix autoplay not saving

    * Bump redux

commit f246992
Author: saltrafael <76502841+saltrafael@users.noreply.github.com>
Date:   Mon Sep 6 11:06:07 2021 -0300

    Fix auto downloads (#7021)

commit 95278f1
Merge: 91ef545 f8e9047
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Fri Sep 3 23:53:49 2021 +0800

    #7015 Allow moderators to perform Timeout as well.

commit f8e9047
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Fri Sep 3 23:33:04 2021 +0800

    Allow moderators to perform Timeout as well.

commit 04f3dfb
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Fri Sep 3 23:28:09 2021 +0800

    Corrected logic that was bypassing the muted list.

commit 91ef545
Author: jessopb <36554050+jessopb@users.noreply.github.com>
Date:   Fri Sep 3 11:29:23 2021 -0400

    Fix stale recsys and fix watchman uncaught promise when videoPlayer is not initialized (#7014)

    * do not set stale recsys id

    * no watchman send if videoplay not initialized

commit 03d56d1
Merge: 049fb28 bf8ab2e
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Fri Sep 3 22:42:26 2021 +0800

    #6872 Comment Moderation - time based bans

commit bf8ab2e
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Fri Sep 3 15:43:01 2021 +0800

    Run the extra app-side comment filter only if the claim is not ours.

    ## Preamble
    - The app-side uses a cached blocklist, so when a Timeout expires, it doesn't know that the ban has been lifted.
    - Meanwhile, we are doing extra comment filtering using this blocklist (so that we don't see comments that we have blocked, regardless of whose claim we are viewing).

    ## Issue
    In a livestream, if a new message from an ex-offender comes in after their ban has been lifted, we do get the websocket message but it's being filtered out locally as mentioned above. So, the msg ended up being visible for everyone except the owner.

    ## Fix (band aid)
    - Don't run the extra filter if the claim we are viewing is ours -- commentron would have filtered it for us anyways, and is the right logic to use even before this Timeout feature is introduced.
        - For the case of Timeout, this only serves as a band-aid until Commentron Issue 80 is available for us to detect the ban has been lifted. This is because it doesn't handle the case where I am a viewer and I decided to timeout someone for a few minutes. Because I am not the owner of the claim, the offender will continue to be blocked due to the same issue mentioned above.

commit 804edd3
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Fri Sep 3 16:47:06 2021 +0800

    Restrict "Timeout" feature to content owners

    The main reason to do this is because we are doing extra comment filtering using our blocklist (so that we don't see people we blocked regardless on who's content we are at), but we are also using a cached blocklist so we don't know when a Timeout will be lifted to allow new livestream messages. We will need Commentron Issue-80 to truly fix this.

    For the case of when we are the owner of the content, we don't run the extra filtering (since it is equivalent to Commentron's filtering), hence the issue doesn't exist. Any new livestream messages received through websocket won't be locally filtered.

commit 0c1554e
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Fri Aug 20 15:18:54 2021 +0800

    Blocklist Page: show the timeout ban duration

    - 'humanize-duration' is used because 'moment''s humanizer sucks.

commit 663376e
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Fri Aug 20 09:04:48 2021 +0800

    Block timeout was changed from "hours" to "seconds" in Commentron

commit a05ccdd
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Thu Aug 12 15:10:44 2021 +0800

    Comment Moderation - time based bans

    ## Issue
    6712 Comment Moderation - time based bans

    ## Approach
    - Consolidated the 3 types of blocking buttons in the comment content menu (i.e. Block, Moderator Block, Admin Block) into 1 regular Block button.
    - Show a modal when Block is clicked.
        - Let user choose the blocklist.
        - Let user choose the timeout duration (this PR's impetus).

commit 049fb28
Author: jessopb <36554050+jessopb@users.noreply.github.com>
Date:   Thu Sep 2 18:39:40 2021 -0400

    recsys v0.2 (#6977)

    * recsys wip

    better logging

    fix floating player popout playing uri bug with recsys

    lint

    add empty entries to create

    use beacon; fire on visibilitychange

    cleanup, not record recs if not seen

    ifweb recsys beacon

    recsys handle embeds, cleanup

    use history.listen to trigger events

    fix recsys embed bug

    bugfix

    more default data

    cleaner

    cleaner

    * remove tentative

    * disable recsys debug logging

commit 64cbd4a
Author: saltrafael <76502841+saltrafael@users.noreply.github.com>
Date:   Thu Sep 2 17:05:32 2021 -0300

    Expanded Playback and List controls (#6921)

    * Dont show countdown on Lists

    * Add Repeat icon

    * Add Shuffle icon

    * Add Replay Icon

    * Add Replay Option to autoplayCountdown

    * Add Loop Control for Lists

    * Add Shuffle control for Lists

    * Improve View List Link and Fetch action

    * Add Play Button to List page

    * Add Shuffle Play Option on List Page and Menus

    * Fix Modal Remove Collection I18n

    * CSS: Fix Large list titles

    * Fix List playback on Floating Player

    * Add Theater Mode to its own class and fix bar text display

    * Add Play Next VJS component

    * Add Play Next Button

    * Add Play Previous VJS Component

    * Add Play Previous Button

    * Add Autoplay Next Button

    * Add separate control for autoplay next in list

    * Bump redux

    * Update CHANGELOG.md

commit 061e4dd
Author: infinite-persistence <64950861+infinite-persistence@users.noreply.github.com>
Date:   Thu Sep 2 09:38:58 2021 -0700

    vjs: hide errors when updating i18n (#7004)

    ## Issue
    6989 console errors and warnings are getting out of hand!

    ## Notes
    This method was previously criticized in 5643. But given that no better solution has been submitted after a long while, nor is there a new solution for doing i18n, I'm reviving it again. It'll be no worse from the status quo.

    Despite try-catch being overkill, I think the code-clarity outweighs the performance issues (if any, in the first place). Also, we are only suppressing the error in a very specialized function which even if it fails, will simply be a no-op and the GUI falling back to English.

commit a0f164c
Author: infinite-persistence <inf.persistence@gmail.com>
Date:   Thu Sep 2 20:50:03 2021 +0800

    Add streamer badge

    https://lbryians.slack.com/archives/C01LTTCEURW/p1629489255028900
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jessopb jessopb jessopb left review comments

@infinite-persistence infinite-persistence infinite-persistence left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Countdown on lists Separate control for autoplay next in list
4 participants
@rafael-xmr @jessopb @tzarebczan @infinite-persistence