LBRYCommunity/lbry-desktop

Fork 0

Autoplay #1453

Merged

daovist merged 4 commits from autoplay into master

2018-05-11 20:16:26 +02:00

daovist commented

2018-05-07 20:04:57 +02:00

(Migrated from github.com)

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.

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.

🎉 1

neb-b (Migrated from github.com) requested changes 2018-05-10 06:06:43 +02:00

neb-b (Migrated from github.com) left a comment

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.

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 commented

2018-05-10 17:22:52 +02:00

(Migrated from github.com)

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.

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 commented

2018-05-10 18:07:28 +02:00

(Migrated from github.com)

@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 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).

neb-b commented

2018-05-10 18:12:30 +02:00

(Migrated from github.com)

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

neb-b commented

2018-05-10 18:15:09 +02:00

(Migrated from github.com)

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)

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 commented

2018-05-10 20:20:30 +02:00

(Migrated from github.com)

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

kauffj commented

2018-05-10 22:15:00 +02:00

(Migrated from github.com)

@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 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.

neb-b commented

2018-05-11 03:27:42 +02:00

(Migrated from github.com)

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.

neb-b commented

2018-05-11 03:42:34 +02:00

(Migrated from github.com)

@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 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.

neb-b (Migrated from github.com) approved these changes 2018-05-11 19:47:47 +02:00

No reviewers

neb-b