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

Fix consecutive sponsor segments not being skipped. #4886

Closed
wants to merge 7 commits into from

Conversation

RyanHx
Copy link

@RyanHx RyanHx commented Apr 6, 2024

Fix consecutive sponsor segments not being skipped

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #3534

Description

Changed the implementation of sponsors auto skip. It instead relies on the mousedown event on the player's seekbar to detect manual seeking via user input, meaning sponsor segments can now be safely skipped consecutively. I also added a listener for the keyboard hotkeys used for seeking, to prevent auto skipping when moving into a sponsored segment that way. These event listeners are disposed of alongside the Player dispose event as I didn't want them left in the global scope (document).

Testing

I performed testing on a video with sponsored segments. I tested seeking on the player via clicking, clicking and dragging, and keyboard hotkeys. Currently there are no ramifications I have found.

Desktop

  • OS: Windows 10 Pro
  • OS Version: 22H2
  • FreeTube version: 0.20.0

Additional context

The keycode package used to pick up the keyboard hotkeys already exists within the FreeTube project via video.js, I didn't add it myself for the purpose of this bugfix.

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 6, 2024 04:00
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 6, 2024
@absidue
Copy link
Member

absidue commented Apr 6, 2024

The keycode package used to pick up the keyboard hotkeys already exists within the FreeTube project via video.js, I didn't add it myself for the purpose of this bugfix.

Regardless of whether a dependency already exists in node_modules, if you use it in a project, you should always explicitly declare it in that project's dependencies. If you don't declare it and one day video.js stops using the keycode package, everything will suddenly break and nobody will know why.

That being said the rest of FreeTube, even in the same file that you edited, manages completely fine without the keycode package, so I would prefer if you didn't add an extra dependency at all and just did it the native way.

auto-merge was automatically disabled April 6, 2024 11:39

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 6, 2024 11:39
auto-merge was automatically disabled April 6, 2024 11:41

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 6, 2024 11:41
@RyanHx
Copy link
Author

RyanHx commented Apr 6, 2024

I also have a change pending to swap out the for loop in ignoreSponsorBlock to a forEach, but am hesitant to push it without testing. Unfortunately the SponsorBlock API is down at the moment so no segments can be retrieved. I've been looking around for an API mirror but no luck.

https://status.sponsor.ajay.app/
https://fosstodon.org/@sponsorblock/112224416738802796

auto-merge was automatically disabled April 7, 2024 10:36

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 7, 2024 10:36
auto-merge was automatically disabled April 7, 2024 12:16

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) April 7, 2024 12:16
@PikachuEXE
Copy link
Collaborator

Found a video with consecutive sponsor segments
https://youtu.be/NhPdXaTF67U

@RyanHx
Copy link
Author

RyanHx commented Apr 8, 2024

Ah sorry yeah I was testing using this one https://youtu.be/5ghgTIBzuRA - it has an intro -> recap/preview -> sponsor at the start.

There's also some extra examples in the linked issue #3534

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Apr 11, 2024

Hi @RyanHx Thankyou for addressing this issue. Unfortunately i have to close this PR because there is a Player migration coming up. The part you addressed has/will be refactored in the player migration.

We hope this wont discourage you to look into other issues as you did a wonderful job on this one :)

auto-merge was automatically disabled April 11, 2024 08:41

Pull request was closed

@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 11, 2024
@RyanHx
Copy link
Author

RyanHx commented Apr 11, 2024

@efb4f5ff-1298-471a-8973-3d47447115dc I look forward to the new player, but is it not worth just getting this fix through in the meantime? It's a 1 year old bug that I often run into myself 😭

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

I understand and share your frustration as I am the one that opened that bug report but it isnt worth it to fix and then refactor your code again

@RyanHx
Copy link
Author

RyanHx commented Apr 11, 2024

I figured if it's already in the works it'd just require a rebase onto this, but fair enough.

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.

[Bug]: SponsorBlock segment isnt skipped
5 participants