This is not the only mess caused by trying to add a feature on top of mpv. IINA needs to resist adding features that really belong in mpv. But we have no choice but to now try and make these features that have already been added work properly.

Is using the menu the right approach? I'm not liking that it seems like we are now deviating from Apple's standard menu key binding concepts. We already have a concept of binding a key to an "IINA command". Should these keys be bound to an IINA cycle pause command? I'm still thinking on this…

Is using the menu the right approach? I'm not liking that it seems like we are now deviating from Apple's standard menu key binding concepts. We already have a concept of binding a key to an "IINA command". Should these keys be bound to an IINA cycle pause command? I'm still thinking on this…

Not exactly sure what you mean by this? I definitely admit the fix in this PR is pretty hacky looking. The problem is that most of the action callbacks in MenuController and MainWindowMenuActions are currently written as requiring an NSMenuItem, e.g:

  @objc func menuToggleFullScreen(_ sender: NSMenuItem) {
    toggleWindowFullScreen()
  }

So I ended up creating "dummy" NSMenuItem objects so that they could be used as selectors, though they are never attached to a menu. A cleaner fix might involve going through and changing all the params in the method signatures to AnyObject if possible, and storing a slimmed down data structure instead - or even a lambda function which calls the action with the proper params.

Although this bug affected all menu items, it looks like "pause/resume" was the only one in which a difference was noticed, probably because it is the only one which deviates from mpv's behavior when executing IINA code compared to the undecorated mpv command.

Actually I just found another case which both helps further illustrate the problem and expands the scope of this defect. Take a look at screenshot from PlayerCore:

  func screenshot() {
    guard let vid = info.vid, vid > 0 else { return }
    let saveToFile = Preference.bool(for: .screenshotSaveToFile)
    let saveToClipboard = Preference.bool(for: .screenshotCopyToClipboard)
    guard saveToFile || saveToClipboard else { return }

    let option = Preference.bool(for: .screenshotIncludeSubtitle) ? "subtitles" : "video"

    mpv.asyncCommand(.screenshot, args: [option], replyUserdata: MPVController.UserData.screenshot)
  }

The first key mapping for screenshot will show a preview image because it fires the menu action, but other key mappings will not because they go directly to mpv. The reason this happens won't be clear to the user. Also I might argue that it is "more ok" for IINA to replace mpv's behavior entirely for something like screenshot since it only expands on the feature.

I also revisited your ab-loop feature, and saw that you successfully avoided the binding problem by calling abLoop() in two places: in the menu item action in MainMenuActions, and by checking for its key sequence in handleKeyBinding(). This works ok for commands which have no/simple arguments and are easy to match (and as long as future developers remember to add to both places). But some commands are matched to IINA actions in more subtle ways -- specifically the algorithm in sameKeyAction() in MenuController -- so it would seem that continuing on that route would require duplicating or linking to its logic.

On this:

So I ended up creating "dummy" NSMenuItem objects

And this:

changing all the params in the method signatures to AnyObject

To me these are indications that this is the wrong approach. The Menu* classes should follow normal Apple conventions. The menu system should be layered on top. Other subsystems should not route through the menu system.

On this:

I also revisited your ab-loop feature, and saw that you successfully avoided the binding problem by calling abLoop() in two places: in the menu item action in MainMenuActions, and by checking for its key sequence in handleKeyBinding().

The code in handleKeyBinding was already there. The change was to call a method in PlayerWindowController instead of PlayerCore.

I would expect that menu processing, key press processing and button clicking all might call into some sort of common command processing.

Yes, the screenshot code seems to be another case where IINA has enhanced mpv's functionality in a way that has created an "IINA command". The error when it comes to key binding is not that all keys that are bound to "screenshot" are not triggering the menu item. The error is that the key is bound to the mpv screenshot command. We want it to trigger the "enhanced" IINA screenshot command. We already have a concept of binding a key to an IINA command, @iina video-panel is one example. Trouble is that users are not going to understand the need to bind to something like @iina screenshot. That is probably the reason for the code in handleKeyBinding that specifically looks for MPVCommand.abLoop.rawValue. It is turning the mpv command into an IINA command. I think that is the approach that is required.

On MenuController and sameKeyAction. Possibly some of the recognition and action code in the menu classes will need to be refactored out into classes that can be shared. In other words most menu actions should be a simple binding to actual processing in other classes.

I will see if I can get the attention of other developers as they may disagree with me on this.

On MenuController and sameKeyAction. Possibly some of the recognition and action code in the menu classes will need to be refactored out into classes that can be shared. In other words most menu actions should be a simple binding to actual processing in other classes.

There is a great need for refactoring this mechanism. But since I've made some attempts in the past which have been ignored (specifically two different approaches to #3732), I'm now striving for minimal impact on existing code.

Some design speculation:

Maybe a cleaner way to design it would be to:

1. Replace all the menu item action functions with a single general-use action, called by all, and after matching the KeyMapping's action string with an NSMenuItem, just set its menuItem.representedObject to the relevant KeyMapping object itself. Then the action function will just send the mapping's action string to mpv. Then:
2. All the action strings sent to mpv from any source in IINA would be parsed with the same matching logic to see if they qualify for calling the IINA code instead.

But while this is great in principle,

1. There aren't really any places where a raw mpv action string is sent, other than by key bindings. Most (all?) of the code in the mpv controller uses the libmpv APIs.
2. The above approach still results in parsing the mpv command string twice, which would be nice to avoid.

Second approach:

