Add live update of color-related options in Settings > Codec, enforce valid values #5204
Conversation
iina/PrefCodecViewController.swift
Outdated
| @IBAction func toneMappingTargetPeakAction(_ sender: NSTextField) { | ||
| let newValue = sender.integerValue | ||
| // constrain to valid mpv values | ||
| let validValue = newValue == 0 ? 0 : newValue.clamped(to: 10...10000) |
There was a problem hiding this comment.
Merely constraining the value is not the convention used when the user enters invalid values for other settings. Trying entering -1. Other settings pop-up an alert box.
There was a problem hiding this comment.
I kinda loathe the built-in modal popup. It's a really old paradigm, it's obtrusive, and not even very helpful. It doesn't tell the user what the valid values actually are, and if they choose the OK button the field is allowed to be left blank, which leaves them guessing about the current value. At least the clamped approach chooses the closest valid value, and doesn't even require reading.
Also...I actually don't know how to make the built-in popup work for 0 because it's outside the contiguous range of 10-10,000. As far as I can tell, the popup is provided via the text field cell's NumberFormatter, which checks against accepted min and max only. I don't see any places to hook into or override it to provide more complex validation.
There was a problem hiding this comment.
Sorry, I didn't mean to imply we needed to use the alert provided by AppKit. I was expecting we would use Utility.showAlert to show our own alert with the appropriate information about what values are allowed.
I agree, the one provided by NumberFormatter can't be used due to the lack of a contiguous range. And yes, I too dislike the lack of info in the alert NumberFormatter provides.
There was a problem hiding this comment.
@svobs Still waiting for an alert to be added.
There was a problem hiding this comment.
I pushed an update which uses an alert instead. I tried to mimic Apple's alert mostly, but it has only an OK button and resets the field to the current (valid) prefs value.
6274213 to
cbd0c0c
Compare
| @@ -245,6 +245,8 @@ | |||
| "alert.error_saving_file" = "Error occurred when saving %@: %@"; | |||
| "alert.error_deleting_file" = "Cannot delete the file."; | |||
|
|
|||
| "alert.target_peak.bad_value" = "The value \"%@\" is invalid.\nPlease provide a value betwen 10 and 10000, or 0."; | |||
There was a problem hiding this comment.
Between. Not sure why we have a typos CI job if it can’t catch these…
There was a problem hiding this comment.
I think that only checks for typos in code and comments
… valid values (iina#5204) * Add live update of ICC profile, tone mapping, target peak, algorithm options in Settings > Codec * Display alert when Target Peak is invalid instead of auto correcting
Description:
PlayerWindowControllerand reloads them when changed, so the user doesn't have to quit & reopen IINA to guarantee they take effect.VideoView.refreshEdrModeso it doesn't cancel other changes ifcolorspaceis already set, so the above can take effect.PrefCodecViewControllerto restrictTarget peakuser entry to valid values, instead of silently failing & using400.The affected settings:
