search

emacs-evil / evil-collection

A set of keybindings for evil-mode
GNU General Public License v3.0
1.22k stars 274 forks source link

help menu in magit is not displayed properly #637

Open cs50Mu opened 2 years ago

cs50Mu commented 2 years ago

Press ? in magit-status gives the following in which some keybindings are wrong:

image

for example, Discard should be x instead of k; Reset should be O instead of X

magit-version gives:

image
jojojames commented 2 years ago

I see 'x' for discard and 'O' for Reset. Can you provide an emacs-q recipe?

cs50Mu commented 2 years ago

I see 'x' for discard and 'O' for Reset. Can you provide an emacs-q recipe?

I'm an emacs noob, did you mean run emacs with emacs -Q in terminal?

jojojames commented 2 years ago

Yeah, I added a section here:

https://github.com/emacs-evil/evil-collection#submitting-issues

emacs -Q in terminal, then you can probably eval that snippet and test from there.

cs50Mu commented 2 years ago

Yeah, I added a section here:

https://github.com/emacs-evil/evil-collection#submitting-issues

emacs -Q in terminal, then you can probably eval that snippet and test from there.

I've reproduced the issue with the following steps:

image

emacs version: I installed emacs from emacsformacosx
operating system: macOS Monterey 12.2.1

jojojames commented 2 years ago

Can you paste the exact emacs -Q recipe you used?

e.g. what you eval'ed

cs50Mu commented 2 years ago

Can you paste the exact emacs -Q recipe you used?

e.g. what you eval'ed

(setq user-emacs-directory "~/.emacs.1.d")

(setq package-enable-at-startup nil
      package-archives
      '(("melpa" . "https://melpa.org/packages/")
        ("gnu" . "http://elpa.gnu.org/packages/")))

