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

Eliminating a race condition with SponsorBlock markers #2493

Merged

Conversation

MarmadileManteater
Copy link
Contributor


Eliminating a race condition with SponsorBlock markers

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

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue
closes #2492

Description
This modifies the SponsorBlock markers to use the lengthSeconds already provided by the API to calculate their size and position instead of using this.player.duration(). this.player.duration() is 0 when the view loads and only obtains a value some time after. lengthSeconds should never be 0 unless the API is giving a strange response. Because of this, changing this.player.duration() to lengthSeconds should eliminate the race condition where this.player.duration() is 0 at the time when addSponsorBlockMarker is called.

Screenshots
(These are with Invidious API selected as primary)
Before:
image
After:
image

Desktop (please complete the following information):

  • OS: Windows 10
  • OS Version: Pro Version 21H2 Installed on ‎4/‎3/‎2022 OS build 19044.1889 Experience Windows Feature Experience Pack 120.2212.4180.0
  • FreeTube version: 0.17.1

the seconds length value provided by the API
rather than trying to pull the time from the video
element. This is related to FreeTubeApp#2492 and
#9.
@PrestonN PrestonN enabled auto-merge (squash) August 17, 2022 20:11
@ChunkyProgrammer
Copy link
Member

ChunkyProgrammer commented Aug 18, 2022

Tested using a live video, a video with sponsors and a video without sponsors and it worked fine for me (tested with both invidious and local api)

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.

Tested locally but not sure where to find a live video to test

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Aug 19, 2022
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

I was only able to review the code as it looks like the SponsorBlock API is ratelimiting me.

@PrestonN PrestonN merged commit 166fe00 into FreeTubeApp:development Aug 20, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Aug 20, 2022
Comment on lines +573 to +576
}).catch((error) => {
this.showToast({
message: error.message
})
Copy link
Member

Choose a reason for hiding this comment

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

@MarmadileManteater This just shows and empty toast, I don't think that error object has a message property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. This was a mistake on my part.

@absidue
Copy link
Member

absidue commented Aug 20, 2022

@MarmadileManteater Could you please create a follow up PR that fixes the empty toast. If the SponsorBlock API doesn't have any segments for a video it 404s. The better option would be to show a toast that says something along the lines of no sponsorblock segments or even better don't show anything at all if no SponsorBlock segments are available.

The black oval is the empty toast message.
empty_toast

MarmadileManteater added a commit to MarmadileManteater/FreeTubeAndroid that referenced this pull request Aug 21, 2022
This was introduced by my last pull FreeTubeApp#2493
@MarmadileManteater MarmadileManteater mentioned this pull request Aug 21, 2022
4 tasks
@MarmadileManteater
Copy link
Contributor Author

I made a PR to remove the toast. It is just something I whipped up, but as stated, it would make more sense to opt for a solution which involves making the toast message meaningful.

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 segments don't always display in the timeline when using the Invidious API
6 participants