Skip to content

feat: Align local video/subtitle split by source aspect ratio on drop and theater toggle#912

Open
khajiitvaper2017 wants to merge 17 commits intokillergerbah:mainfrom
khajiitvaper2017:fix-aspect-ratio-v2
Open

feat: Align local video/subtitle split by source aspect ratio on drop and theater toggle#912
khajiitvaper2017 wants to merge 17 commits intokillergerbah:mainfrom
khajiitvaper2017:fix-aspect-ratio-v2

Conversation

@khajiitvaper2017
Copy link
Copy Markdown
Contributor

This PR improves local playback layout when dropping a video + subtitle file.

On drop, the app reads source video metadata and initializes the split so the video pane matches the video aspect ratio as closely as possible, maximizing visible video area and reducing black side bars.

The same one-time recalculation also runs when Toggle Theater Mode changes layout (app bar hidden/shown), so split initialization stays correct after theater mode transitions.

After initialization, manual subtitle panel resizing remains user-controlled.

2026-03-01.23-09-06.mp4

@killergerbah
Copy link
Copy Markdown
Owner

Thanks for doing this but it's not really obvious to me that this behavior would benefit everyone. I think some users would like the subtitle list to be a certain size and for the app to remember that. I'm open to alternate solutions if you can think of any.

@khajiitvaper2017
Copy link
Copy Markdown
Contributor Author

Thanks for doing this but it's not really obvious to me that this behavior would benefit everyone. I think some users would like the subtitle list to be a certain size and for the app to remember that. I'm open to alternate solutions if you can think of any.

Add setting like "Video-Subtitle Split Behavior" with options: "Remember my split position" or "Auto-maximize video"?

@killergerbah
Copy link
Copy Markdown
Owner

Add setting like "Video-Subtitle Split Behavior" with options: "Remember my split position" or "Auto-maximize video"?

I see, yeah a setting feels like the natural way to do this. My preference would be to default to "Remember my split position".

Only other alternative I can think of is to add a new button to the video player or subtitle list but I don't know if there's a suitable icon for the button and where it makes sense to put it.

@khajiitvaper2017
Copy link
Copy Markdown
Contributor Author

Add setting like "Video-Subtitle Split Behavior" with options: "Remember my split position" or "Auto-maximize video"?

I see, yeah a setting feels like the natural way to do this. My preference would be to default to "Remember my split position".

Added the setting.

@khajiitvaper2017
Copy link
Copy Markdown
Contributor Author

fixed scrollbars appearing on video player when toggling theater mode

@@ -1,3 +1,18 @@
html,
body {
Copy link
Copy Markdown
Owner

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?

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

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 using preview, it's rare

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Owner

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 hidden would just hide the part of the iframe that's overflowing.

Copy link
Copy Markdown
Contributor Author

@khajiitvaper2017 khajiitvaper2017 Mar 23, 2026

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.

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.

2 participants