(require 'package)
(package-initialize)
(unless (package-installed-p 'use-package)
  (package-refresh-contents)
  (package-install 'use-package))
(require 'use-package)
(setq use-package-always-ensure t)

(use-package evil
  :ensure t
  :init
  (setq evil-want-keybinding nil)
  :config
  (evil-mode 1))

(use-package evil-collection
  :after evil
  :ensure t
  :config
  (evil-collection-init))

(use-package magit)
jojojames commented 2 years ago

This one is very strange for me. The initial emacs-q doesn't work but sometimes, subsequent ones seem to 'resolve' itself.. Not sure what's going on.

Also, I messed up the snippet I gave you (but fixed it in my own while repro-ing). It should look like this instead.

It should include setting the package dir too.

(setq package-user-dir
      (format "%selpa/%s/" user-emacs-directory emacs-major-version))

Will update the snippet soon.

As for me, I'm suspecting maybe a new commit on magit and/or transient may be the issue, definitely not sure though. I'm a few commits behind on both. The other possibility is something with magit-section.

Magit: 6d66ab34 master magit-refresh-get-relative-position: Fix regression Transient: 2e4426f origin/master Quote single quote in docstring

I may update soon and try to see if I'm seeing the same issue.

jojojames commented 2 years ago

@cs50Mu When you emacs -Q a second time with the same recipe, what happens?

(setq user-emacs-directory "~/.emacs.1.d")

(setq package-user-dir
      (format "%s/elpa/%s/" user-emacs-directory emacs-major-version))

(setq package-enable-at-startup nil
      package-archives
      '(("melpa" . "https://melpa.org/packages/")
        ("gnu" . "http://elpa.gnu.org/packages/")))

(require 'package)
(package-initialize)
(unless (package-installed-p 'use-package)
  (package-refresh-contents)
  (package-install 'use-package))
(require 'use-package)
(setq use-package-always-ensure t)

(use-package evil
  :ensure t
  :init
  (setq evil-want-keybinding nil)
  :config
  (evil-mode 1))

(use-package evil-collection
  :after evil
  :ensure t
  :config
  (evil-collection-init))

(use-package magit)

For me, after the first run, the next emacs -Q (but pointed at the same elpa directory) works as expected.

cs50Mu commented 2 years ago

@jojojames sorry, a bit awkward, can you tell me how to do this..?

the next emacs -Q (but pointed at the same elpa directory)

I don't know how to load the previous elpa with emacs -Q

jojojames commented 2 years ago

Delete the emacs -Q directory (.emacs.1.d) emacs -Q with recipe -> creates a new .emacs.1.d quit emacs emacs -Q with same recipe -> should be pointed at .emacs.1.d

cs50Mu commented 2 years ago

After emacs -Q with the same recipe, I still get the same result...

k for Discard, and X for Reset

cs50Mu commented 2 years ago

I still have another emacs instance running when I'm doing the emacs -Q testing, does it matter?

jojojames commented 2 years ago

It shouldn't because your main emacs instance should be pointed at .emacs.d and your emacs -Q recipe should be pointed at .emacs.1.d. So you wiped .emacs.1.d, emacs -Q -> restarted and then -> emacs -Q again?

cs50Mu commented 2 years ago

I did the follows:

first time

first, make sure that .emacs.1.d folder is not there

second time

@jojojames

jojojames commented 2 years ago

@condy0919 Can you give it a shot?

condy0919 commented 2 years ago

Same issue. Let me dig it deeper.

progfolio commented 2 years ago

Just an FYI. I wrote yodel with these kinds of tests in mind. Here's a demo with your current test case:

Yodel Report (2022-06-08 15:05:14):

(yodel
  :save "yodel-magit"
  :interactive nil
  :packages* use-package
  :post*
  (setq straight-use-package-by-default t)
  (use-package evil
    :init
    (setq evil-want-keybinding nil)
    :config (evil-mode 1))
  (use-package evil-collection
    :after evil
    :config
    (evil-collection-init))
  (use-package magit)
  (let ((default-directory (straight--repos-dir "magit")))
    (magit-status)
    (magit-dispatch)
    (with-current-buffer transient--buffer-name
      (print (buffer-substring-no-properties (point-min) (point-max))))))
STDOUT: ```emacs-lisp Loading /tmp/yodel-magit/straight-bootstrap-snippet.el (source)... "Transient and dwim commands A Apply i Ignore r Rebase b Branch I Init t Tag B Bisect j Jump to section T Note c Commit J Display buffer V Revert C Clone l Log w Apply patches d Diff L Log (change) W Format patches D Diff (change) m Merge X Reset e Ediff (dwim) M Remote y Show Refs E Ediff o Submodule Y Cherries f Fetch O Subtree z Stash F Pull P Push Z Worktree h Help Q Command ! Run H Section info Applying changes a Apply s Stage S Stage all v Reverse u Unstage U Unstage all k Discard Essential commands g refresh current buffer C-x m show all key bindings q bury current buffer C-x i show Info manual toggle section at point visit thing at point " ```
Environment - **emacs version**: GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.34, cairo version 1.17.6) of 2022-06-04 - **system type**: gnu/linux
Packages | Name | Branch | Commit | Date | Source | |------------------------------------------------------------------|--------|-----------------------------------------------------------------------------------------------|------------|-----------------| | [use-package](https://github.com/jwiegley/use-package) | master | https://github.com/jwiegley/use-package/commit/a7422fb8ab1baee19adb2717b5b47b9c3812a84c | 2021-02-10 | melpa | | [bind-key](https://github.com/jwiegley/use-package) | master | https://github.com/jwiegley/use-package/commit/a7422fb8ab1baee19adb2717b5b47b9c3812a84c | 2021-02-10 | melpa | | [evil](https://github.com/emacs-evil/evil) | master | https://github.com/emacs-evil/evil/commit/157af04d2cf466e301e82b0e667c5e7203fd96a2 | 2022-05-18 | melpa | | [goto-chg](https://github.com/emacs-evil/goto-chg) | master | https://github.com/emacs-evil/goto-chg/commit/278cd3e6d5107693aa2bb33189ca503f22f227d0 | 2022-01-07 | melpa | | [evil-collection](https://github.com/emacs-evil/evil-collection) | master | https://github.com/emacs-evil/evil-collection/commit/79fc09b0140311377726551ee31622f61d28c571 | 2022-06-06 | melpa | | [annalist](https://github.com/noctuid/annalist.el) | master | https://github.com/noctuid/annalist.el/commit/134fa3f0fb91a636a1c005c483516d4b64905a6d | 2019-10-20 | melpa | | [magit](https://github.com/magit/magit) | master | https://github.com/magit/magit/commit/2dfeaa6839c643a54d96c9f855bae11d5cba2453 | 2022-06-07 | melpa | | [compat](https://github.com/emacs-straight/compat) | master | https://github.com/emacs-straight/compat/commit/884d47ef896cff2fa6910c359f1aa0e0ff8c7acd | 2022-06-07 | gnu-elpa-mirror | | [dash](https://github.com/magnars/dash.el) | master | https://github.com/magnars/dash.el/commit/0c49a33a61c739eb12e4c3680138c1659926202d | 2022-06-07 | melpa | | [git-commit](https://github.com/magit/magit) | master | https://github.com/magit/magit/commit/2dfeaa6839c643a54d96c9f855bae11d5cba2453 | 2022-06-07 | melpa | | [transient](https://github.com/magit/transient) | master | https://github.com/magit/transient/commit/2e4426fe8161893382f09b3f4635e152ee02488e | 2022-05-28 | melpa | | [with-editor](https://github.com/magit/with-editor) | master | https://github.com/magit/with-editor/commit/cfcbc2731e402b9169c0dc03e89b5b57aa988502 | 2022-06-08 | melpa | | [magit-section](https://github.com/magit/magit) | master | https://github.com/magit/magit/commit/2dfeaa6839c643a54d96c9f855bae11d5cba2453 | 2022-06-07 | melpa |

I left the :interactive nil in there because it's often useful to toggle that to t so you can play around in the test emacs session.

jojojames commented 2 years ago

What about the second run?

progfolio commented 2 years ago

What about the second run?

That was the second run. :save path preserves the environment between runs. On your first run you would see much more output from straight bootstrapping and installing the packages.

jojojames commented 2 years ago

Not really sure :D, maybe @tarsius can help. We're not (hopefully not) doing anything special with the popups other than swapping them here: https://github.com/emacs-evil/evil-collection/blob/e049bc4777cae21d937e08ad2d3960f3e1432dd5/modes/magit/evil-collection-magit.el#L621

tarsius commented 2 years ago

Try if doing this manually works:

(transient-suffix-put 'magit-dispatch "O" :key "\"")
(transient-suffix-put 'magit-dispatch "X" :key "O")
(transient-suffix-put 'magit-dispatch "k" :key "x")

The only potential problem that I see is (require 'magit nil t). Like a keymap, a transient command cannot be modified until it is defined. But if it is not, then you should get errors like transient--layout-member: foobar is not a transient command. Still, my guess is that an with-eval-after-load is missing somewhere.

I would add debug statements to see whether the relevant code is being called, and when. Start by adding (message "-- %s: %s => %s (%s)" popup from to (featurep 'magit)) to evil-collection-magit-change-popup-key.

condy0919 commented 2 years ago

Try if doing this manually works:

(transient-suffix-put 'magit-dispatch "O" :key "\"")
(transient-suffix-put 'magit-dispatch "X" :key "O")
(transient-suffix-put 'magit-dispatch "k" :key "x")

The only potential problem that I see is (require 'magit nil t). Like a keymap, a transient command cannot be modified until it is defined. But if it is not, then you should get errors like transient--layout-member: foobar is not a transient command. Still, my guess is that an with-eval-after-load is missing somewhere.

It's here. https://github.com/emacs-evil/evil-collection/blob/master/evil-collection.el#L884

ThorpeJosh commented 2 years ago

Hey, Just wanted to list a few more keybindings in the menu that are not evil compliant:

Discard should be x instead of k Reset should be O instead of X Reverse should be - instead of v Revert should be _ instead of V Display status should be g (I think) instead of j

adamliter commented 1 year ago

I'm having this same issue with a relatively vanilla installation of Doom Emacs, for what it's worth.

zach-delong commented 1 year ago

I am seeing the same issue running Emacs 28.2 on Fedora. Specifically, I am seeing the help menu tell me that o is bound to "submodule" but it isn't (submodule is bound to '). o is actually bound to magit-reset-quickly, even when running emacs -q with the recipe posted above.

FWIW, I am running my own configs and have not changed a ton. here is a link: https://github.com/ZacheryPD/Emacs-From-Scratch/

randomizedthinking commented 1 year ago

I got the same issue with evil-collection, and my screenshot is the same as @cs50Mu has posted.

What I observed is that these key-mappings are correct in some sense. For instance, when I pressed h, the help window is shown. When the help window is present, X is indeed mapped to magit-reset. Yet, when just inside the magit buffer without the help window, X is mapped to magit-file-untrack.

All the incorrectly mapped keys are the same: their mappings are different with or without the help window.

sg-qwt commented 1 year ago

I'm having this same issue. After I put magit to load after evil-collection, issue seems to go away.

(use-package evil-collection
  :after (evil magit)
  .....
  )
tarsius commented 1 year ago

Check whether (featurep 'magit) is non-nil in evil-collection-magit-setup. If not, throwing a (require 'magit) might do the trick.

If that doesn't work, I recommend inserting debug statements in strategic places. Does evil-collection-magit-revert-popups get called unexpectedly? Is (get 'magit-dispatch 'transient--layout) non-nil before trying to modify it? Compare the value of that before and after calling (transient-suffix-put popup from :key to)), does it change as expected? Etc.

tshu-w commented 1 year ago

I tried to (require 'magit) in evil-collection-magit-setup, but it doesn't seem to work. It seems that I need to put (require 'magit) before (evil-collection-init) for it to work.

  (defun evil-collection-magit-setup@override ()
    "Set up `evil' bindings for `magit'."
    (require 'magit)
    (evil-collection-define-key 'normal 'magit-blame-mode-map
      "q" 'magit-blame-quit)
    (evil-collection-define-key 'normal 'magit-blame-read-only-mode-map
      "q" 'magit-blame-quit)

    (evil-collection-magit-init))
  (advice-add 'evil-collection-magit-setup@override :override 'evil-collection-magit-setup)
zach-delong commented 1 year ago

I'm having this same issue. After I put magit to load after evil-collection, issue seems to go away.

(use-package evil-collection
  :after (evil magit)
  .....
  )

I can't believe I never thought of this. I did this in my config and it works flawlessly now. Thank you!

sooriravindra commented 1 year ago 
(use-package evil-collection
  :after (evil magit)
  :config (evil-collection-init))

The above snippet solves it. But it has the side effect that dired doesn't behave as expected. Eg: hitting enter on a file name doesn't open the file in normal mode (you need to go into insert mode to be able to do so). Removing magit from the :after restores the behavior of dired but the magit help menu is wrong.

1player commented 1 year ago

But it has the side effect that dired doesn't behave as expected. Eg: hitting enter on a file name doesn't open the file in normal mode (you need to go into insert mode to be able to do so). Removing magit from the :after restores the behavior of dired but the magit help menu is wrong.

Can't reproduce here. :after (evil magit) solves this issue, and does not affect dired.

sooriravindra commented 1 year ago

I tracked down the issue to this minimal config:

(setq-default use-package-always-ensure t)
(require 'package)

(setq package-archives '(("melpa" . "https://melpa.org/packages/")
             ("elpa-devel" . "https://elpa.gnu.org/devel/")))
(package-initialize)

(package-refresh-contents)

(unless (package-installed-p 'use-package)
  (package-install 'use-package))

(require 'use-package)

(use-package evil
  :init
  (setq evil-want-integration t
        evil-want-keybinding nil)
  :config
  (evil-mode 1))

(use-package magit
  :config
  :bind ("C-x g" . magit-status)
  )

(use-package evil-collection
  :after evil magit
  :config
  (evil-collection-init))

The issue is seen with the above init.el but goes away when I remove the :bind for Magit. I can't figure out why! But my issue is resolved. It seems that bind is redundant anyway.

1player commented 1 year ago

@sooriravindra I would hazard a guess: The :bind incantation in use-package also enables deferred loading, so the package is loaded only when you press C-x g.

https://github.com/jwiegley/use-package#key-binding

If you configure evil-collection to wait until magit and dired are loaded, it means that it won't be loaded until you open magit at least once (dired is loaded with emacs AFAIK). Until then, evil-collection will not have set up the dired keybinds.

The workaround, if you want to still use the convenient use-package :bind feature, is to force it not to defer loading. You can do so by adding the :demand t option:

(use-package magit
  :demand t
  :config
  :bind ("C-x g" . magit-status)
  )

https://github.com/jwiegley/use-package#notes-about-lazy-loading

This will fix it. In any case, there is no need to bind C-x g to magit-status, as it's already the default, so as you've already noticed, removing that line makes your issue go away.

sooriravindra commented 1 year ago

@1player Thanks a bunch for demystifying it for me.

swinkels commented 4 months ago

It took some time to find out what causes this issue, but I think I found it:

  1. When you load the upstream magit module for the first time, it starts by loading its dependencies, one of them being the upstream magit-repos module.
  2. The load of magit-repos is setup to trigger the load and setup of evil-collection-magit - this happens in lines 921-925 of evil-collection.el. So the load of magit is interrupted to do that load and setup.
  3. The load of evil-collection-magit also loads upstream magit, and because the load of magit at step 1 was interrupted, this actual loads it. This sets up the original keybindings for the magit dispatch popup menu. The subsequent call to evil-collection-magit-setup overwrites them with the evil ones.
  4. Now the bug shows up: the load of magit at step 1 was interrupted but continues after the evil-collection-magit work is done. When it continues, it again sets up the keybindings of the dispatch menu, thereby "resetting" the evil ones.
swinkels commented 4 months ago

I think you can workaround the issue by removing magit-repos as a dependency from magit in evil-collection--supported-modes at https://github.com/emacs-evil/evil-collection/blob/12fd87b631eea6b9b9be96456f39a03f0864dd3c/evil-collection.el#L253 This should be safe to do as magit requires magit-repos, "famous last words" anyone?

Disclaimer: I use evil-collection via Doom Emacs and Doom does things somewhat different with regard to evil-collection. For example, it initializes evil-collection-mode-list to a list that is very similar to the list of evil-collection--supported-modes. If I apply the workaround to evil-collection-mode-list in Doom Emacs, the bindings of transient menu magit-dispatch are set correctly.

tshu-w commented 4 months ago

I think you can workaround the issue by removing magit-repos as a dependency from magit in evil-collection--supported-modes at

I confirm removing magit-repos from evil-collection--supported-modes fix this issue.

condy0919 commented 4 months ago

Installed. Thanks for the excellent work @swinkels :rocket: