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

Change Subtitle Sync slider to go from -300 to 300 #4754

Merged
merged 1 commit into from Sep 7, 2023

Conversation

mkanilsson
Copy link
Contributor

@mkanilsson mkanilsson commented Aug 18, 2023

Changes

This patch changes the subtitle sync from using a precentage to an 'slider value' which ranges from -300 to 300 (ms offset). The reasons for this is that LG WebOS doesn't jump in 0.1 slider increments as specified in the subtitlesync.template.html file but instead jumps in 1.0 increments. That results in Subtitle Sync jumping 0.6s per increment. Using a value from -300 to 300 makes LG WebOS jump 0.1s instead.

There is no noticeable effect on any other platform.

Issues
Fixes #4753

@mkanilsson mkanilsson requested a review from a team as a code owner August 18, 2023 18:56
@mkanilsson mkanilsson changed the title Change Subtitle Sync slider go from 0-600 Change Subtitle Sync slider to go from 0 to 600 Aug 18, 2023
@mkanilsson mkanilsson changed the title Change Subtitle Sync slider to go from 0 to 600 Change Subtitle Sync slider to go from -300 to 300 Aug 19, 2023
src/components/subtitlesync/subtitlesync.js Outdated Show resolved Hide resolved
src/components/subtitlesync/subtitlesync.template.html Outdated Show resolved Hide resolved
src/components/subtitlesync/subtitlesync.js Outdated Show resolved Hide resolved
src/components/subtitlesync/subtitlesync.js Outdated Show resolved Hide resolved
@dmitrylyzo dmitrylyzo added bug Something isn't working ui & ux This PR or issue mainly concerns UI & UX labels Aug 23, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

This patch changes the subtitle sync from using a procentage to a
'slider value' that ranges from -300 to 300. The reasons for this is that
WebOS doesn't jump in 0.1 increments but instead jumps 1.0 increments in
the slider, which results in subtitle sync jumping 0.6s per increment.
Using a value from -300 to 300 makes LG WebOS jump 0.1s instead.
Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

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

LGTM + it is good to be moved to release-10.8.z branch.

We still can improve it by making slider value to subtitle offset ratio 1:1 (step=".1" min="-30" max="30" value="0").
Possible options:

Given the above, we can rely on the slider: this becomes redundant and this can be simplified.

#4760 will allow to simplify getBubbleHtml and drop getOffsetFromPercentage.

@thornbill thornbill added stable backport Backport into the next stable release p: webos This PR or issue mainly concerns WebOS clients labels Aug 30, 2023
@thornbill thornbill added this to the v10.9.0 milestone Aug 30, 2023
@thornbill
Copy link
Member

@dmitrylyzo is this PR good to merge or should it wait on the other changes you have mentioned? Thanks!

@dmitrylyzo
Copy link
Contributor

This PR definitely fixes stepping and makes it even.

I didn't want to burden the contributor too much, but it would be better to make the slider in 1:1 ratio - this won't change UI/UX (relative to PR), but makes the code cleaner.

FYI, #4758 fixes stepping (in the slider itself), but it's uneven due to rounding.

As for getBubbleHtml thing, I will do it in my PR later - this is just enhancement anyway.

@thornbill thornbill merged commit b114f1a into jellyfin:master Sep 7, 2023
19 checks passed
@dmitrylyzo dmitrylyzo removed the stable backport Backport into the next stable release label Sep 24, 2023
nyanmisaka added a commit to nyanmisaka/jellyfin-web that referenced this pull request Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p: webos This PR or issue mainly concerns WebOS clients ui & ux This PR or issue mainly concerns UI & UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LG WebOS Subtitle offset jumps by 0.6s
3 participants