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

evil-collection: respect evil-overriding-maps #506

Closed
wants to merge 1 commit into from
Closed

Conversation

condy0919
Copy link
Collaborator

Respect evil-overriding-maps.

If dictionary-mode-map is in evil-overriding-maps, h/l/n/p will not
be overridden by evil. Currently evil-collection overrides theses keys, see
https://github.com/emacs-evil/evil-collection/blob/master/modes/dictionary/evil-collection-dictionary.el#L42-L45

If evil-overriding-maps is '() (default now), h, l, n and p will be defined by evil as these are the basic keys. In such a case, evil-collection needn't to redefine. It will help us to maintain less keybindings.

@condy0919 condy0919 requested a review from jojojames July 17, 2021 01:20
@jojojames
Copy link
Collaborator

Hmnn, I'm very hesitant to support respecting this evil-override-map. IIRC, ideally we'd want to get away from override maps in the first place.

Secondly, I'm not sure all the implications of each comment here:

;; `overriding' is non-nil, and cdr of `overriding' is nil, which means evil
;; shouldn't define any keybindings in that keymap.

What if evil-collection wanted to define a,b,c,d,e,f but the overriding map was nil? Why would overriding be non nil but the cdr of overriding be nil? When would that happen?

;; `overriding' is non-nil, and cdr of `overriding' is non-nil, which means
;; evil shouldn't define keybindings in that keymap in such state.

What if evil-collection wanted to define a,b,c,d,e,f but the overriding map was a,b,c? What would happen to the d,e,f?

@condy0919
Copy link
Collaborator Author

Hmnn, I'm very hesitant to support respecting this evil-override-map. IIRC, ideally we'd want to get away from override maps in the first place.

Secondly, I'm not sure all the implications of each comment here:

;; `overriding' is non-nil, and cdr of `overriding' is nil, which means evil
;; shouldn't define any keybindings in that keymap.

What if evil-collection wanted to define a,b,c,d,e,f but the overriding map was nil? Why would overriding be non nil but the cdr of overriding be nil? When would that happen?

The cdr of overriding is a evil state whose evil bindings should be overridden
by the original keymap. When it's nil, it means all evil state bindings will be
overriden. e.g.

