-
Notifications
You must be signed in to change notification settings - Fork 891
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 autoplay toggle to the video player #5866
Add autoplay toggle to the video player #5866
Conversation
src/renderer/components/ft-shaka-video-player/player-components/AutoplayToggle.js
Show resolved
Hide resolved
src/renderer/components/ft-shaka-video-player/player-components/AutoplayToggle.js
Show resolved
Hide resolved
…s/AutoplayToggle.js Co-authored-by: absidue <[email protected]>
…., in another window
…FreeTube into feat/autoplay-in-media-player
src/renderer/components/ft-shaka-video-player/ft-shaka-video-player.js
Outdated
Show resolved
Hide resolved
src/renderer/components/ft-shaka-video-player/ft-shaka-video-player.js
Outdated
Show resolved
Hide resolved
Co-authored-by: absidue <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I've never even noticed this feature existed until you just pointed it out. Removed |
c0cce9e
to
3a55565
Compare
I wonder why in additional to adding the button to player (great for mobile especially which I have no problem with) As a desktop user I expect controls for playlist to be all on that panel |
The "Stop autoplay after this video" button is a very niche feature amongst other apps and seems to have been our temporary replacement for an easily accessible autoplay toggle. The cost of users having to switch to using the autoplay toggle for their use cases is less than that of having to maintain two redundant features. Even if we were to make the older pause button act exactly like the autoplay toggle, the mental load cost of multiple controls on the same page with the same behavior is high and should be avoided whenever possible. |
Still not sure I understand your point. The autoplay toggle will be visible in the player bar in such a case. There is no point in moving the location of the autoplay toggle or duplicating it for different kinds of videos. Autoplay, as of this PR, will be a property identified with the video player rather than a playlist control, the same as the audio level or theater mode. |
There is a point, playlist related info should be visible on playlist panel including auto play enabled or not |
Will leave it for other team members than us to decide, as we seem to be in another of our famous impasses. Will leave my main points up:
|
This isnt playlist related info. All buttons in there affects the whole playlist (except for the standard skip to next/previous video buttons). This is a button that is related to the individual video and by the introduction of this button we made the association to the playlist.
That is just an example. The purpose of my issue is to make it the same as YT. If we look at Autoplay in YT it is always enabled in playlists. BUT on YT everything that regards Autoplay is built into the player. So when new users come from YT to FT I'd argue that new users are mentally way more familiar with the way its done now. |
@efb4f5ff-1298-471a-8973-3d47447115dc I am not opposing adding a button in the player If the objective is the make FT looks like YT (not limited to this PR), then prev/next playlist item buttons should be in player as well If this is the case I can agree with the placement but only if the prev/next buttons are moved too (in the same/another PR) |
I think we're on the same page now. I / whomever can work on that in a separate PR ( |
Relevant issue #1220 :D @kommunarr maybe controls for previous next video can be implemented after #2138 (comment), In my mind sounds like the logical next step |
If i may ask what is the progress on this? Is this chore holding up this PR? |
@efb4f5ff-1298-471a-8973-3d47447115dc That's correct. There are decision points regarding the context menu that I would prefer to not be directly involved in, and I would also prefer for #6400 to go ahead first in the queue. |
…at/autoplay-in-media-player
Conflicts have been resolved. A maintainer will review the pull request shortly. |
#6400 expected to be merged after this? |
Pull request was converted to draft
I still needed to adjust the force overflow menu breakpoint; this is now ready for review. In retrospect, I think it would make sense for this to go first instead of #6400 so we can have one PR for implementing the logic for both autoplay settings. |
Briefly tested (except mobile & code review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this for 2 weeks no issue
Add autoplay toggle to the video player
Pull Request Type
Related issue
closes #1181
Description
Adds autoplay toggle to the media player.
Autoplay Playlist Videos
setting is the one exposed/manipulated. Otherwise, theAutoplay Recommended Videos
setting is the one exposed/manipulated.Watch.js
to callthis.$refs.watchVideoPlaylistplayNextVideo
.EN-US
values to more understandable meanings (see Screenshots section for details)autoplay
/autopause
. I decided onplay_circle
/play_pause
instead. One other considered alternative wastoggle_on
/toggle_off
, but it would have been less descriptive.Screenshots
Autoplay off:
Autoplay on:
Mobile overflow menu:
Autoplay Settings Label/Ordering Changes:
(Autoplay Videos -> Start Videos Automatically, moved to bottom)
(Play Next Video -> Autoplay Recommended Videos, moved to top)
(Autoplay Playlists -> Autoplay Playlist Videos)
Rationale: define "autoplay" consistently, colocate autoplay buttons, colocate the non-autoplay right-column buttons. To preempt the potential criticism that moving the controls might confuse users who have forced themselves into learning the existing labels/ordering, I disagree. I have worked a good deal with these values as both a developer and a user, and I still mess these up. I would contend that the benefit of the changes starkly outweighs the risk.
Testing
Hide Recommended Videos
is enabledDesktop
Additional context
I considered firing
handleEnded
on enabling autoplay on an ended video in order to start the autoplay countdown, but I decided against it. Partially because it's a race condition risk to wait sufficiently long forisAutoplayEnabled
to update, partially because it's not that important.I originally marked this story as a
good first issue
, but considering its 3+ y/o age and the amount of time it would take to guide a new contributor through all of the above bullet points & intricacies, I decided to take a stab at it myself.