Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple keymaps in :map argument #1051

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fishyfriend
Copy link
Contributor

This is a reimplementation of #830, released as #1019 but reverted due to lacking copyright assignment. The copyright issue should now be solved as I have signed the Emacs copyright papers.

Sorry for the trouble of requesting a second review. I felt a rewrite was warranted due to discovered loose ends as well as readability.

Noteworthy changes:

  • :map nil, which currently causes an error, is now treated as synonymous with :map global-map.

  • nil, global-map, and override-global-map are explicitly disallowed as arguments for :prefix-map or :repeat-map. Existing checks here and here may have been intended for this purpose (perhaps mistakenly substituting map for (cadr args)?). These checks had simply been removed in the original PR as I couldn't make sense of them.

  • A redundant check related to repeat maps has been removed

Original description from #830

This PR augments bind-keys to support passing a list of keymaps as the :map argument.

When one wants to bind the same key/command pair in multiple keymaps, the current options have some drawbacks:

;; Option 1: Code duplication, problematic when duplicated keybindings are numerous
(use-package foopkg
   :bind (:map bar-mode-map
          ("C-c x" . foocmd1)
          ("C-c y" . foocmd2)
          :map baz-mode-map
          ("C-c x" . foocmd1)
          ("C-c y" . foocmd2)))

;; Option 2: Use a loop, sacrificing the convenience and readability of :bind
(use-package foopkg
  :commands (foocmd1 foocmd2)
  :init
  (dolist (map (list bar-mode-map baz-mode-map))
    (bind-keys :map map
               ("C-c x" . foocmd1)
               ("C-c y" . foocmd2))))

With the new way, it is possible to avoid duplication and still use :bind:

(use-package foopkg
  :bind (:map (bar-mode-map baz-mode-map)
        ("C-c x" . foocmd1)
        ("C-c y" . foocmd2)))

Additionally this PR clarifies the documentation of bind-chords to reflect that this style is already available for that command.


Thanks,
Jacob

This updates the bind-keys functions to accept either a symbol or a
list as argument for the `:map' keyword, with additional related
fixes:

    (1) Handle the keymap name `nil' as a synonym for `global-map';

    (2) Fail if an invalid argument is specified for `:prefix-map' or
    `:repeat-map' keywords.
@benthamite
Copy link

benthamite commented Mar 11, 2024

@jwiegley Would it be possible to merge this pull request? It seems like you were ready to merge the previous version and the only obstacle was the copyright assignment legal requirement, which is now addressed. Thanks.

@fishyfriend
Copy link
Contributor Author

Hi, original submitter here, responding to @benthamite's comment. IMO a fresh review is needed before merging. Although this PR is not a huge set of changes, the part of bind-key.el involved is pretty complex, the functional changes are not totally identical to the original PR (see description), and the revised PR touches code that moved after the original version was reverted.

@benthamite
Copy link

@fishyfriend, oh, I see. Thanks for clarifying, and please disregard my previous comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants