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.

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

Feat: SponsorBlock improvements #1849

Merged
merged 10 commits into from
May 29, 2022

Conversation

ChunkyProgrammer
Copy link
Member

@ChunkyProgrammer ChunkyProgrammer commented Oct 23, 2021


Sponsorblock improvements

Pull Request Type
Please select what type of pull request this is:

  • Bugfix
  • Feature Implementation

Related issue
closes #1840 (All categories implemented)
closes #1440 (User decides color for each category)
#1380 (Auto-skip is now optional, no prompt to skip however)

Description
This PR allows users to configure each SponsorBlock category individually (Color + Skip Mode)

Skip Modes:

  • Don't Skip (No indication of sponsor will be shown in seek bar)
  • Show In Seek Bar (Show the sponsored segment in the seek bar but don't autoskip)
  • AutoSkip (AutoSkip when the video reaches the sponsored segment)

Mode not included (Might do a future PR to implement it):

  • Prompt To Skip (Have a "Skip Sponsored Segment" button appear when a sponsored segment is reached)

Screenshots (if appropriate)
image

image

Testing (for code that is not small enough to be easily understandable)
Has this pull request been tested?
Yes, tested by updating each category to make sure the setting works and loading a video with multiple categories involved

Desktop (please complete the following information):

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 0.15.0

@PrestonN PrestonN enabled auto-merge (squash) October 23, 2021 14:06
@ChunkyProgrammer ChunkyProgrammer added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 23, 2021
@ChunkyProgrammer
Copy link
Member Author

@efb4f5ff-1298-471a-8973-3d47447115dc do you think this should close #1440 ?

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Constant definition code deduplicate oppotunity?

src/renderer/components/ft-video-player/ft-video-player.js Outdated Show resolved Hide resolved
src/renderer/store/modules/settings.js Outdated Show resolved Hide resolved
src/renderer/store/modules/settings.js Outdated Show resolved Hide resolved
src/renderer/store/modules/settings.js Outdated Show resolved Hide resolved
src/renderer/store/modules/settings.js Outdated Show resolved Hide resolved
src/renderer/store/modules/settings.js Outdated Show resolved Hide resolved
src/renderer/store/modules/settings.js Outdated Show resolved Hide resolved
src/renderer/components/ft-video-player/ft-video-player.js Outdated Show resolved Hide resolved
src/renderer/components/ft-video-player/ft-video-player.js Outdated Show resolved Hide resolved
@efb4f5ff-1298-471a-8973-3d47447115dc

@efb4f5ff-1298-471a-8973-3d47447115dc do you think this should close #1440 ?

Oh yes definitely!

@efb4f5ff-1298-471a-8973-3d47447115dc

The categories work for me, so if it detects a certain category it skips it but i never get to see the colors in the seek bar but maybe that is related to #1439

@ChunkyProgrammer
Copy link
Member Author

The categories work for me, so if it detects a certain category it skips it but i never get to see the colors in the seek bar but maybe that is related to #1439

Is this color showing when not testing this PR?

@efb4f5ff-1298-471a-8973-3d47447115dc

Nope its not showing for me on the latest nightly build.

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Code Review cleared
But how do I test this locally?

@peepo5
Copy link
Contributor

peepo5 commented Nov 6, 2021

Hi, are these large settings titles still in this PR or have they been made smaller? In my opinion they should not be bold and should be around 3px smaller in size.

Another nitpick (not your fault but worthy of adding in this PR);
The enable toggle and notify toggle should be aligned by their button, not their center of text, for consistency. The enable toggle should also not have so much padding.

@efb4f5ff-1298-471a-8973-3d47447115dc

Do the colors show in your seek bar?

@ChunkyProgrammer
Copy link
Member Author

Do the colors show in your seek bar?

Yes,
image

@ChunkyProgrammer
Copy link
Member Author

ChunkyProgrammer commented Nov 12, 2021

Code Review cleared But how do I test this locally?

I'll try to get a list of videos for each category soon. To test it would basically be:

  • Enable sponsor block
  • individually enable categories
  • go on a video and see seek bar populate with different colored markers (autoskips when autoskip enabled for category)

@efb4f5ff-1298-471a-8973-3d47447115dc

I don't understand why it doesn't show the colors in the bar for me. I tried out a bunch of videos but none of them show any of the catagorie colors

@elesto
Copy link

elesto commented Jan 7, 2022

Will you add Filler Tangent skip?

@ChunkyProgrammer
Copy link
Member Author

Will you add Filler Tangent skip?

Was not aware of that category, I will add it

@elesto
Copy link

elesto commented Jan 18, 2022

More features just released! https://blog.ajay.app/full-video-sponsorblock

@maxnordlund
Copy link
Contributor

@PikachuEXE did you manage to test this locally? I would love to see this in an already amazing app. I want to know if there's sponsored content, but not automatically skip it, and this would allow for that (and more).

@PikachuEXE
Copy link
Collaborator

  1. My last review comment already said I don't know how to test this
  2. There are code conflicts

loading a video with multiple categories involved

At least I need the URL(s) for the video(s) 😅

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Apr 9, 2022
@maxnordlund
Copy link
Contributor

Here's a video with a few SponsorBlock segments: https://youtu.be/V_xFDsxPmNM

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented May 13, 2022

@maxnordlund
Thanks for helping!
Though I am still waiting for code conflict to be resolved...
Wait can I do it myself?

Edit: Resolved conflicts myself, force pushed

@PikachuEXE PikachuEXE added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: merge conflicts / rebase needed labels May 13, 2022
@efb4f5ff-1298-471a-8973-3d47447115dc

today my machine finally let me review this properly.

This might be a problem. U cant see where u are in the video because the segmentcolor is blocking the seekbarcolor. U also cant know if it has buffered that part of the video.

VirtualBoxVM_g5qwJEe4sF.mp4

This is not related to this PR but the user doesn't get to see the notification in fullscreen and fullwindow

@ChunkyProgrammer
Copy link
Member Author

today my machine finally let me review this properly.

This might be a problem. U cant see where u are in the video because the segmentcolor is blocking the seekbarcolor. U also cant know if it has buffered that part of the video.

Do you think making the indicators semi-transparent will solve the issue you have with the segmentcolor?

@efb4f5ff-1298-471a-8973-3d47447115dc

today my machine finally let me review this properly.
This might be a problem. U cant see where u are in the video because the segmentcolor is blocking the seekbarcolor. U also cant know if it has buffered that part of the video.

Do you think making the indicators semi-transparent will solve the issue you have with the segmentcolor?

Yeah i think that making it semi transparent will solve the issue. Maybe making it only semi transparent when the user get at the segment. So before the segement and after it just stays a solid color and in the segment semi-transparent?

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels May 23, 2022
@ChunkyProgrammer ChunkyProgrammer added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels May 24, 2022
Copy link
Member

Choose a reason for hiding this comment

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

LGTM Thanks ChunkyWizard!

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented May 25, 2022

I think 0.6 would be a better value since 0.4 make it too dim... especially for yellow and green
(Also research linear gradient... not sure if possible or looks good)
Edit: linear gradient with rgba impossible as we have color values in names (e.g. blue), HEX values required...

0.6:
image
0.4:
image

0.8 looks like 1.0 lol
image

@PikachuEXE
Copy link
Collaborator

@absidue ~

@PrestonN PrestonN merged commit dfb28b1 into FreeTubeApp:development May 29, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label May 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sponsorblock: support more categories Sponserblock segment and progressbar theme settings
8 participants