(evil-collection-define-key 'normal 'Info-mode-map
  "h" 'evil-backward-char)

But h will be bound to Info-history due to override.

;; `overriding' is non-nil, and cdr of `overriding' is non-nil, which means
;; evil shouldn't define keybindings in that keymap in such state.

What if evil-collection wanted to define a,b,c,d,e,f but the overriding map was a,b,c? What would happen to the d,e,f?

evil-collection will skip a,b,c and evil-collection-define-key only in d,e,f.
It's easier to implement by just skipping instead of overriding by the original
keymap.

@jojojames
Copy link
Collaborator

But h will be bound to Info-history due to override.

Could you elaborate more here?

What do you expect to happen before this change and what do you expect to happen after this change, and why?

evil-collection will skip a,b,c and evil-collection-define-key only in d,e,f.
It's easier to implement by just skipping instead of overriding by the original
keymap.

I'm not sure that's what we want.

@condy0919
Copy link
Collaborator Author

condy0919 commented Jul 18, 2021

But h will be bound to Info-history due to override.

Could you elaborate more here?

What do you expect to happen before this change and what do you expect to happen after this change, and why?

evil-collection will skip a,b,c and evil-collection-define-key only in d,e,f.
It's easier to implement by just skipping instead of overriding by the original
keymap.

I'm not sure that's what we want.

Oh, I found the example is wrong. All modes should have hjkl, e.g. h is
evil-backward-char in normal state.

But if Info-mode-map is in evil-overriding-maps and the cdr is nil, h will
be Info-history at last due to the override mechanism.

So we must manually use evil-collection-define-key to redefine h to evil-backward-char in some keymaps (e.g. dictionary-mode-map).

But is it user expected? If a keymap is in evil-overriding-maps, it means
users would like to use the vanilla keybindings.

So that we try to respect the evil-overriding-maps. Currently, the value of
evil-overriding-maps is '(), so that this PR won't create issues for most people.

After this patch, we can delete tons of redundant keybdings.

If dictionary-mode-map isn't in evil-overriding-maps,

(evil-collection-define-key 'normal 'dictionary-mode-map
  ;; motion
  (kbd "l") 'evil-forward-char  ; otherwise bound to `dictionary-previous'
  (kbd "h") 'evil-backward-char ; otherwise bound to `dictionary-help'
  (kbd "p") 'ignore             ; otherwise bound to `backward-button'
  (kbd "n") 'evil-search-next   ; otherwise bound to `forward-button'
  ;; mouse
  [mouse-1] 'link-selected
  ;; misc
  "g?" 'dictionary-help ; normally under `h` which is rebound here
  (kbd "C-o") 'dictionary-previous) ; normally under `l` which is rebound here

l/h/p/n can be removed safely.

The diff in #501 also reflects the benefit.

@jojojames
Copy link
Collaborator

But if Info-mode-map is in evil-overriding-maps and the cdr is nil, h will
be Info-history at last due to the override mechanism.

Yes, we want 'h' to be 'evil-backward-char'.

So we must manually use evil-collection-define-key to redefine h to evil-backward-char in some keymaps (e.g. dictionary-mode-map).

That's expected and wanted. In this case, the reasoning is 'this mode binds 'h' to something random, lets change it back to what a user would expect'.

But is it user expected? If a keymap is in evil-overriding-maps, it means
users would like to use the vanilla keybindings.

That's not necessarily true. It could be that another mode (say evil-vars) is binding it for them without their intention.

After this patch, we can delete tons of redundant keybdings.

If I'm understanding what you're saying, this isn't true. We'd be removing bindings -and also- changing what those bindings do.

Just going by your example, all the 'redundant' binds of hjkl that we've added to various modes will become what evil-overriding-map has in its map right? In this case, the binds in evil-collection serves a purpose of rebinding to hjkl.

The diff in #501 also reflects the benefit.

From my quick skim through there, I'd say the problems lie in evil using those override maps for those various modes.

Somewhat related: #48

@condy0919
Copy link
Collaborator Author

condy0919 commented Jul 19, 2021

From my quick skim through there, I'd say the problems lie in evil using those override maps for those various modes.

Do you mean using evil-overriding-maps is not recommended? If so, we can
suggest users set evil-overriding-maps to '() as evil-want-keybinding.

@jojojames
Copy link
Collaborator

Do you mean using evil-overriding-maps is not recommended?

Yes.

If so, we can
suggest users set evil-overriding-maps to '() as evil-want-keybinding.

Maybe that's not a bad idea, let me think on this further.

@jojojames
Copy link
Collaborator

I think it may be worth looking at where evil uses evil-overriding-maps and see if we can maybe PR something where -evil-want-binding- gates those maps from being set. That's already what we're doing with the evil-keybindings file in evil.

e.g. https://github.com/emacs-evil/evil/blob/5c28294d830a5a79e9b9da2c32e7675d52d76720/evil.el#L141

@condy0919
Copy link
Collaborator Author

evil-overriding-maps defaults to '() now, changed since emacs-evil/evil#1494

@jojojames
Copy link
Collaborator

Is there something to be done here @condy0919 ? I think we don't want to support this (evil-collection respecting evil-overriding-maps)?

@condy0919
Copy link
Collaborator Author

Is there something to be done here @condy0919 ? I think we don't want to support this (evil-collection respecting evil-overriding-maps)?

Yes.

But we could prompt a diagnostic message to encourage user to set evil-overriding-maps to nil (it's now the default) to reduce redundant keybindings (e.g. h/j/k/l, w/b/W/E).

(unless evil-overriding-maps
  (message "Make sure to set `evil-overriding-maps' to nil before loading evil \
or evil-collection."))

@jojojames
Copy link
Collaborator

It's probably OK to not prompt them, maybe a documentation change is all we need.

If we do prompt them, we'd also need a way to disable the prompt for the users already aware but don't care for the message.

@condy0919
Copy link
Collaborator Author

It's probably OK to not prompt them, maybe a documentation change is all we need.

If we do prompt them, we'd also need a way to disable the prompt for the users already aware but don't care for the message.

Ok. I will raise an another PR later.

@condy0919 condy0919 closed this Aug 30, 2021
@condy0919 condy0919 deleted the overriding branch August 30, 2021 23:27
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