-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix describe-mode
keybinding breaking helm
#16397
Fix describe-mode
keybinding breaking helm
#16397
Conversation
describe-mode
keybinding breaking helm
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. |
Maybe @bcc32 also wants to have a quick look. |
This moves the existing workaround of aliasing `describe-mode' from the ivy layer to spacemacs-defaults. To avoid unnecessary aliasing we condition on the helm package being used. If it is not used, there is no problem with using the original `describe-mode' function. In addition, we remove the alias `lazy-helm/describe-mode' (which added an unnecessary (require 'helm)) from the helm layer, which originally prevented the issue from occuring with the helm layer on its own. With the introduction of the `spacemacs/describe-mode' alias it is obsolete, we do not need two aliases.
e93698c
to
553cd48
Compare
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. |
I can confirm that this fixed the 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 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 But that makes me wonder why spacemacs's custom definition of |
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.
Interesting, I don't know either. |
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 |
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.
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+ |
Oh, good call. I just spent a while reading about the history of I'm sorry for sidetracking this PR! Thanks for your fix. |
No worries, it is quite interesting :) |
Acctually the #16398 is a more simple solution. |
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 :) |
No worries, yes a test suite would be great!
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 |
Hi @fnussbaum
If arranged the |
As fnussbaum commented in #16401, also reported this issue to Helm: |
Fixes #16395