This appears to be a regression via the code which was committed to fix #3692 (4abeb5b). Storing the key in an unsorted dictionary did remove the duplicates, but caused the ordering to be lost.

Thanks for entering this issue! Not sure this is a regression?

Likely somewhere a Swift Dictionary is being used resulting in random ordering. If a Dictionary style mapping is required you might have to make use of OrderedDictionary.

The fix should follow mpv conventions for handling keys that have multiple different assignments in the input.conf file. I believe the file is treated as ordered and the first definition wins. Check that.

So in this case IINA should always pick for the menu item the key that first appears in the conf file for the associated command.

The fix should follow mpv conventions for handling keys that have multiple different assignments in the input.conf file. I believe the file is treated as ordered and the first definition wins. Check that.

So in this case IINA should always pick for the menu item the key that first appears in the conf file for the associated command.

This issue is a bit tricky. Let me try to formalize it.
Binding := Key -> Action

Rules for setting the list of bindings:

1. If there are multiple bindings with the same key, all but the last is discarded.
2. For each menu item, go over the remaining bindings, in order, and choose the first binding whose action is "equivalent" to the menu item action.
3. All remaining bindings (i.e., those not chosen for menu key equivalents), become window actions and can be triggered when a player window is in focus.

I'll work on creating some automated tests to verify and confirm this behavior going forward.

I think the fix for this also needs to be run past @lhc70000 and @saagarjha.

I was thinking that as mpv's convention is "first wins" (did you confirm that?) for key binding, it would be odd if IINA adopted a "last wins" for menu binding. Not what happens to menu assignment in this case:

p stop
p cycle pause

To be consistent I guess the second definition needs to be ignored and the menu item would not have a binding if there isn't another key that is assigned to cycle pause.

As the input configuration files are external it is not possible to insure duplicates do not exist. And as mpv accepts files with duplicates and just ignores additional key bindings that is the behavior IINA must accept.

However, I'm thinking the Key Bindings preference must be updated to provide some feedback about this. Maybe red highlighting of duplicate bindings that will be ignored? Warning when you try and add a duplicate? Something to help the users with this. As it is you can add a duplicate and wonder why the binding is being ignored.

I was thinking that as mpv's convention is "first wins" (did you confirm that?) for key binding, it would be odd if IINA adopted a "last wins" for menu binding.

I think you switched that around - last definition wins. But it does make sense to just keep with the "last wins" theme for menu items since it will make things easier to remember. I'll see about changing it.

As for confirmation of "last wins" - strangely, I could not find anything in the mpv manual which documents this exact behavior. But I've been tracing through the code and I think this is where it happens:

// Note: all the comments are mine.
// This function sets a single binding (key sequence -> command).
//
// The "builtin" flag indicates that we are setting one of the "weak default" bindings, which applies
// to all the lines in input.conf after  `default-bindings start`. IINA doesn't parse this, so every call
// that applies to us will have builtin==false.
static void bind_keys(struct input_ctx *ictx, bool builtin, bstr section,
                      const int *keys, int num_keys, bstr command,
                      const char *loc, const char *desc)
{
    // Gets the "input section" associated with the section name.
    // IINA doesn't support named sections coming from conf files, so this will always be "default".
    // (IINA does allow Lua scripts to introduce other input sections but doesn't know about them - WIP)
    struct cmd_bind_section *bs = get_bind_section(ictx, section);
    struct cmd_bind *bind = NULL;

    assert(num_keys <= MP_MAX_KEY_DOWN);

    // This checks all the previously set bindings in the "default" section for key sequence.
    // `bind` is a pointer, so this grabs the previous value if it exists.
    // `b->is_builtin == builtin` always resolves to `false == false`
    for (int n = 0; n < bs->num_binds; n++) {
        struct cmd_bind *b = &bs->binds[n];
        if (bind_matches_key(b, num_keys, keys) && b->is_builtin == builtin) {
            bind = b;
            break;
        }
    }

    // No previous value? Then allocate memory for a new entry.
    if (!bind) {
        struct cmd_bind empty = {{0}};
        MP_TARRAY_APPEND(bs, bs->binds, bs->num_binds, empty);
        bind = &bs->binds[bs->num_binds - 1];
    }

    // Deallocate existing command, loc, desc fields, if they exist
    bind_dealloc(bind);

    // Overwrite fields and duplicate the command, loc, desc fields
    *bind = (struct cmd_bind) {
        .cmd = bstrdup0(bs->binds, command),
        .location = talloc_strdup(bs->binds, loc),
        .desc = talloc_strdup(bs->binds, desc),
        .owner = bs,
        .is_builtin = builtin,
        .num_keys = num_keys,
    };
    memcpy(bind->keys, keys, num_keys * sizeof(bind->keys[0]));
    if (mp_msg_test(ictx->log, MSGL_DEBUG)) {
        char *s = mp_input_get_key_combo_name(keys, num_keys);
        MP_TRACE(ictx, "add: section='%s' key='%s'%s cmd='%s' location='%s'\n",
                 bind->owner->section, s, bind->is_builtin ? " builtin" : "",
                 bind->cmd, bind->location);
        talloc_free(s);
    }
}

