jwiegley / use-package

A use-package declaration for simplifying your .emacs
https://jwiegley.github.io/use-package
GNU General Public License v3.0
4.37k stars 260 forks source link

Allow multiple keymaps in :map argument #1051

Open fishyfriend opened 1 year ago

fishyfriend commented 1 year ago

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:

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

benthamite commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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