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

help menu in magit is not displayed properly #637

Open
cs50Mu opened this issue May 26, 2022 · 39 comments
Open

help menu in magit is not displayed properly #637

cs50Mu opened this issue May 26, 2022 · 39 comments

Comments

@cs50Mu
Copy link

cs50Mu commented May 26, 2022

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

@cs50Mu cs50Mu changed the title help menu in magit is not display properly help menu in magit is not displayed properly May 26, 2022
@jojojames
Copy link
Collaborator

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

@cs50Mu
Copy link
Author

cs50Mu commented May 28, 2022

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
Copy link
Collaborator

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
Copy link
Author

cs50Mu commented May 28, 2022

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:

  • start emacs with emacs -Q
  • in the scratch buffer, paste the code snippet you just provided: submitting-issues
  • add this line in the buffer: (use-package magit)
  • M-x and run the command: eval-buffer
  • open a file in a git project
  • press C-x g to show the magit-status panel
  • press ? to show the help menu
  • and it still show the wrong keybindings

image

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

@jojojames
Copy link
Collaborator

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

e.g. what you eval'ed

@cs50Mu
Copy link
Author

cs50Mu commented May 29, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Author

cs50Mu commented Jun 2, 2022

@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
Copy link
Collaborator

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
Copy link
Author

cs50Mu commented Jun 2, 2022

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

k for Discard, and X for Reset

@cs50Mu
Copy link
Author

cs50Mu commented Jun 2, 2022

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

@jojojames
Copy link
Collaborator

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
Copy link
Author

cs50Mu commented Jun 2, 2022

I did the follows:

first time

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

  • emacs -Q to start emacs
  • paste the recipe you just provided
  • M-x eval-buffer to run the recipe
  • open a file in a git project
  • press C-x g to show the magit-status panel
  • press ? to show the help menu
  • and it show the wrong keybindings
  • :q to exit emacs

second time

  • emacs -Q to start emacs again
  • paste the recipe you just provided
  • M-x eval-buffer to run the recipe (emacs reuses the previous .emacs.1.d folder)
  • open a file in a git project
  • in the magit-status panel, still sees wrong keybindings in help menu

@jojojames

@jojojames
Copy link
Collaborator

@condy0919 Can you give it a shot?

@condy0919
Copy link
Collaborator

condy0919 commented Jun 2, 2022

Same issue. Let me dig it deeper.

@progfolio
Copy link
Contributor

progfolio commented Jun 8, 2022

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:
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
 <tab>    toggle section at point   
 <return> 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 master jwiegley/use-package@a7422fb 2021-02-10 melpa
bind-key master jwiegley/use-package@a7422fb 2021-02-10 melpa
evil master emacs-evil/evil@157af04 2022-05-18 melpa
goto-chg master emacs-evil/goto-chg@278cd3e 2022-01-07 melpa
evil-collection master 79fc09b 2022-06-06 melpa
annalist master noctuid/annalist.el@134fa3f 2019-10-20 melpa
magit master magit/magit@2dfeaa6 2022-06-07 melpa
compat master emacs-straight/compat@884d47e 2022-06-07 gnu-elpa-mirror
dash master magnars/dash.el@0c49a33 2022-06-07 melpa
git-commit master magit/magit@2dfeaa6 2022-06-07 melpa
transient master magit/transient@2e4426f 2022-05-28 melpa
with-editor master magit/with-editor@cfcbc27 2022-06-08 melpa
magit-section master magit/magit@2dfeaa6 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
Copy link
Collaborator

What about the second run?

@progfolio
Copy link
Contributor

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
Copy link
Collaborator

Not really sure :D, maybe @tarsius can help. We're not (hopefully not) doing anything special with the popups other than swapping them here:

(transient-suffix-put popup from :key to))

@tarsius
Copy link
Contributor

tarsius commented Jun 9, 2022

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
Copy link
Collaborator

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
Copy link

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
Copy link

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

@zach-delong
Copy link

zach-delong commented Jan 20, 2023

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
Copy link

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
Copy link

sg-qwt commented Mar 29, 2023

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
Copy link
Contributor

tarsius commented Mar 29, 2023

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
Copy link
Contributor

tshu-w commented Mar 31, 2023

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
Copy link

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
Copy link

sooriravindra commented Apr 30, 2023

(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
Copy link

1player commented May 16, 2023

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
Copy link

sooriravindra commented May 17, 2023

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
Copy link

1player commented May 19, 2023

@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
Copy link

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

Nebucatnetzer pushed a commit to Nebucatnetzer/nixos that referenced this issue Sep 20, 2023
Otherwise the evil-collection doesn't get enabled until I've opened magit once.
This is required because of a bug with evil-collection. If it doesn't get
started after magit, the help menu is broken.
emacs-evil/evil-collection#637
christoffer-arvidsson added a commit to christoffer-arvidsson/null_emacs that referenced this issue Oct 1, 2023
@swinkels
Copy link

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
Copy link

swinkels commented Jun 3, 2024

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

(magit magit-repos magit-submodule)
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
Copy link
Contributor

tshu-w commented Jun 4, 2024

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
Copy link
Collaborator

Installed. Thanks for the excellent work @swinkels 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests