syl20bnr / spacemacs

A community-driven Emacs distribution - The best editor is neither Emacs nor Vim, it's Emacs *and* Vim!
http://spacemacs.org
GNU General Public License v3.0
23.71k stars 4.9k forks source link

Broken Colemak bindings for Magit status buffer #14366

Closed fosskers closed 3 years ago

fosskers commented 3 years ago

Description :octocat:

With the recent changes involving evil-collection, the hneio (i.e. colemak) bindings for the Magit status buffer seems to have stopped working. Moving the cursor up and down now requires j and k like normal evil-mode, but this isn't so ergonomic for Colemak users.

Reproduction guide :beetle:

Observed behaviour: :eyes: :broken_heart:

Pressing n (The physical j on the keyboard) to move the cursor down activates the "next" functionality of searching.

Expected behaviour: :heart: :smile:

Pressing n should move the cursor down one line.

System Info :computer:

duianto commented 3 years ago

A PR is pending with a possible fix: [keyboard-layout] Fix colemak-hneio next/prev line in magit #14369

Colemak isn't my default layout. Could you test that it works as expected.

fosskers commented 3 years ago

Your PR fixes it :+1: Thank you.

fosskers commented 3 years ago

Unfortunately this is broken again.

lebensterben commented 3 years ago

@fosskers can you pin down the commit that brings back the issue, via git-bisect ?

fosskers commented 3 years ago

Yup I'll do that today.

fosskers commented 3 years ago

Alright, found it. That was my first time actually using git-bisect and it was quite a good time! Here's the result:

982e2515a39fbaddf53cef2e583d748ca9eb4e9a is the first bad commit
commit 982e2515a39fbaddf53cef2e583d748ca9eb4e9a
Author: Lucius Hu <1222865+lebensterben@users.noreply.github.com>
Date:   Wed Apr 28 02:14:00 2021 +0000

    [keyboard-layout] replace `evil-magit` by `evil-collection-magit`

    `evil-magit` is part of `evil-collection-magit` now

 layers/+intl/keyboard-layout/packages.el | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
lebensterben commented 3 years ago

oops that's my fault then. I will fix it.

lebensterben commented 3 years ago

@fosskers Can you try again? I just pushed the fix. The cause of the issue is a mis-spelling..

fosskers commented 3 years ago

Unfortunately it's still broken :thinking:

lebensterben commented 3 years ago

@fosskers Below is the commit that breaks the keyboard layout: https://github.com/syl20bnr/spacemacs/commit/982e2515a39fbaddf53cef2e583d748ca9eb4e9a

The motivation of this commit was that evil-magit was superseded by evil-collection, and is provided by it in evil-collection.magit.el (See https://github.com/emacs-evil/evil-collection/tree/master/modes/magit).

This commit is straight-forward, just replace the old symbol names with new ones.

I tried to check every new symbol and made sure they are all defined.

Have you updated packages recently? Is evil-collection installed? (It would be in ~/.emacs.d/elpa/*/develop/evil-collection-*)

fosskers commented 3 years ago

I am all up-to-date and evil-collection is indeed installed, but Up and Down still aren't working in my Magit buffer. Is it relevant that I'm on Emacs 28? Do the recent changes interfere with the original fix for this issue?

lebensterben commented 3 years ago

It should not.

lebensterben commented 3 years ago

Can you manually remove evil-magit if it's installed?

lebensterben commented 3 years ago

BTW, try https://github.com/emacs-evil/evil-collection#key-translation in your user-config. I'm reviewing keyboard-layout layer and it's super badly written.

duianto commented 3 years ago

Reverting the previous fix: [keyboard-layout] Fix colemak-hneio next/prev line in magit #14369

1 file changed, 1 insertion(+), 9 deletions(-)
layers/+intl/keyboard-layout/packages.el | 10 +---------

modified   layers/+intl/keyboard-layout/packages.el
@@ -465,15 +465,7 @@
     :colemak-jkhl
     (kl/evil-correct-keys 'visual magit-mode-map
       "j"
-      "k")
-    :colemak-hnei
-    (progn
-      (kl/evil-correct-keys 'normal magit-mode-map
-        "j"
-        "k")
-      (kl/evil-correct-keys 'visual magit-mode-map
-        "j"
-        "k"))))
+      "k")))

 (defun keyboard-layout/pre-init-mu4e ()
   (kl|config mu4e

seems to fix colemak-hnei: n and e (qwerty j and k), so that they navigate up and down again.

These commands are called in both evil normal state and visual state:

 n                         ;; evil-next-visual-line
 e                         ;; evil-previous-visual-line

The previous "fix" was just done by copying the :colemak-jkhl key bindings above. https://github.com/syl20bnr/spacemacs/blob/a26121db4f60d49dd0ccb0bcf4a4fc538a11bc67/layers/%2Bintl/keyboard-layout/packages.el#L465-L468

@fosskers could you test if it also starts working for you when that commit is reverted? [keyboard-layout] Fix colemak-hneio next/prev line in magit · syl20bnr/spacemacs@3acf188 - Google Chrome https://github.com/syl20bnr/spacemacs/commit/3acf188bad47cab05eefaba1943a748c74015af9

fosskers commented 3 years ago

I will test that. I also got this warning in my *Warnings* buffer after blowing away my .emacs.d and letting everything reinstall/recompile: evil-bindings

Is this relevant to the issue?

duianto commented 3 years ago

The evil-want-keybinding warning is unrelated to this issue.

It appeared when the evil-collection was added.

Currently the variable is set in the function spacemacs-bootstrap/init-evil: https://github.com/syl20bnr/spacemacs/blob/a26121db4f60d49dd0ccb0bcf4a4fc538a11bc67/layers/%2Bdistributions/spacemacs-bootstrap/packages.el#L80-L83

spacemacs-bootstrap/init-evil might be loading before the packages are being installed.

And when the evil package is installed, then it might set the variable to t: https://github.com/emacs-evil/evil/blob/8dc0ccdc4c0246af87588a93812a23268f83ab94/evil-vars.el#L1965

And then the evil-collection loads and shows the warning?

The warning goes away after restarting, but it would be better if we could handle it somehow.

fosskers commented 3 years ago

Reverting that commit does indeed fix it. There's the PR for it ^^^^.

Thanks for diagnosing, folks.