search

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.56k stars 4.9k forks source link

Fix `describe-mode` keybinding breaking helm #16397

Closed fnussbaum closed 1 month ago

fnussbaum commented 1 month ago

Fixes #16395

smile13241324 commented 1 month ago

If you're ready please ping me directly I would like to have this fixed soon otherwise I have to rollback some of these changes I am afraid.

smile13241324 commented 1 month ago

Maybe @bcc32 also wants to have a quick look.

fnussbaum commented 1 month ago

Thank you @smile13241324 ! The PR is ready for review now. I added a commit message and tested basic functionality with the helm, ivy and compleseus layers (also with helm added as an additional package).

Conditioning on the helm package being used is more of a personal preference to avoid creating unnecessary aliases, one could also just always alias.

donm commented 1 month ago

I can confirm that this fixed the Key sequence M-m h d m starts with non-prefix key M-m error for me in a vanilla config of spacemacs, as well as with my personal config.

And now, not to overcomplicate things, but...

If it would be preferable to others, I think that it's possible to move this workaround to the Helm layer, by defining the spacemacs/describe-mode alias in +completion/helm/funcs.el and then setting up the keybinding in +completion/helm/packages.el files.

However, while testing that option, I went a bit further down a rabbit hole. I'll mention it in case it's helpful, but feel free to tell me that you all don't want to worry about this right now.

I noticed that spacemacs is already redefining the builtin describe-mode with a custom version, on line 867 of layers/+spacemacs/spacemacs-defaults/local/help-fns+/help-fns+.el. If we renamed that function to spacemacs/describe-mode, then this extra defalias wrapper wouldn't even be needed.

But that makes me wonder why spacemacs's custom definition of describe-mode (and related functions, like describe-face and describe-function) weren't already using the spacemacs/ prefix. Maybe there's a good reason, and even if not, maybe we don't want to worry about it right now just for the sake of fixing the current bug.

fnussbaum commented 1 month ago

If it would be preferable to others, I think that it's possible to move this workaround to the Helm layer, by defining the spacemacs/describe-mode alias in +completion/helm/funcs.el and then setting up the keybinding in +completion/helm/packages.el files.

Does this also work when not using the helm layer, but only including helm as an additional package? I tried to cover all of the cases with the above, but would be happy to change something if it is preferable.

But that makes me wonder why spacemacs's custom definition of describe-mode (and related functions, like describe-face and describe-function) weren't already using the spacemacs/ prefix. Maybe there's a good reason, and even if not, maybe we don't want to worry about it right now just for the sake of fixing the current bug.

Interesting, I don't know either.

donm commented 1 month ago

Does this also work when not using the helm layer, but only including helm as an additional package?

Well, that is a very good point. Probably not--it would have to be addressed somewhere in the base spacemacs config somewhere, like you did. (Well, as long as the"hdm" keybinding is also set up in the base spacemacs config.)

fnussbaum commented 1 month ago

Does this also work when not using the helm layer, but only including helm as an additional package?

Well, that is a very good point. Probably not--it would have to be addressed somewhere in the base spacemacs config somewhere, like you did. (Well, as long as the "hdm" keybinding is also set up in the base spacemacs config.)

Alright, I think we should support this case, as it seems to be common enough to keep helm around for some functionality after switching to ivy or compleseus. Having the hdm binding set up in the defaults is also a good thing I think.

I noticed that spacemacs is already redefining the builtin describe-mode with a custom version, on line 867 of layers/+spacemacs/spacemacs-defaults/local/help-fns+/help-fns+.el. If we renamed that function to spacemacs/describe-mode, then this extra defalias wrapper wouldn't even be needed.

This seems to only be the case when using an emacs version <= 27: 

    (help-fns+ :location local
               :toggle (not (fboundp 'describe-keymap))) ; built in emacs28+
donm commented 1 month ago

Oh, good call. I just spent a while reading about the history of help-fns+.el and what is and isn't included in vanilla emacs, and...it's a whole thing, evidently. I didn't know before that the file wasn't unique to spacemacs. But that explains why the functions defined in that file don't have the spacemacs/ prefix.

I'm sorry for sidetracking this PR! Thanks for your fix.

fnussbaum commented 1 month ago

No worries, it is quite interesting :)

sunlin7 commented 1 month ago

Acctually the https://github.com/syl20bnr/spacemacs/pull/16398 is a more simple solution.

bcc32 commented 1 month ago

Hi folks, sorry for the disruption this caused. (Oh how I wish there was just a test suite I could run.) I think #16398 would have been a cleaner solution but I don't have a strong opinion about this. I have some PRs incoming to clean more of this up once I get some free time---will try to remember to test with all the completion layers folks use before I submit :)

fnussbaum commented 1 month ago

Hi folks, sorry for the disruption this caused. (Oh how I wish there was just a test suite I could run.)

No worries, yes a test suite would be great!

Acctually the #16398 is a more simple solution.

I think #16398 would have been a cleaner solution but I don't have a strong opinion about this.

I do not see how #16398 would have solved the issue, the error also persists in my tests with just the ivy/helm layer changes.

The error was not caused by redefining the same binding within Spacemacs, it was caused by the Helm hack of looking up the global keybindings of describe-mode and trying to use the same keys to bind helm-help in the helm map. The problem with this is that M-m is already bound to helm-toggle-all-marks in there.

sunlin7 commented 1 month ago

Hi @fnussbaum Yes, you're right. The helm has special code to deal the describe-mode: https://github.com/emacs-helm/helm/blob/6d23a65ca6bcb6c0ea6f21f3cf2e58f8570ef75b/helm-core.el#L479-L481

(defvar helm-map
;;...
    (define-key map (kbd "M-m")        #'helm-toggle-all-marks)
;;...
    ;; Use `describe-mode' key in `global-map'.
    (dolist (k (where-is-internal #'describe-mode global-map))
      (define-key map k #'helm-help))
;;...

If arranged the describe-mode with key "M-m ..." before helm, the issue will be triggered. Thanks

sunlin7 commented 1 month ago

As fnussbaum commented in #16401, also reported this issue to Helm: https://github.com/syl20bnr/spacemacs/pull/16401#issuecomment-2111857243