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

Allows sponsor segments to be watched even if Auto Skip is enabled #3116

Merged
merged 3 commits into from
Feb 5, 2023

Conversation

lodwhet
Copy link
Contributor

@lodwhet lodwhet commented Jan 25, 2023

Allow sponsor segments to be watched even if Auto Skip is enabled

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Partial fix for #1380.

Description

Currently, when SponsorBlock is enabled with the Auto Skip option turned on, sponsor segments are automatically skipped without letting the user watch them. They have to go into the settings, disable SponsorBlock Auto Skip and then return on the video.

This PR allows the user to watch sponsor segments (even if Auto Skip is enabled) by manually seeking into them (by clicking on the progress bar or with arrow keys). A keyboard shortcut has also been added: 'b' or 'B' (may be changed). When it is pressed, the playback is moved at the beginning of the previous sponsor segment (if there is one), thus allowing the user to watch it completely if it has been auto-skipped.

Testing

I manually tested the changes in dev:web on videos with multiple sponsor segments. I also traced the execution to see if the code works as intended.

Desktop

  • OS: Arch Linux
  • OS Version: Rolling
  • FreeTube version: development branch

Additional context

There is an annoying behavior in videojs that can happen while seeking. The corresponding seeking event can be called after the first timeupdate event on the new time. Therefore, sponsor segments can be skipped even before the seeking event has been triggered. To deal with this problem, I used a countdown (skipCountdown) that only skip sponsor segments after 2 timeupdate events. Note that I have never seen 2 timeupdate events triggered before the seeking event. If it happens sometimes, then we have to increase the countdown value, or find another (better) workaround.

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 25, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 25, 2023 17:26
@PikachuEXE
Copy link
Collaborator

I agree adding Allow sponsor segments to be watched but not the keyboard shortcut

@lodwhet
Copy link
Contributor Author

lodwhet commented Jan 26, 2023

but not the keyboard shortcut

I agree that 'b' is probably not the best key choice. It can be changed. But why do you want to remove it completely?

@PikachuEXE
Copy link
Collaborator

That feature description is confusing to me
the playback is moved at the beginning of the previous sponsor segment (if there is one)
Looking at the implementation it seems moving to the closest segment in the past
But not just for sponsor type, any type that has an action set
Since the skipSegments comes from sponsorBlockSkipSegments, and the FT passes categories with action not set as doNothing

if (sponsorVal.skip !== 'doNothing') {
seekBar.push(x)
}

For the solution itself (as to make disabling auto skip easier), what about having a keyboard shortcut to disable Auto Skip for this video only?
Allowing the player to auto skip it and then pressing a keyboard shortcut to jump back seems like an odd solution/workaround to me.
Remember there are segments which are quite long, jumping forth and back might make video loading even slower.

Let me know what you think

@lodwhet
Copy link
Contributor Author

lodwhet commented Jan 26, 2023

Yes, it would probably be better to only jump back to the previous auto-skipped segment. I just fixed this.

Disabling Auto Skip for the video would require the user to know in advance whether they want to watch the sponsor segments or not, otherwise they would have to press the keyboard shortcut and then go back to the segment manually. Sometimes we can start watching a video without thinking about sponsors, and when FreeTube auto-skips a sponsor segment, we'd still like to watch this skipped segment. This has happened to me several times, which is why I've implemented this keyboard shortcut.

Obviously I can also add a keyboard shortcut to disable Auto Skip temporarily but perhaps a visual feedback should be added.

auto-merge was automatically disabled January 26, 2023 16:45

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 26, 2023 16:45
@PikachuEXE
Copy link
Collaborator

Sometimes we can start watching a video without thinking about sponsors, and when FreeTube auto-skips a sponsor segment, we'd still like to watch this skipped segment.

I need some examples to understand this

@lodwhet
Copy link
Contributor Author

lodwhet commented Jan 27, 2023

Sometimes we can start watching a video without thinking about sponsors, and when FreeTube auto-skips a sponsor segment, we'd still like to watch this skipped segment.

I need some examples to understand this

For example, you can start watching a video immersively, not wondering if you want to watch sponsor segments or not, or even without thinking about sponsors at all. And when FreeTube skips the first sponsor segment, you notice there's one and you might want to watch it. In this situation, I find the keyboard shortcut relevant.

But maybe I'm the only one in this situation. If you really don't want it, I can remove the keyboard shortcut.

@PikachuEXE
Copy link
Collaborator

I am certain I wouldn't want this new keyboard shortcut to be b

Existing shortcuts: https://docs.freetubeapp.io/usage/keyboard-shortcuts/

\ (Backslash)?

@ChunkyProgrammer
Copy link
Member

ChunkyProgrammer commented Jan 28, 2023

Personally, I think that the keyboard shortcut shouldn't be included until #251 is implemented but if the other reviewers agree with the shortcut then i'm okay with it being implemented

@lodwhet
Copy link
Contributor Author

lodwhet commented Jan 28, 2023

I am certain I wouldn't want this new keyboard shortcut to be b

Existing shortcuts: https://docs.freetubeapp.io/usage/keyboard-shortcuts/

\ (Backslash)?

Yes I think Backslash or Backspace are better shortcuts.

Personally, I think that the keyboard shortcut shouldn't be included until #251 is implemented but if the other reviewers agree with the shortcut then i'm okay with it being implemented

OK, should I open a different PR for the keyboard shortcut?

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

Personally, I think that the keyboard shortcut shouldn't be included until #251 is implemented

I agree.

auto-merge was automatically disabled January 28, 2023 17:56

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 28, 2023 17:56
@lodwhet
Copy link
Contributor Author

lodwhet commented Jan 28, 2023

I just removed the keyboard shortcut.

@FreeTubeBot FreeTubeBot merged commit c7a9906 into FreeTubeApp:development Feb 5, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Feb 5, 2023
@ChunkyProgrammer
Copy link
Member

Thanks for opening this PR 😄

@ApurvR20
Copy link

Hi, does this feature still work? While watching on firefox using sponsorblock, I use the enter key to unskip the segment to check whether the segment is worth my time, and enter again if it isn't. Often sponsors are useless and generic, and something I wouldn't ever spend money on. Some implementation like this would be extremely helpful.

@absidue
Copy link
Member

absidue commented Aug 17, 2023

@ApurvR20 If you are using a nightly build you can seek back into the sponsor and watch it, e.g. with the arrow keys or clicking on the seek bar with a mouse.

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.

7 participants