general-define-key :predicate broken?

[stepnem]

This doesn't seem to work (the nil binding has unconditional effect):

(general-define-key :keymaps 'normal
                    :predicate 'magit-blame-read-only-mode
                    "q" nil)

This works as expected:

(general-define-key :keymaps 'normal
                    "q" (general-predicate-dispatch 'evil-record-macro
                          magit-blame-read-only-mode nil))

Tested with current master.

[noctuid]

This is the expected behavior. In the first keybinding, you are replacing the default evil-record-macro keybinding. :predicate is only useful if the fallback keybinding is defined in another lower precedence keymap. general-predicate-dispatch has to be used for cases like this.

[stepnem]

Thanks. I didn't really understand the "Note that with :predicate, you can still only have a key bound once in a single keymap." sentence in the README, now it makes sense.

[noctuid]

I've (hopefully) improved the documentation. Let me know if you have any suggestions to make it more clear.

[stepnem]

Thanks, now that I (hopefully) have a better understanding of how :predicate works the explanations do seem quite clear. I think what's most counter-intuitive and conducive to misunderstanding is the fact that :predicate actually pertains to the whole keymap, not just one binding, IOW that one modifies the keymap unconditionally; conditional is only the key lookup, i.e. keymap precedence. And I know the information is _there_: you even mention it twice explicitly: "condition under which a map should be active". Yet the "Hey, it's :predicate, so obviously the key will continue to work as before, only when the condition is true the new binding will take effect" seems like an intuition easy to fall for, as I did. I guess the Vim map(ping) ~= Emacs bind(ing) dissonance might have been at work here, too ("a map should be active" ... uh huh!). But yeah, it's more that I feel stupid/careless now than that I think the documentation should be improved. Maybe if something like "the keymap is modified unconditionally, so the new binding will be in effect whenever the keymap is active" would be added, it could be another push toward the correct mental model.