By attaching a flag + an action reference to the matched KeyMapping, after the first match, this flag could then be checked when the user's key press is matched to a KeyMapping. That's more or less the mechanism I settled on here. Unfortunately it also requires packaging up the whole action function along with its arguments so they can be called directly at time of key press. But at least with this approach, all the real logic is done at the time of key binding. So the code in handleKeyBinding won't need to be updated when commands are added or modified.

I'm trying to get the attention of other developers to get their thoughts on this.

I will have a look after finishing the plugin system (one PR remaining).

I noticed the new plugin PR. Aren't PRs 4423, 4403, 4319 about the plugin system?
There is also an open PR in the iina-plugin-definition repository.

@low-batt yes, definitely will handle them before releasing 1.4.0 beta. I meant on my side I only have one remaining PR to submit.

Sorry for my extremely late reply.

I don't think modifying the KeyMapping class is a good idea because its purpose is not only connecting menu items, so it's better to keep it generic.

How about this: create a hidden submenu somewhere (e.g. File - Other actions from key bindings), and when encountering a duplicated key binding, clone the menu item with its action and add it to this hidden submenu. In this way, these additional key bindings are even searchable from the Help menu, and we are not introducing extra complexity when handling events.

How about this: create a hidden submenu somewhere (e.g. File - Other actions from key bindings), and when encountering a duplicated key binding, clone the menu item with its action and add it to this hidden submenu. In this way, these additional key bindings are even searchable from the Help menu, and we are not introducing extra complexity when handling events.

Definitely a more elegant solution since it will always be using NSMenuItems. But will need to check whether the menu items are always given higher priority than the window controller in the responder chain - if I recall correctly, the window controller is first responder for single-key presses. We may need to remove the KeyMappings from the list which turn out to be NSMenuItems so that there are truly no duplicates which could result in one getting resolved before the other.

Will dig in to this soon.

According to the documentation, the order is key equivalents → keyboard interface control → keyboard action (i.e keyDown) → text insertion. So a menu item with key equivalent should handle the event before the keyDown in the window. The only thing having higher priority than a menu item would be a focused button with key equivalent, which, in this case, should be honored, anyway.

I tested locally, and a second key binding (S) for screenshot worked. Of course we need more testing.

How about this: create a hidden submenu somewhere (e.g. File - Other actions from key bindings), and when encountering a duplicated key binding, clone the menu item with its action and add it to this hidden submenu.

Implemented! See latest push. And yeah, looks like priority for single key bindings was not a problem. This seems to be the cleanest solution yet.

To test that this fixes issue #4582 I changed my key bindings to be mpv Default and in General checked Include subtitles in the Screenshots section.

I then played a video with screenshots, paused on a frame with subtitles and tried taking screenshots by typing both s and S. Only one brought up the screenshot preview and included subtitles.

That issue does not seem to be fixed.

To test that this fixes issue #4582 I changed my key bindings to be mpv Default and in General checked Include subtitles in the Screenshots section.

I then played a video with screenshots, paused on a frame with subtitles and tried taking screenshots by typing both s and S. Only one brought up the screenshot preview and included subtitles.

@low-batt the "mpv Default" config has different commands bound to s and S:

s screenshot
S screenshot video

It looks like the S version simply doesn't qualify for binding to a menu item. The mpv manual supports this decision:

screenshot <flags>
Take a screenshot.

Multiple flags are available (some can be combined with +):

<subtitles> (default)
Save the video image, in its original resolution, and with subtitles. Some video outputs may still include the OSD in the output under certain circumstances.
<video>
Like subtitles, but typically without OSD or subtitles. The exact behavior depends on the selected video output.

You need to use a custom config file, and create two separate mappings for screenshot, for a proper test.

With regard to #4582, I tried the no-arg screenshot command with Include subtitles checked and with unchecked, and it seems to work properly for both cases and multiple bindings.

Issue 4582 uncovered multiple problems. We could get this PR merged and create another PR to address the remaining problems.

The 4 screenshot related key bindings in mpv Defaults are:

The s key is now working. But S should bring up the screenshot preview OSD. That is not happing. Yes, it can't be mapped to a menu item. But that just means it must be fixed in another way.

I believe the mapping for Ctrl+s must be removed. The command screenshot window apparently is tied to mpv's window and is failing when executed from IINA.

The Alt+s binding also fails to bring up the OSD. If possible it should bring up a preview of the last screenshot taken.

The s key is now working. But S should bring up the screenshot preview OSD. That is not happing. Yes, it can't be mapped to a menu item. But that just means it must be fixed in another way.

I'm not clear on what you're arguing. It sounds like there are multiple ideas:

1. So the following should also trigger the mpv OSD: screenshot subtitles, screenshot video, and...screenshot each-frame. What about screenshot-to-file <filename> subtitles|video?
2. Assuming that each of the previous triggers the IINA menu item...what does that mean for their behavior? The command screenshot video is defined as not capturing subtitles. Are you suggesting that it should be overridden by the IINA pref Include subtitles if it is checked? I would argue that it should not.
3. The command screenshot window should be removed from mpv Default. No argument there but that definitely belongs in a separate PR.

On this:

Assuming that each of the previous triggers the IINA menu item...what does that mean for their behavior?

No? These don't really match any menu items. I was expecting this would require a special case in the handleKeyBinding method of PlayerWindowController as is done for abloop. Instead of sending the command directly to mpv it would call something like PlayerCore.screenshot. Either that method would have to allow the argument to be passed in, or a new method could be used.

I don't think we have anything bound to screenshot-to-file? I'd hold of adding support for that one for now.

Agree, IINA setting is not applicable to screenshot video. That one should never include subtitles.

Let's merge this first and move the remaining screenshot problems to another issue.