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

spacemacs-core: Fix issues with major-mode leader #3002

Closed
wants to merge 1 commit into from
Closed

spacemacs-core: Fix issues with major-mode leader #3002

wants to merge 1 commit into from

Conversation

justbur
Copy link
Contributor

@justbur justbur commented Sep 12, 2015

mode-map and major-mode-map were not let bound and had global scope. Seems like a bad idea for variables named like this

@justbur justbur changed the title spacemacs-core: Use let in activate-major-mode-leader spacemacs-core: Fix issues with major-mode leader Sep 13, 2015
Use same hook as evil-leader for major-mode-leader. This fixes an
inconsistency that could arise between the leader key being bound but
the major-mode leader key not being bound (See for example #3000). By
using the same hook as evil-leader-mode, we ensure that either both keys
or bound or neither.

A minor problem that was fixed was `mode-map` and `major-mode-map` were
not let bound and had global scope.
@justbur
Copy link
Contributor Author

justbur commented Sep 13, 2015

Added another change related to one of the issues brought up in #3000. It makes sense to use the same hook that evil-leader-mode does in binding the major-mode-leader key, so that we don't get the inconsistency between the leader being bound without the major-mode leader key being bound, which seems to be a result of one hook successfully firing and the previously used hook for the major-mode leader key not firing (because of an error in setting up the major-mode).

@TheBB
Copy link
Contributor

TheBB commented Sep 13, 2015

YES, that's a good solution. 💯

@TheBB
Copy link
Contributor

TheBB commented Sep 13, 2015

Although I should like to have it in both hooks, so that it updates if one actually does change the major mode. AFAICT there's no problem if this code runs twice.

@justbur
Copy link
Contributor Author

justbur commented Sep 13, 2015

@TheBB It will as it is here. evil-leader has to worry about the major-mode changing too. I'm just following it

@TheBB
Copy link
Contributor

TheBB commented Sep 13, 2015

Hm, you're right, it works.

@TheBB TheBB added the Reviewed label Sep 13, 2015
@justbur
Copy link
Contributor Author

justbur commented Sep 13, 2015

The idea is that it's as if we just rewrote evil-leader and added the code to the minor-mode initialization. For spacemacs we essentially need a version of evil-leader that support multiple leader keys

@syl20bnr
Copy link
Owner

Having our own evil-leader implementation can be considered. Also it may be useful to support minor modes if it is possible.

Thank you 👍
I moved the functions to a new file core-keybindings.el.

@TheBB @justbur I just realized that the choice of spacemacs-core for the base layer is not very smart... we already have a core in spacemacs... Do you have some alternative proposition while we can change it ?

Cherry-picked into develop branch, you can safely delete your branch.

@syl20bnr syl20bnr closed this Sep 14, 2015
@TheBB
Copy link
Contributor

TheBB commented Sep 14, 2015

Kernel? Essentials?

@TheBB
Copy link
Contributor

TheBB commented Sep 14, 2015

@justbur
Copy link
Contributor Author

justbur commented Sep 14, 2015

I like base layer. Something to build on.
On Mon, Sep 14, 2015 at 1:14 AM Eivind Fonn notifications@github.com
wrote:

http://i.imgur.com/i7LP5V9.png


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

@syl20bnr
Copy link
Owner

spacemacs-base ? I would prefer namespacing it so people are free to define a base layer for their own use.

@justbur
Copy link
Contributor Author

justbur commented Sep 15, 2015

Oh yes, sorry. That's what I meant

On Tue, Sep 15, 2015 at 9:29 AM, Sylvain Benner notifications@github.com
wrote:

spacemacs-base ? I would prefer namespacing it so people are free to
define a base layer for their own use.


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

robbyoconnor added a commit to robbyoconnor/spacemacs that referenced this pull request Sep 16, 2015
For background see the discussion in syl20bnr#3002.
robbyoconnor added a commit to robbyoconnor/spacemacs that referenced this pull request Sep 16, 2015
robbyoconnor added a commit to robbyoconnor/spacemacs that referenced this pull request Sep 16, 2015
robbyoconnor added a commit to robbyoconnor/spacemacs that referenced this pull request Sep 16, 2015
robbyoconnor added a commit to robbyoconnor/spacemacs that referenced this pull request Sep 16, 2015
Per the discussion in syl20bnr#3002. A comment in syl20bnr#3047 prompted this PR.
@robbyoconnor
Copy link
Contributor

I renamed spacemacs-core to spacemacs-base in #3048

syl20bnr pushed a commit that referenced this pull request Sep 17, 2015
Per the discussion in #3002. A comment in #3047 prompted this PR.
@justbur justbur deleted the let-bind-major-mode-map-func branch November 15, 2015 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants