-
-
Notifications
You must be signed in to change notification settings - Fork 45
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 feature to toggle on/off sponsorblock with a single key press #101
base: main
Are you sure you want to change the base?
Conversation
a2efdf5
to
d67f311
Compare
d67f311
to
1debde6
Compare
How was this tested, and on what platform? Does anyone else have any comments? |
I tested this on my LG TV with OS version 03.33.95. I have been using this version every day since I raised this PR. And Haven't faced any issue so far. |
However, there's another feature that I'm thinking to build on top of it. I'll raise that as a separate PR as I'm not sure it would be accepted as a feature, and don't want to hinder this PR as a result. The idea is, while the SponsorBlock is turned off, and the current video timestamp is under a skippable segment and video is playing, upon turning on SponsorBlock using the GREEN button, I would like it to skip the current segment immediately. The use case is, sometimes I like to hear the sponsored segments. But after a few seconds of watching the segment, if I feel like skipping the segment, right now I have to press "right button" and scroll thru the timestamp manually, and sometimes it overshoots, so I have to watch some of the sponsored segment No need to scroll timeline back and forth. As I said, I haven't implemented it yet, but I plan to. So this PR could be a precursor to the new idea. |
@alyyousuf7 I like those changes, and I also very much like the idea in your last comment. But one note: I've tested this on an LG OLED C9 (webOS 4.9.7) and the changed popup messages on startup about which buttons you can press are not shown correctly. They are indistinguishable boxes as can be seen in this screenshot: |
Oh, so not all TVs support these unicodes. I'll try to find an alternative solution, like using some image instead. |
You could use words yellow and green if images don't work. Make yellow word in yellow colour would suffice. |
Those are literally just rectangles. A div with a background color will suffice (and maybe text on the inside indicating the color for accessibility). HTML using Tailwind classes for example: <span class="inline-block bg-green-500 h-1 w-2 text-sm font-bold">GREEN</span> |
I was in the middle of trying that out lol |
1debde6
to
67b6f15
Compare
c732497
to
b585c27
Compare
I also have accessibility concerns, especially with the colors being red and green. On my remote, red has one dot and green has two. However, I would like there to be text saying "red" and "green" somewhere. On a related note, does any of this work with Audio Guidance? |
I agree, there should also be text for the colors so that color blind people can understand them. |
Ok accessibility for color blind people - idea sold! Will add text inside the colored boxes. |
b585c27
to
121bb92
Compare
121bb92
to
6c07a1c
Compare
6c07a1c
to
3961784
Compare
May I suggest using a different shade of red? White on pure red might be hard to read. |
What do you think of these: UPDATE: pushed changes with colors mentioned above. |
3961784
to
322552b
Compare
322552b
to
56cc064
Compare
Better. Even better would be if the boxes had the same width for all four colors. |
I like the second one. Is this something that people are going to want to do frequently enough for it to be worth dedicating 1/3 of our remaining buttons to it? (I suppose we could use non-color buttons, but I'm not sure which ones are already used by YouTube.) Unfortunately I don't think detecting long presses is feasible (although I haven't actually investigated how keydown/up works in WAM). |
I personally use this feature quite often (my local build with this PR commits).
I don't think YouTube uses any buttons other than up/down/left/right/ok. My reasoning is based on the fact that YouTube for TV has to support many different TVs, and not all TVs would expose all buttons on web as key events except for these essential five buttons.
On my remote at least, I do not see any non-colored buttons that could be utilized for this feature. All non-colored buttons have very specific meaning. |
5763dbe
to
4b9160b
Compare
4b9160b
to
cf0cf90
Compare
In an ideal world, I'd like to see those features on buttons:
Of course that's only for SponsorBlock. There migth be other interesting features that could be placed on buttons. Back to this comment:
Don't you mean the red button? |
Now that I read it again, yes I meant Red button 🙈 |
Yellow button is already used to bring up search during video playback. |
Oh true. I also noticed that the blue button opens up the timeline during video playback. So essentially, the red button is the only one available to use, given we already use green button for config menu. |
We could also just reuse the green button to skip when the skip action is available. I don't think there's many instances where you would want to open the config menu instead while a skip prompt is active. |
The red button also does this when you run an app version without this PR. I assume this is just a fallback action. I think we can still use red and blue since the timeline can also be opened by up or down.
There are skip prompts in this app? I've never seen one of those. I only get notifications for automatic skips. |
There aren't any skip prompts yet in this app, but the original Chrome extension for SponsorBlock does. |
Just a heads up: there are fairly significant changes to the config system in the works, some of which have already been merged. Still to come is the ability to listen for changes to config options, which can be previewed here. Sorry to step on your PRs. I'm hoping the result ends up being easier to work with, though. (I got tired of having to edit hardcoded HTML every time I wanted to add an option.) You might want to wait until everything lands before continuing work on the parts that are going to touch config stuff. I'm also willing to resolve the merge conflicts for you if you'd rather not have to deal with it. Edit: Should be mostly stable now until after the next release. |
No worries, I'll keep an eye on new changes, and resolve conflicts as they land to master branch. |
I recreated the whole app (just for fun, with my own touch to it) - and after lots of experimentation, I ended up creating a separate config to enable/disable only the auto-skipping feature, instead of enabling/disabling SponsorBlock completely. https://github.com/alyyousuf7/youtube-webos It's a complete rewrite, i'm not here to promote my version - i'm suggesting that maybe this repo should also head the same direction for this specific feature, with a button to skip segment on demand only. But I guess the first step would be to introduce remote key presses based on what page the user is on right now - what @cremor suggested. |
This looks awesome - what do we need to merge it? |
width: 21.34px; | ||
height: 21.34px; |
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.
Where did you get 21.34px
?
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 think it was the computed height of the text before button. And I wanted to keep the height of the button same as text, so it doesn't look weird.
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.
Hmm. I think it must have originally been 16pt (21.3333...px).
window.sponsorblock = new SponsorBlockHandler(videoID); | ||
window.sponsorblock.init(); |
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.
Why is the SponsorBlock object always created now?
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.
Oh, I think I see. It's always running, but if it's not enabled, getSkippableCategories()
just returns an empty list.
Rebased version (untested): https://github.com/throwaway96/youtube-webos/tree/sponsorblock-skip-1 I'd like to see this adapted to take advantage of the new config event system, but I doubt I'll have the time to do it myself. |
I quite often find myself to toggle on/off the sponsorblock, as I prefer watching in-video sponsorships on some videos.
There are two features I added to make that possible: