Expand IINA screenshot functionality to screenshot flags #4853
Conversation
svobs
changed the title
Summary: • Applies IINA screenshot OSD & other enhancements to screenshot commands which have flags (except for "each-frame" flag) • Expand functionality of PlayerCore.screenshot() to handle screenshot command from a key binding • Add logic so that if any flags are found in the screenshot command, they will be used • Else if no flags are specified, then the value for pref key "screenshotIncludeSubtitle" will be used to determine the flags
svobs
force-pushed
the
pr-screenshot-opts-integration
branch
from
594faf2
to
1c72e06
Compare
|
I'm busy on Tuesdays, so I only had a chance to read through the description and peek at the code. Seems like the right approach. I will take a closer look soon. |
| /// 1. As noted above, if the stored values for `Preference.Key.screenshotSaveToFile` and `Preference.Key.screenshotCopyToClipboard` are | ||
| /// set to false, all screenshot commands will be ignored. | ||
| /// 2. When no flags are given with `screenshot`: instead of defaulting to `subtitles` as mpv does, IINA will use the value for | ||
| /// `Preference.Key.screenshotIncludeSubtitle` to decide between `subtitles` or `video`. |
There was a problem hiding this comment.
The documentation comment is very helpful!
If you highlight the function name and then under the Xcode Editor menu open the Structure sub-menu and click on Add Documentation then Xcode will add a template with the appropriate documentation tags. In the case of screenshot method it will add:
/// <#Description#>
/// - Parameter keyBinding: <#keyBinding description#>
/// - Returns: <#description#>
iina/PlayerCore.swift
Outdated
| @discardableResult | ||
| func screenshot(fromKeyBinding keyBinding: KeyMapping? = nil) -> Bool { | ||
| guard isScreenshotEnabled() else { | ||
| Logger.log("Ignoring screenshot request\(keyBinding == nil ? "" : " from key binding") because all forms of screenshots are disabled in prefs") |
There was a problem hiding this comment.
This log message is not entirely correct. If you disable the screenshots settings and then press s key the log message will not indicate the request was triggered "from key binding".
iina/PlayerCore.swift
Outdated
| } | ||
| } | ||
|
|
||
| guard let vid = info.vid, vid > 0 else { return false } // TODO: why this is needed? |
There was a problem hiding this comment.
Always good to emit a log message when ignoring input from the user like was done at the start of the method indicating screenshots are disabled. If that had been done in the past for this statement not only would it be possible to tell what happened from the log file, it also would have made obvious what this code is for. I suggest changing this code to something like this:
let fromKey = keyBinding == nil ? "" : " from key binding"
guard let vid = info.vid, vid > 0 else {
Logger.log("Ignoring screenshot request\(fromKey) because no video stream is being played")
return false
}And moving it right to the start of the method.
I believe the thinking was that it is better to ignore the request than interrupt the user with the alert that says "Cannot take a screenshot".
To trigger this play a mp3 and press the s key.
That makes sense. I pushed another commit with the suggestions, although I removed the talk about key bindings from the error messages (weren't really useful) but added a couple extra lines of logging to facilitate future troubleshooting. |
There was a problem hiding this comment.
Tested and looks good.
I think we should prevent Preference.Key.screenshotSaveToFile and Preference.Key.screenshotCopyToClipboard from both being false. I will consider making a PR to make these 2 options radio buttons so that only one of them can be selected. And also combining these 2 preference keys into 1 so that the user cannot set them both to false in via commands either.
Description:
Current functionality
IINA's
Playback>Take a Screenshotmenu item (I'll call it "IINA's screenshot") utilizes mpv'sscreenshotcommand (i.e. "mpv's screenshot") as its backbone, but differs in the following ways:screenshotIncludeSubtitlepref (bool) to decide betweensubtitlesorvideoflags to send toscreenshot.If a key binding's is mapped to
screenshotwith no flags, then it is matched to the menu item, and so it will call IINA's screenshot. However, if any flags are supplied (e.g.,screenshot subtitles), it will not match and will end up calling mpv's screenshot.PR functional changes
PlayerCore.screenshot()to take an optional key binding argument, and adds a call to it fromPlayerWindowController.handleKeyBindingso it can handlescreenshotactions.screenshotIncludeSubtitlewill be used to determine the flags.Caveats
each-frameflag. If this flag is present, mpv's screenshot will be used instead (sorry - maybe some future PR).screenshot window, although the feature itself will continue to error out until the fix for Ctrl-s (screenshot window) does not work #4822 is merged.i. If the stored values for
Preference.Key.screenshotSaveToFileandPreference.Key.screenshotCopyToClipboardare set to false, all screenshot commands will be ignored. It may not be obvious to the end user why their screenshot seems to be getting ignored. Although no one seems to have filed any issues about it.ii. When no flags are given with
screenshot, the mpv spec says that default issubtitlesflag. But IINA will use eithersubtitlesorvideo, depending on the value forPreference.Key.screenshotIncludeSubtitle.