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

Enhance the describe-mode and more document #16401

Closed sunlin7 closed 1 month ago

sunlin7 commented 1 month ago

Hi, This PR is try to make more simple code for describe-mode, and more document for it.

Originally posted by @sunlin7 in https://github.com/syl20bnr/spacemacs/issues/16397#issuecomment-2110729312

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

smile13241324 commented 1 month ago

Is this still necessary? And can @fnussbaum have a quick look also given that this touches his recent fix?

sunlin7 commented 1 month ago

Hi @smile13241324 It makes the code simple and more document for future readers.

fnussbaum commented 1 month ago

I see no harm in applying this patch. That being said, I think the reason for the alias is sufficiently documented in the comment above it. I deliberately chose to turn the docstring from the workaround in the ivy layer into a comment because this makes describe-key SPC hdm immediately show the documentation of describe-mode (while also clearly mentioning that it is aliased). I assumed people curious about the alias would look into the source or even the vc-region-history, it mostly does not matter to end users I think. But if a docstring is preferred, that would of course be fine too.

Removing the condition on Helm being used is also okay, as I said before, I just subjectively prefer not defining an unnecessary alias when Helm is not used.

@sunlin7 I believe the cleanest solution would involve fixing the underlying issue in Helm and completely removing the alias. So maybe it would make sense to report this upstream, but that is not a high priority for me.

sunlin7 commented 1 month ago

Thanks for the comment, very clear.This issue took me hours to figure out the root cause, adding a doc here may save developer's hours when who runs into here.