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

Fix caption sizes not being changed #11806

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Thompson3142
Copy link
Contributor

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

The caption size setting now correctly applies to the main player. I have tested this on several (emulated) devices, but additional test are probably needed to ensure it works correctly on all devices.
If I overlooked any aspects of the original implementation, I’m happy to adjust the code as needed.

Restarting the app is no longer required - only restarting the player is necessary. If this PR is accepted, the related strings in the settings should be updated to reflect this change.

Fixes the following issue(s)

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.

Due diligence

@github-actions github-actions bot added the size/small PRs with less than 50 changed lines label Dec 20, 2024
@AudricV AudricV added bug Issue is related to a bug GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background) and removed size/small PRs with less than 50 changed lines labels Dec 20, 2024
@Profpatsch
Copy link
Contributor

Looks like there is another place where this is set, in the Popup player:

protected void setupSubtitleView(final float captionScale) {
final float captionRatio = (captionScale - 1.0f) / 5.0f + 1.0f;
binding.subtitleView.setFractionalTextSize(
SubtitleView.DEFAULT_TEXT_SIZE_FRACTION * captionRatio);
}

This will use the exact subtitle sizes the user requested, both for
the main and the popup player. They will always be the same fraction
of the video, even if the popup player is resized.
@github-actions github-actions bot added the size/small PRs with less than 50 changed lines label Jan 21, 2025
@Profpatsch
Copy link
Contributor

@Thompson3142 I adjusted the commit to include the Popup player, please take a look.

@Thompson3142
Copy link
Contributor Author

Doesn't that make the subtitles size in the Popup player extremly large? I doubt that anybody actually reads subtitles in the Popup player anyway but they should probably be smaller if somebody wants to read them.
On the other side I also don't get what final float captionRatio = (captionScale - 1.0f) / 5.0f + 1.0f; is exactly trying to achieve so if you have checked that the subtitles are readable its gtg.

@Profpatsch
Copy link
Contributor

Doesn't that make the subtitles size in the Popup player extremly large?

It didn’t look like it. They had the same size as in the main player, and would shrink according to the resize. Although maybe a point could be made that they should stay at least the same size if you shrink the video, so you can still read them.

@Thompson3142
Copy link
Contributor Author

You could spend months optimizing this for every possible screen size and caption size, but it’s not worth it. I’ve reviewed it myself, and the caption size in the popup seems appropriate (at least it is what I would expect it to be). I’m happy with this as is, making it a little more simple is definetly an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background) size/small PRs with less than 50 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing caption size doesn't work
4 participants