evil-leader -> bind-map #3860

justbur · 2015-11-18T01:03:07Z

Obviously a large change and deserves scrutiny, but everthing is working fine for me. The benefits are

No longer need to hack evil-leader to get it to support multiple leader keys. Also no need for a minor mode to be active for leader keys to work
Centralized control of spacemacs/set-keys and spacemacs/set-keys-for-mode
Ability to use minor-mode bindings
Distinguish maps from where they are bound. All of the major mode maps could be moved off of "m" for example very easily.
describe-keymap can be used on each of the generated maps to easily inspect them, since they are all distinct and easily available.
More complicated schemes become possible, such as having leader maps that span multiple minor and/or major modes. A lisp leader map perhaps
Less code to maintain

TheBB · 2015-11-18T07:02:51Z

layers/+keyboard-layouts/bepo/keybindings.el

+      "C-S-j" nil
+      "C-S-k" nil
+      "C-S-l" nil
+      "tH" nil                          ;


Rogue comment. :-)

TheBB · 2015-11-18T07:11:29Z

Nice work. I don't see any show stoppers. The diff is predictably mostly dull reading.

justbur · 2015-11-18T11:34:34Z

@TheBB fixed. thanks for going through it

syl20bnr · 2015-11-19T04:01:12Z

core/core-keybindings.el

-          (define-key state-map
-            (kbd dotspacemacs-major-mode-emacs-leader-key) major-mode-map))
-        (define-key state-map (kbd dotspacemacs-emacs-leader-key) root-map)))))
+(defalias 'spacemacs/set-keys-for-mode 'spacemacs/set-keys-for-major-mode)


Why this alias ?

Seems like spacemacs/set-keys-for-major-mode is used only once, consider defined directly the function with name spacemacs/set-keys-for-mode (renamed accordingly if we change it, see above comments).

syl20bnr · 2015-11-19T04:15:49Z

Excellent 💯

See my comments.
Also how do we set keys for a minor mode ? Is this the reason behind the defalias and the minor mode variant is missing for now ?

syl20bnr · 2015-11-19T04:18:08Z

Since this PR is huge we have to merge it as soon as possible, ideally we should try to address the review tomorrow so I can merge it during the evening.

I'm going to work on the move of evilification stuff, so you'll be able to rebase on this.

syl20bnr · 2015-11-19T06:00:12Z

I finished to move evilified-state files.

justbur · 2015-11-19T13:11:33Z

Also how do we set keys for a minor mode ? Is this the reason behind the defalias and the minor mode variant is missing for now ?

It was but I'm going to change it to be more explicit

Since this PR is huge we have to merge it as soon as possible, ideally we should try to address the review tomorrow so I can merge it during the evening.

I'm working on it now. Should be done soon

A couple of things :

I'm going to bring back the evil-leader setup temporarily otherwise everyone who uses it to set keys in their personal config will have problems
We might need to adjust the key bindings in some modes after this change. The issue is that the evil-leader map might have been shadowing some minor mode bindings, and since we are now using the global state maps (like evil-normal-state-map) these bindings might now take priority. Before you do this, be aware of this because there might be some reports like this (they are easy to fix though).

StreakyCobra · 2015-11-19T13:17:33Z

I'm going to bring back the evil-leader setup temporarily otherwise everyone who uses it to set keys in their personal config will have problems

@justbur Instead on bringing back evil-leader just for this, can't you provide functions doing the same on top of your new code? It will allow to indicate its depreciation to users with a message and do the transition smoothly.

Edit: wording

syl20bnr · 2015-11-19T13:43:11Z

@StreakyCobra good idea, let's define our own transitional and minimalistic evil-leader package with the same API that redirects to bind-map. I know it is not as trivial as it sounds to emulate evil-leader but I don't see how we can better deal with this big refactoring.

justbur · 2015-11-19T13:44:46Z

@syl20bnr @StreakyCobra it should be perfectly fine for the two packages to coexist. If people want to use evil-leader for some reason, we shouldn't prevent that.

I'd say just issue a deprecation warning for a while and then remove support

StreakyCobra · 2015-11-19T13:48:37Z

@justbur I said this because of the point 2) where you explained there could be problem between the two :-) If it's possible and easier to have both, let's go for this. The depreciation can still be advised.

syl20bnr · 2015-11-19T13:50:24Z

@justbur there won't be any leader key conflict bound to different sub-maps ?

syl20bnr · 2015-11-19T13:55:03Z

Since the evil-leader package is in core space and not user space I would go for an adapter to use only bind-map under the hood.

justbur · 2015-11-19T14:05:50Z

@justbur I said this because of the point 2) where you explained there could be problem between the two

The problem would be if we removed the configuration for evil-leader and people just use evil-leader/set-key. It wouldn't work because the leader key is no longer configured.

@justbur there won't be any leader key conflict bound to different sub-maps ?

Emacs deals with overlaying multiple active maps fine. It's a conflict if the same key sequence is bound to different commands (not prefixes) in different maps, emacs just uses the first one it finds so you have to know the priority rules for maps.

The only issue that may crop up is that we would now be using maps with different priority than the ones that evil-leader uses. The only place this would be a problem I think is if some minor mode uses evil-make-overriding-map to change the priority of its map with respect to evil's default map. I did this in evil-magit and I know dired does this, but there might be some other odd cases here or there. See my last commit for what to do.

I went through and renamed everything and added docstrings as requested. We still need to update the doc files, but I don't have time to do that right now.

TheBB · 2015-11-19T14:09:05Z

But if we define a shim evil-leader/set-key that uses bind-map as a backend, it will work.

justbur · 2015-11-19T14:36:14Z

We could alias them for now
On Thu, Nov 19, 2015 at 9:09 AM Eivind Fonn notifications@github.com
wrote:

But if we define a shim evil-leader/set-key that uses bind-map as a
backend, it will work.

—
Reply to this email directly or view it on GitHub
#3860 (comment).

Set up to use bind-map instead of evil-leader for leader key functionality. Alias evil-leader funcs and remove package

Removes dependence on evil-leader centralizing control over the method of key binding in core-keybindings.el

Adds useful help functions like describe-keymap.

Specifically need to be concerned with modes that make their maps overriding. 1. dired 2. evil-magit

justbur · 2015-11-19T15:07:13Z

Ok they're aliased now.

By the way evil-lisp-state has evil-leader as a dependency

TheBB · 2015-11-21T10:18:34Z

There are some merge conflicts now. Maybe you can address those? I would really like to have this merged today or tomorrow, before going on to the other PRs.

justbur · 2015-11-21T17:08:33Z

@TheBB would you mind doing it then? I'm traveling for most of the day and
probably won't have time.
On Sat, Nov 21, 2015 at 5:18 AM Eivind Fonn notifications@github.com
wrote:

There are some merge conflicts now. Maybe you can address those? I would
really like to have this merged today or tomorrow, before going on to the
other PRs.

—
Reply to this email directly or view it on GitHub
#3860 (comment).

TheBB · 2015-11-21T17:34:58Z

Okay, done. 👍

Everything works here except the minor mode function. I can use it and it sets the keys in the correct map, but they don't become available when the minor mode is activated. We can fix that later though, since it's not used anywhere at the moment.

justbur · 2015-11-21T17:43:44Z

It won't work right away it requires a call to evil-normalize-keymaps which
doesn't happen right away
On Sat, Nov 21, 2015 at 10:35 AM Eivind Fonn notifications@github.com
wrote:

Okay, done. [image: 👍]

Everything works here except the minor mode function. I can use it and it
sets the keys in the correct map, but they don't become available when the
minor mode is activated. We can fix that later though, since it's not used
anywhere at the moment.

—
Reply to this email directly or view it on GitHub
#3860 (comment).

TheBB · 2015-11-21T17:47:13Z

Oh okay. That works but I see now they end up under the major mode keys.

Anyway, we can deal with that later.

justbur · 2015-11-21T18:05:28Z

What do you mean under the major mode keys?
On Sat, Nov 21, 2015 at 10:47 AM Eivind Fonn notifications@github.com
wrote:

Oh okay. That works but I see now they end up under the major mode keys.

Anyway, we can deal with that later.

—
Reply to this email directly or view it on GitHub
#3860 (comment).

justbur · 2015-11-21T18:06:39Z

Oh I see SPC m. Yes that's where I put them but that can easily be changed.
On Sat, Nov 21, 2015 at 11:05 AM Justin Burkett justin@burkett.cc wrote:

What do you mean under the major mode keys?
On Sat, Nov 21, 2015 at 10:47 AM Eivind Fonn notifications@github.com
wrote:

Oh okay. That works but I see now they end up under the major mode keys.

Anyway, we can deal with that later.

—
Reply to this email directly or view it on GitHub
#3860 (comment).

TheBB · 2015-11-21T18:13:46Z

Thanks, this is now merged. Brace yourselves. :-)

syl20bnr · 2015-11-21T20:46:13Z

For this kind of big changes I would like to be able to merge it myself. In this particular case I would have not cherry-picked commits, I would have preferred to merge without fast-forward using the GitHub button to have a merge commit in the history for easy revert if needed.

I still cannot review the now merged changes so I cannot comment anything for now. I hope to be able to do it this evening.

TheBB · 2015-11-21T20:48:46Z

All right 👍

syl20bnr · 2015-11-22T15:27:32Z

Amazing job guys !! 💜
I fixed evil-lisp state, I'll update the configuration when MELPA has processed it.

TheBB reviewed Nov 18, 2015
View reviewed changes

layers/+keyboard-layouts/bepo/keybindings.el

"C-S-j" nil

"C-S-k" nil

"C-S-l" nil

"tH" nil ;

Copy link

Contributor

TheBB Nov 18, 2015

Choose a reason for hiding this comment

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

Rogue comment. :-)

syl20bnr reviewed Nov 19, 2015
View reviewed changes

justbur added 6 commits November 19, 2015 10:00

core: Add new set-key functions to core-keybindings

363b52e

Set up to use bind-map instead of evil-leader for leader key functionality. Alias evil-leader funcs and remove package

layers: Transition to new key bindings functions

9047a86

Removes dependence on evil-leader centralizing control over the method of key binding in core-keybindings.el

doc: Minor doc changes for evil-leader removal

ebad06b

spacemacs-base: Add help-fns+

0265448

Adds useful help functions like describe-keymap.

core: Assume all which-key funcs available now

5290372

layers: Fix leader key in some modes

c34ba93

Specifically need to be concerned with modes that make their maps overriding. 1. dired 2. evil-magit

TheBB added the High Priority label Nov 21, 2015

TheBB closed this Nov 21, 2015

TheBB removed the High Priority label Nov 21, 2015

TheBB mentioned this pull request Nov 21, 2015

Evil lisp state bindings gone #3901

Closed

TheBB mentioned this pull request Nov 22, 2015

Profiling startup times #3911

Closed

StreakyCobra mentioned this pull request Nov 22, 2015

Symbols's function definition is void: bind-map-evil-define-key #3916

Closed

justbur deleted the use-bind-map branch January 30, 2016 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

evil-leader -> bind-map #3860

evil-leader -> bind-map #3860

justbur commented Nov 18, 2015

TheBB Nov 18, 2015

TheBB commented Nov 18, 2015

justbur commented Nov 18, 2015

syl20bnr Nov 19, 2015

syl20bnr Nov 19, 2015

syl20bnr commented Nov 19, 2015

syl20bnr commented Nov 19, 2015

syl20bnr commented Nov 19, 2015

justbur commented Nov 19, 2015

StreakyCobra commented Nov 19, 2015

syl20bnr commented Nov 19, 2015

justbur commented Nov 19, 2015

StreakyCobra commented Nov 19, 2015

syl20bnr commented Nov 19, 2015

syl20bnr commented Nov 19, 2015

justbur commented Nov 19, 2015

TheBB commented Nov 19, 2015

justbur commented Nov 19, 2015

justbur commented Nov 19, 2015

TheBB commented Nov 21, 2015

justbur commented Nov 21, 2015

TheBB commented Nov 21, 2015

justbur commented Nov 21, 2015

TheBB commented Nov 21, 2015

justbur commented Nov 21, 2015

justbur commented Nov 21, 2015

TheBB commented Nov 21, 2015

syl20bnr commented Nov 21, 2015

TheBB commented Nov 21, 2015

syl20bnr commented Nov 22, 2015

evil-leader -> bind-map #3860

evil-leader -> bind-map #3860

Conversation

justbur commented Nov 18, 2015

TheBB Nov 18, 2015

Choose a reason for hiding this comment

TheBB commented Nov 18, 2015

justbur commented Nov 18, 2015

syl20bnr Nov 19, 2015

Choose a reason for hiding this comment

syl20bnr Nov 19, 2015

Choose a reason for hiding this comment

syl20bnr commented Nov 19, 2015

syl20bnr commented Nov 19, 2015

syl20bnr commented Nov 19, 2015

justbur commented Nov 19, 2015

StreakyCobra commented Nov 19, 2015

syl20bnr commented Nov 19, 2015

justbur commented Nov 19, 2015

StreakyCobra commented Nov 19, 2015

syl20bnr commented Nov 19, 2015

syl20bnr commented Nov 19, 2015

justbur commented Nov 19, 2015

TheBB commented Nov 19, 2015

justbur commented Nov 19, 2015

justbur commented Nov 19, 2015

TheBB commented Nov 21, 2015

justbur commented Nov 21, 2015

TheBB commented Nov 21, 2015

justbur commented Nov 21, 2015

TheBB commented Nov 21, 2015

justbur commented Nov 21, 2015

justbur commented Nov 21, 2015

TheBB commented Nov 21, 2015

syl20bnr commented Nov 21, 2015

TheBB commented Nov 21, 2015

syl20bnr commented Nov 22, 2015