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.

Sign up for GitHub

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

Jump to bottom

add keybinding to control subtitles offset #5193

Merged
merged 2 commits into from
Aug 16, 2024
Merged

add keybinding to control subtitles offset #5193

merged 2 commits into from
Aug 16, 2024

Conversation

carlo-colombo
Copy link
Contributor

@carlo-colombo carlo-colombo commented Feb 9, 2024 •
edited
Loading

Changes
Bounding g and h to control subtitles offset in 0.1 steps, bindings are taken from VLC.
The subtitle offset overlay is shown while the offset is increased or decreased by pressing the keys. Pressing the keys while the offset is not shown still changes the offset.

Issues

@carlo-colombo carlo-colombo requested a review from a team as a code owner February 9, 2024 21:41
@carlo-colombo carlo-colombo force-pushed the subtitle-offset-keybinding branch 2 times, most recently from 54eb554 to dd0b496 Compare February 10, 2024 07:34
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Feb 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

1hitsong
1hitsong approved these changes May 25, 2024
View reviewed changes
Copy link
Member

@1hitsong 1hitsong left a comment

Choose a reason for hiding this comment

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

Tested in Waterfox on Windows 11.

Works great

@carlo-colombo
Copy link
Contributor Author

I updated the PR, thank you so much for the support. Is there anything else that I can do for the PR, as updating documentation?

Thanks

github-actions[bot]
github-actions bot previously requested changes Jun 9, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

src/components/subtitlesync/subtitlesync.js 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
src/controllers/playback/video/index.js Outdated Show resolved Hide resolved
src/controllers/playback/video/index.js Outdated Show resolved Hide resolved
@carlo-colombo carlo-colombo force-pushed the subtitle-offset-keybinding branch 2 times, most recently from 1ccfdf7 to d116013 Compare June 9, 2024 10:25
@dmitrylyzo dmitrylyzo added feature New feature or request ui & ux This PR or issue mainly concerns UI & UX labels Jun 9, 2024
dmitrylyzo
dmitrylyzo reviewed Jun 9, 2024
View reviewed changes
src/controllers/playback/video/index.js Outdated Show resolved Hide resolved
src/controllers/playback/video/index.js 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
src/components/subtitlesync/subtitlesync.js Outdated Show resolved Hide resolved
@dmitrylyzo
Copy link
Contributor

dmitrylyzo commented Jun 9, 2024 •
edited
Loading

This should be tested for cases when there're no subtitles and subtitles are off.

UPD:
Seems to work (with my suggestions). Except that it prints No available track, cannot apply offset to the console when there are no subtitles or they are off.

@carlo-colombo
Copy link
Contributor Author

Updated the PR following the suggestion, but I wonder if the conditional chaining is supported in the older browser that has been mentioned earlier in the discussion (WebKit engines (Tizen 2.x, webOS 1/2).)

@dmitrylyzo
Copy link
Contributor

but I wonder if the conditional chaining is supported in the older browser that has been mentioned earlier in the discussion (WebKit engines (Tizen 2.x, webOS 1/2).)

We use Babel to transpile code so that it is supported on older platforms.

@carlo-colombo carlo-colombo force-pushed the subtitle-offset-keybinding branch 3 times, most recently from da77704 to f9c249b Compare June 14, 2024 19:50
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jun 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@carlo-colombo
Copy link
Contributor Author

Hi, is there anything else that need to be addressed before merging the PR?

@dmitrylyzo dmitrylyzo added this to the v10.10.0 milestone Jul 16, 2024
@thornbill @dmitrylyzo
Add uppercase variants
92e5d69 
Co-authored-by: Dmitry Lyzo <[email protected]>
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Aug 16, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@jellyfin-bot
Copy link
Collaborator

Cloudflare Pages deployment

Latest commit 92e5d692ea92c060f31b87ec36d27ec485ac9eeb
Status ✅ Deployed!
Preview URL https://04ab8ba4.jellyfin-web.pages.dev
Type 🔀 Preview

@thornbill thornbill merged commit a00c68d into jellyfin:master Aug 16, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitrylyzo dmitrylyzo dmitrylyzo approved these changes

@thornbill thornbill thornbill left review comments

@1hitsong 1hitsong 1hitsong approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

Assignees
No one assigned
Labels
feature New feature or request ui & ux This PR or issue mainly concerns UI & UX
Projects
Jellyfin 10.10
Status: Done
Milestone
v10.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@carlo-colombo @dmitrylyzo @jellyfin-bot @1hitsong @thornbill