-
Notifications
You must be signed in to change notification settings - Fork 167
feat: Align local video/subtitle split by source aspect ratio on drop and theater toggle #912
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
Open
khajiitvaper2017
wants to merge
17
commits into
killergerbah:main
Choose a base branch
from
khajiitvaper2017:fix-aspect-ratio-v2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
ff5082e
set player's width to align with aspect ratio of video
khajiitvaper2017 07ca265
fixed video pane
khajiitvaper2017 68f951d
fix side panel
khajiitvaper2017 746ce6b
change aspect ratio when toggling theater mode
khajiitvaper2017 8e257f1
excess code
khajiitvaper2017 de4a8e2
auto scroll on toggling theater mode
khajiitvaper2017 4a0b4d2
initialize appliedInitialWidthKeyRef with undefined
khajiitvaper2017 63ee626
added Video Subtitle Split Behavior setting in Misc Settings
khajiitvaper2017 9c556d8
Merge remote-tracking branch 'upstream/main' into fix-aspect-ratio-v2
khajiitvaper2017 944818c
merge-loc-from-en
khajiitvaper2017 e92b2ff
move to playback-preferences
khajiitvaper2017 dd48eda
simplification
khajiitvaper2017 8e6fe2f
prettier
khajiitvaper2017 827497f
undefined
khajiitvaper2017 8c58a63
move useVideoAspectRatio to video-subtitle-split
khajiitvaper2017 0195128
fix video player overflows
khajiitvaper2017 cff61d0
simplify
khajiitvaper2017 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { VideoSubtitleSplitBehavior } from '@project/common/settings'; | ||
| import { clampSubtitlePlayerWidth, resolveVideoSubtitleSplitLayout } from './video-subtitle-split'; | ||
|
|
||
| it('uses the saved split width in remember mode', () => { | ||
| expect( | ||
| resolveVideoSubtitleSplitLayout({ | ||
| behavior: VideoSubtitleSplitBehavior.rememberSplitPosition, | ||
| persistedWidth: 420, | ||
| autoWidth: 300, | ||
| }) | ||
| ).toBe(420); | ||
| }); | ||
|
|
||
| it('leaves the width unchanged in remember mode when nothing has been saved', () => { | ||
| expect( | ||
| resolveVideoSubtitleSplitLayout({ | ||
| behavior: VideoSubtitleSplitBehavior.rememberSplitPosition, | ||
| autoWidth: 300, | ||
| }) | ||
| ).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('ignores saved width in auto-maximize mode', () => { | ||
| expect( | ||
| resolveVideoSubtitleSplitLayout({ | ||
| behavior: VideoSubtitleSplitBehavior.autoMaximizeVideo, | ||
| persistedWidth: 420, | ||
| autoWidth: 300, | ||
| }) | ||
| ).toBe(300); | ||
| }); | ||
|
|
||
| it('clamps split width to the current bounds', () => { | ||
| expect(clampSubtitlePlayerWidth(150, 200, 500)).toBe(200); | ||
| expect(clampSubtitlePlayerWidth(600, 200, 500)).toBe(500); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { useEffect, useState } from 'react'; | ||
| import { VideoSubtitleSplitBehavior } from '../../settings'; | ||
|
|
||
| interface ResolveVideoSubtitleSplitLayoutArgs { | ||
| behavior: VideoSubtitleSplitBehavior; | ||
| persistedWidth?: number; | ||
| autoWidth?: number; | ||
| } | ||
|
|
||
| export function clampSubtitlePlayerWidth(width: number, minWidth: number, maxWidth: number) { | ||
| return Math.min(maxWidth, Math.max(minWidth, width)); | ||
| } | ||
|
|
||
| export function resolveVideoSubtitleSplitLayout({ | ||
| behavior, | ||
| persistedWidth, | ||
| autoWidth, | ||
| }: ResolveVideoSubtitleSplitLayoutArgs) { | ||
| if (behavior === VideoSubtitleSplitBehavior.rememberSplitPosition) { | ||
| return persistedWidth; | ||
| } | ||
|
|
||
| return autoWidth; | ||
| } | ||
|
|
||
| export function useVideoAspectRatio(videoFileUrl: string | undefined, enabled: boolean) { | ||
| const [videoAspectRatio, setVideoAspectRatio] = useState<number>(); | ||
|
|
||
| useEffect(() => { | ||
| setVideoAspectRatio(undefined); | ||
|
|
||
| if (!enabled || !videoFileUrl) { | ||
| return; | ||
| } | ||
|
|
||
| const video = document.createElement('video'); | ||
| video.preload = 'metadata'; | ||
| video.onloadedmetadata = () => { | ||
| setVideoAspectRatio(video.videoWidth / video.videoHeight); | ||
| }; | ||
| video.src = videoFileUrl; | ||
|
|
||
| return () => { | ||
| video.onloadedmetadata = null; | ||
| video.removeAttribute('src'); | ||
| video.load(); | ||
| }; | ||
| }, [videoFileUrl, enabled]); | ||
|
|
||
| return videoAspectRatio; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this added to
subtitles.css?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When theater mode was toggled back on, the iframe document was ending up a bit taller than the viewport, so the browser showed a scrollbar. This problem is also present on live app, This fixes it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though it's mostly visible when using
yarn run start, when usingpreview, it's rareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept only one
overflow: hidden;, should be enough to fix this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be addressed in a separate change then? It doesn't seem critical to fix, if it is only dev-facing. Also, I'm not completely convinced it's the right fix, if it's really the iframe that's taller then setting overflow to
hiddenwould just hide the part of the iframe that's overflowing.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, feel free to remove it, just didn't seem to be worthy of separate pull request. In my first post's video you can see scrollbars appearing for a split second when toggling theater mode, that's what I am addressing. On live version this problem also exists but layout changes faster than scrollbar can appear and it jerks a little instead.
2026-03-23.20-42-49.mp4
I tried to log it and for some reason, htmlScrollHeight is taller than viewport:
bodyClientHeight = 847
bodyScrollHeight = 847
htmlClientHeight = 847
htmlScrollHeight = 865
If you can find anything better than just slapping
overflow: hidden;, that would be great.