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 #830

Closed
wants to merge 2 commits into from

Conversation

fishyfriend
Copy link
Contributor

@fishyfriend fishyfriend commented Apr 15, 2020

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.

@jwiegley
Copy link
Owner

Conflicts need resolving, please.

(put #'cmd2 'repeat-map 'rm)
(bind-key "y" #'cmd2 rm nil))))

(ert-deftest bind-key/:repeat-map-3 ()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expansion in this test is not equivalent to the prior test as might be expected. The repeat-map property is applied to bound commands even after an intervening :map directive. This seems fishy to me but is existing behavior (the tests both pass independently of the changes in this PR). I'll leave this alone for now, but happy to open an issue if the behavior is indeed incorrect.

((eq :prefix-docstring (car args))
(setq prefix-doc (cadr args)))
((and (eq :prefix-map (car args))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed this condition (not (memq map '(global-map override-global-map))) and the same condition again below. I do not understand their purpose, but informal testing leads me to believe these are safe to remove. E.g., if I specify :map override-global-map with :prefix-map or :repeat-map, the behavior is the same as if these checks were not in place.

@fishyfriend
Copy link
Contributor Author

Hi @jwiegley, thanks. I've revised this and resolved the conflicts. I made one assumption that could use a second-guess, please see this comment. Other than that, it should be good to go. Cheers, Jacob. (cc: @Hugo-Heagren)

@jwiegley
Copy link
Owner

I was unable to update this PR directly, so I've opened another PR with the same changes @fishyfriend.

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.

None yet

2 participants