evil-collection: respect evil-overriding-maps

[condy0919]

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.

[jojojames]

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]

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]

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]

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]

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]

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]

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]

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]

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

[jojojames]

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

[condy0919]

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]

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]

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.