-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
{stats,osc}.lua: respect --osd-scale-by-window
by default
#14158
Conversation
I implemented this too but was waiting for the console one to get merged. Personally I would just remove vidscale unless someone can think of a use case of having each script scale differently, especially since the name of this option is wrong, they scale with the window and not with the video, which can be between black bars. |
Download the artifacts for this pull request: |
This is kept for compatibility reason. If someone has already set Also since |
FWIW the OSC behavior will change by default anyway if we're going to default to |
Changing the default of I don't see any value in removing this option from users if the mechanism isn't broken or doesn't become a significant maintenance burden. |
You could also add |
32c139c
to
0a15a94
Compare
Added |
Since @Dudemanguy had strong opinion about this PR, I will let you guys decide what is the path forward. I personally think that scaling make sense, if you resize window often, so that OSC doesn't cover half of the video with small sizes. But as long as there is an option to control that, it will be ok. |
Hmm no, I'm fine with this. It doesn't change current behavior. |
I refereed to #14114 which guido mentioned. This all seems like contented changes. |
My general feeling on the matter is that scaling the scripts that are primarily text (e.g. Anyways, this PR doesn't change the status quo so no objections here. |
I hear you, but there is a range of readability. Also if user makes player window small (and video) it means they can see it, presumably looking at the player up close. If the user make it big the situation is reversed. I mean scaling the text (and video) makes it usable for both close watching on small screen and far watching on big screen. Without scaling, both those cases would need different font size, manually managed by user. I think it makes little sense in practice to force it and I don't think users are complaining about it and imho it would be jarring change. |
This recommends querying the value of this option when drawing UI elements. This allows a greater level of consistency by using a single flag which already controls the scaling behavior of the OSD to control the behavior of all scripts. Also fix a capitalization nearby.
This adds auto to vidscale script option, which lets the scale be inherited from OSD --osd-scale-by-window option.
This adds auto to vidscale script option, which lets the scale be inherited from OSD --osd-scale-by-window option.
This lets these scripts scale the elements with OSD by default.
Allows the scale mode to be changed at runtime if vidscale is set to auto.
Allows the scale mode to be changed at runtime if vidscale is set to auto.
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.
LGTM. Please rebase.
For the record Kasper and I agreed to keep the current defaults for the reasons he stated and for consistency with VLC and MPC-HC (I thought only mpv scaled text, but those do too, though only subtitles for MPC-HC, and only mpv scales the UI). |
This lets these scripts scale the elements with OSD by default. The
vidscale
option now accepts and defaults toauto
value which enables this behavior.