However, I'm thinking the Key Bindings preference must be updated to provide some feedback about this. Maybe red highlighting of duplicate bindings that will be ignored? Warning when you try and add a duplicate? Something to help the users with this. As it is you can add a duplicate and wonder why the binding is being ignored.

Definitely some kind of visual feedback should be added! I think I'd personally favor the ignored duplicates being colored in gray, or maybe have a warning, rather than a bright red error. My input.conf files are littered with duplicates because, knowing mpv's behavior, I tend to use the space above as a sort of scratch pad or Lego bin for stuff I might want to use or play with. Although I suppose if IINA supported comments, I'd gladly just use commented-out commands instead.

I may very well have messed up on what mpv does. I usually take the time to check everything before commenting, but I'm crazy busy this week.

Possibly definitions that are being ignored should be in the standard dim color used for disabled items. Users are used to what that means. We'd still want them clickable so you can edit them, which would be a little odd, but probably ok. On the panel that allows you to edit a binding we could have a caution ICON and an explanation that this binding is not being used as it is overridden by another one. Something like that maybe?

I was going to say, I don't know how you manage to keep up with all these posts! I'm still grappling with how best to deal with the format. I think I can forgive you for giving an issue like this a lower priority than something like a new bug which has just come in.

OK, I think I understand what you mean - for a typical UI app's keybindings it might be more standard to use alert icons...at the same time, what isn't standard is to for an app to make a well-defined choice when that happens. In my experience, either the app automatically unsets any previous bindings, or disables both until the conflict is resolved... OK, second attempt... I agree on the caution icon + explanation in the Edit Binding window, and in the Key Bindings preference pane itself, each ignored duplicate should have a small yellow caution ICON with a tooltip which includes that same explanation, and color the text in its row differently: not a "disabled" shade of gray, but lighter than black, to give it a more "ignored" feel.

I probably should file a new issue based on this UI discussion...

When doing keybindings there's typically two special cases to consider. One is when you assign the same item multiple shortcuts, as it the issue here. The other is when you have the same shortcut used for two different items. The latter is usually a hard error that prevents either item from functioning, and I think IINA should probably do the same here and throw up an "error" (taking care to ensure the two items are genuinely conflicting, rather than present in two different modes).

For the former, things are more complicated. Having multiple shortcuts for the same action is not very common across Mac apps, but it's not unheard of either. I think the behavior we want here is that both keyboard shortcuts to work, which leaves the question of what the menu item should say. For most apps this would be the "preferred" shortcut, which is programmer-defined in advance. For us we don't really have a way to say which one should be shown in the menu. I think we should pick one of the sane options ({first, last} in {order of appearance in configuration file, sorted order}) and ship that.

The latter is usually a hard error that prevents either item from functioning, and I think IINA should probably do the same here and throw up an "error" (taking care to ensure the two items are genuinely conflicting, rather than present in two different modes).

@saagarjha there's an extra wrinkle that arises, in that IINA is trying to support mpv behavior, where possible, and there seem to be a good amount of users who either want to use their existing mpv configuration or even switch back and forth between mpv & IIINA. And mpv has the convention that duplicates are not a "hard error", but that the last one always wins. So we have been trying to decide how best to convey that. I was suggesting more of a warning than an error. If we block the user from creating a duplicate, we might end up getting in the way of power users or mpv fans who are relying on mpv's behavior. Does that sound reasonable?

For the other issue (multiple qualifying menu item bindings) it sounds like there is consensus, at least up to the final detail about whether to choose the last qualifying binding or the first. Just throwing this out there - how about the last? Just to continue the pattern.