-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Improvements to speed UI (fixes #5147, #5168) #5175
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
Conversation
Details: • Add speedResetBtn which will reset speed to 1x • Update osd.speed message to support unlimited digits after decimal • Update customSpeedTextField, speedSliderIndicator to support up to 9 digits after decimal (will round to 9th decimal place) • Increase width of customSpeedTextField to fit at least 6 total digits • Update speed slider action to round to 2 digits after decimal
Thanks for picking this one up! Will have to see what the other developers think about some of the UI details of this… I think mpv is truncating The I like the reset button. Should that have a tool tip? For some reason when I set the language to The thumb of the slider is slightly cut off when it is at the end of the slider. This is an existing problem. Happens with |
Sure. I am not super happy about the text field having to scroll. But it should be uncommon for most cases... I suppose I could move the text field onto another row, which would allow the slider to fill the whole width, which would be nice. But it would look awkward aesthetically if the text field is all alone. Also wanted to highlight that I moved the number formatting out of the localized strings for
When I enter
And then But it is helpful to know that the IPC interface stops at 6 digits. Maybe that's all anyone has ever needed? Maybe Another issue. I also just now discovered there are problems setting very low speeds. It looks like Not sure why this particular error code might show up. And there are no problems when higher values are used (for example,
Mmm yes, forgot about that. Will post an update.
I thought it would be easy enough to guess, since there are buttons like it down below for the equalizers. A tooltip could be added though. Maybe just saying "Reset speed to 1x" or something?
Sounds like it might be the way I am doing the rounding. Will post an update to fix it. Would be great to learn more about Apple's "negative 0", "NaN", "infinity", etc and how it makes those & stores them.
Hmm. My guess is that the slider thumb can be drawn slightly outside the slider's actual bounds. Because it looks similar to an issue which can happen for rounded buttons. Ugly, ugly AppKit... But it only does this for Hebrew? I will see if I can add a workaround. |
1289fdb
to
937dd7f
Compare
I added a commit which changes the
I dug in to this, and it's just due to a mismatch in formatting strings, a result of the
Unable to reproduce. Tried dark & light themes; Hebrew & English; Retina display & regular display; |
i will defer to @uiryuu on the change to how the numbers are localized. When I paste
Why is the log you showed missing The OSD message shows the full value. It is truncated to just The documentation for --speed says:
So the problem with values less than 0.01 is expected mpv behavior. "Reset speed to 1x" is fine for the tool tip. As for whether the tool tip is needed or not, I rather like tool tips, but I don't know how the other developers would feel about one for this button. They may think it is obvious what it does and shouldn't need one. Lets see what they think. The problem with the slider thumb being cut off is not specific to The hidden macOS build number has been there for a long time. When I was adding build information to IINA's about window I copied that convention where build information was not shown until you clicked on it. But following the macOS convention got a thumbs down from the reviewers. |
Regarding the slider knob clipping problem, I found it and fixed it. I was able to reproduce it by changing I also found & fixed another minor regression: a horizontal line in the Audio tab did not go all the way across the sidebar. |
Glad you found the problem with clipping the thumb. I was just about to report that the clipping did not reproduce for me under macOS Sonoma. I tested the latest commits under Ventura and the clipping is fixed. This is looking good. Need to check with @uiryuu as to how we can change the format specifiers in the files for other locals. |
Looks nice, thank you! If there are still unfinished tasks regarding this issue, please open another PR. |
* Improvements to speed UI Details: • Add speedResetBtn which will reset speed to 1x • Update osd.speed message to support unlimited digits after decimal • Update customSpeedTextField, speedSliderIndicator to support up to 9 digits after decimal (will round to 9th decimal place) • Increase width of customSpeedTextField to fit at least 6 total digits • Update speed slider action to round to 2 digits after decimal * Support unlimited decimal places for speed numbers in menus * Use localized formatting of decimals in osd.speed msg & menu speeds * Fix more holes in custom speed values. Localize decimal fmt of speed slider labels * Fix clipping of tick sliders. Fix horizontal line widths * Add tooltip to speedResetBtn * Fix name, update comments based on feedback
speedResetBtn
which will reset speed to 1xosd.speed
message; andmenu.speed
,menu.speed_up
, menu.speed_down` menu item titles to support unlimited digits after decimal and also properly localize numeric values.customSpeedTextField
,speedSliderIndicator
to support up to 6 digits after decimal (will round to 6th decimal place, rounding down if 7th decimal place is a5
, matching mpv's behavior).customSpeedTextField
to fit at least 6 total digitsDescription:
This attempts to address concerns mentioned in #5168, while striking a good balance between usability, versatility, and technical constraints.
It's not clear how many decimal places mpv will support. It looks like it will allow whatever will fit in adouble
. But it's doubtful that mpv has thoroughly tested very very precise speeds.[EDIT] It has been determined that mpv supports 6 digits after the decimal and has a minimum value of 0.01, so this is now enforced when changing the speed.
This PR ensures that the OSD message will display the exact speed mpv is using.
It looks like text fields want a maximum number of decimal places, so thecustomSpeedTextField
in the Video Settings sidebar is now 9 decimals instead of 3. This also means that thespeedSliderIndicator
label above the slider will also be rounded to 9 decimals because it uses the number fromcustomSpeedTextField
, but they do not limit the actual value used by mpv. So key bindings can be used to set speed with any precision.The width of
speedSlider
had to be decreased slightly to expand the width ofcustomSpeedTextField
, which now can comfortably fit 5 digits after the decimal.I also modified the speed slider's action to round the selected speed to 2 digits after the decimal. This should be more usable because having more precision when dragging the slider wouldn't seem to make much sense and would only make the speed value harder to read.
This also expanded to fix number formatting for everything speed-related, so that locales which use commas instead of decimals are supported. There is more work to do for audio delay & subtitle controls, however.