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

Fixes to Subtitle Chooser layout #4825

Merged
merged 1 commit into from May 25, 2024
Merged

Conversation

svobs
Copy link
Contributor

@svobs svobs commented Feb 17, 2024


Description:

  • Allow expansion tooltips (as suggested in this comment)
  • Set a preferred width of 480 to that it doesn't collapse to size of osdLabel. Uses a priority of 499 so that it will still shrink if there is not enough space.
  • Fixes vertical text alignment for each table item's left & right labels
  • Fix layout warning

…rred width of 480 to that it doesn't collapse to size of OSD title, fix row item text alignment, fix layout warning
@low-batt
Copy link
Contributor

I'm pretty sure @lhc70000 is currently preparing changes in this area as well. The online subtitle providers are being transitioned from the built-in implementations to plug-ins. I believe that involves enhancing the user interface as well.

@svobs
Copy link
Contributor Author

svobs commented Feb 18, 2024

I'm pretty sure @lhc70000 is currently preparing changes in this area as well. The online subtitle providers are being transitioned from the built-in implementations to plug-ins. I believe that involves enhancing the user interface as well.

Ah, that is good to know. I got carried away and already had more extensive changes prepared. I will hold off on those.

Copy link
Contributor

@low-batt low-batt left a comment

Choose a reason for hiding this comment

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

Pulled PR, built IINA and tested searching for subtitles online. Made the window tiny and confirmed tool tips worked. Looks good to me.

@low-batt
Copy link
Contributor

Will have to see what @lhc70000 thinks, but I'm thinking these changes can go into the bug fix release with the improvements @lhc70000 is working on going into the feature release.

This change possibly calls into question my decision, namely this comment:

As the spacing is rather tight I rounded the fps up.

When I was working on that issue I was expecting we would not be altering the OSD for the bug fix release. If we proceed with this change, should we switch the fps to be a floating point number with two fractional digits?

@svobs
Copy link
Contributor Author

svobs commented Feb 19, 2024

If we proceed with this change, should we switch the fps to be a floating point number with two fractional digits?

I haven't paid much attention to subtitles stuff until now. I'm not sure how helpful or accurate the "fps" field is. But seems like the 2 decimals should be important given its use by traditional standards like 23.98, 29.98, etc.

I guess I'll try posting a new PR with all the fixes I made (which includes that) and see what people think.

@low-batt
Copy link
Contributor

I will keep an eye out for the new PR.

The fps field is helpful because if the fps of the video used when authoring the subtitles does not match the fps of the video you are trying to obtain subtitles for then the time codes within the subtitles may not precisely match the video which can cause the subtitles to drift out of synchronization with the video. The accuracy of the subtitle fps I believe depends upon the author correctly reporting that value, so it could be suspect.

@uiryuu uiryuu merged commit 0cb7154 into iina:develop May 25, 2024
1 check passed
@uiryuu
Copy link
Member

uiryuu commented May 25, 2024

Thanks!

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.

The Find Subtitle Result Window was too Narrow to see the whole name.
3 participants