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

Autoplay #1453

Merged
merged 4 commits into from May 11, 2018

Conversation

Projects
None yet
4 participants
@daovist
Collaborator

daovist commented May 7, 2018

This PR implements an autoplay featured for issue #584

Autoplay is a user setting which can be checked on/off on the settings and file pages. When on, it will play videos that have downloaded at least one blob. Otherwise, it will begin the download.

This adds a new icon and some CSS.

@seanyesmunt

Nice @daovist. This will be a great feature. I have a few comments

Can you rebase so only your new code is in this PR? I'm not sure what you added and what is from other commits
I definitely don't think the checkbox should be on the file page. I think only having it in settings is fine.
I see you added a new icon, but I don't see it being used anywhere.

Lastly this doesn't seem to work for me. I've tried a few different free videos, but nothing happens when I navigate to them.

@daovist

This comment has been minimized.

Show comment
Hide comment
@daovist

daovist May 10, 2018

Collaborator

This PR should be properly rebased. I now understand that force pushing to a branch is normal.

I spoke with @kauffj about the UI and this is what we came up with. I should have explained this in more detail.

There needs to be some way the user can turn control autoplay on this page. I started with a button on the layover beneath the loading message but we looked at another video site and decided we liked a toggle switch and also that that was beyond the scope of this PR. We discussed creating a toggle switch interface to replace most/all checkboxes throughout, including this one.

There is also the issue of needing some way to cancel a download--which would be called when the autoplay is turned off--but that requires changes to the daemon first.

The icon part was from when I first had this working as a button.

I just tried again and "it works on my machine". I'm not sure what could be preventing autoplay for you.

Collaborator

daovist commented May 10, 2018

This PR should be properly rebased. I now understand that force pushing to a branch is normal.

I spoke with @kauffj about the UI and this is what we came up with. I should have explained this in more detail.

There needs to be some way the user can turn control autoplay on this page. I started with a button on the layover beneath the loading message but we looked at another video site and decided we liked a toggle switch and also that that was beyond the scope of this PR. We discussed creating a toggle switch interface to replace most/all checkboxes throughout, including this one.

There is also the issue of needing some way to cancel a download--which would be called when the autoplay is turned off--but that requires changes to the daemon first.

The icon part was from when I first had this working as a button.

I just tried again and "it works on my machine". I'm not sure what could be preventing autoplay for you.

@kauffj

This comment has been minimized.

Show comment
Hide comment
@kauffj

kauffj May 10, 2018

Member

@seanyesmunt re: checkbox placement I did ask @daovist to put it there, but none of us were in love with that.

The thought process was:

  • If the feature starts off and is only in settings than the vast majority of people will not discover it.
  • Showing a popup is annoying for first-run users ("I just want to watch the video, stop popping stuff up")
  • YouTube does display autoplay on their equivalent of a file page, though the behavior is different (it toggles whether to continue to play videos).
Member

kauffj commented May 10, 2018

@seanyesmunt re: checkbox placement I did ask @daovist to put it there, but none of us were in love with that.

The thought process was:

  • If the feature starts off and is only in settings than the vast majority of people will not discover it.
  • Showing a popup is annoying for first-run users ("I just want to watch the video, stop popping stuff up")
  • YouTube does display autoplay on their equivalent of a file page, though the behavior is different (it toggles whether to continue to play videos).
@seanyesmunt

This comment has been minimized.

Show comment
Hide comment
@seanyesmunt

seanyesmunt May 10, 2018

Member

What about a snackbar message? That way we would display the info without affecting a playing video.

Member

seanyesmunt commented May 10, 2018

What about a snackbar message? That way we would display the info without affecting a playing video.

@seanyesmunt

This comment has been minimized.

Show comment
Hide comment
@seanyesmunt

seanyesmunt May 10, 2018

Member

I'm really not a fan of having it on this page at all, because it won't do anything. If a video has already started, and they un-check it, the video will continue to play. And if a video isn't playing and they check it, it won't start playing.

For the majority of people of who keep this setting on, it will just be an extra input on the page that they never interact with (unless by accident, and they wonder why the videos aren't playing anymore)

Member

seanyesmunt commented May 10, 2018

I'm really not a fan of having it on this page at all, because it won't do anything. If a video has already started, and they un-check it, the video will continue to play. And if a video isn't playing and they check it, it won't start playing.

For the majority of people of who keep this setting on, it will just be an extra input on the page that they never interact with (unless by accident, and they wonder why the videos aren't playing anymore)

@daovist

This comment has been minimized.

Show comment
Hide comment
@daovist

daovist May 10, 2018

Collaborator

If a video isn't playing, this does make it start playing.

Collaborator

daovist commented May 10, 2018

If a video isn't playing, this does make it start playing.

@kauffj

This comment has been minimized.

Show comment
Hide comment
@kauffj

kauffj May 10, 2018

Member

@seanyesmunt if you want to remove it you are welcome to.

I will say that I think a snackbar is wrong. Snackbars are to give feedback related to actions the user took.

Member

kauffj commented May 10, 2018

@seanyesmunt if you want to remove it you are welcome to.

I will say that I think a snackbar is wrong. Snackbars are to give feedback related to actions the user took.

@seanyesmunt

This comment has been minimized.

Show comment
Hide comment
@seanyesmunt

seanyesmunt May 11, 2018

Member

Still trying to see why it isn't playing for me. I think we can keep it. I think a better spot for it would be on the right side, below the tip/subscribe buttons.

Member

seanyesmunt commented May 11, 2018

Still trying to see why it isn't playing for me. I think we can keep it. I think a better spot for it would be on the right side, below the tip/subscribe buttons.

@seanyesmunt

This comment has been minimized.

Show comment
Hide comment
@seanyesmunt

seanyesmunt May 11, 2018

Member

@daovist Ah I realized why. You have a check to not autoplay if users have obscureNSFW as false. I think it's ok to play content if they have that setting checked, as long as the content is not marked NSFW. And in that case, there should be some help text under the checkbox saying "we won't autoplay this because you have NSFW content hidden" (not that but similar). It is really confusing in it's current state that it isn't playing because I have some other setting checked.

Another thing I would add is having some check inside componentWillReceiveProps to only call handleAutoplay if it's necessary, so it's not being called a bunch of times when data that isn't relevant to this piece of code is changing.

Member

seanyesmunt commented May 11, 2018

@daovist Ah I realized why. You have a check to not autoplay if users have obscureNSFW as false. I think it's ok to play content if they have that setting checked, as long as the content is not marked NSFW. And in that case, there should be some help text under the checkbox saying "we won't autoplay this because you have NSFW content hidden" (not that but similar). It is really confusing in it's current state that it isn't playing because I have some other setting checked.

Another thing I would add is having some check inside componentWillReceiveProps to only call handleAutoplay if it's necessary, so it's not being called a bunch of times when data that isn't relevant to this piece of code is changing.

daovist and others added some commits Apr 30, 2018

Autoplay: start playing/downloading video/audio/image in video compon…
…ent. Adds on/off checkbox on settings and file pages. Adds CHECK_SIMPLE icon (w/o circle).

add changelog entry

@seanyesmunt seanyesmunt merged commit 45aec4a into master May 11, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@seanyesmunt seanyesmunt deleted the autoplay branch May 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment