casouri / eldoc-box

childframe doc for eglot and anything that uses eldoc
361 stars 26 forks source link

How to vary `eldoc-box-buffer-hook` locally? #111

Closed greghendershott closed 1 month ago

greghendershott commented 2 months ago

eldoc-box is cool! I'm trying to make sure it works well with eldoc from Racket Mode.

For Racket Mode, I have documentation strings carefully formatted from HTML. So I don't need the various markdown adjustments, and, I want to disable visual-line-mode (don't want any extra line-breaks).

I'm not sure the best way to do this. Of course I don't want to mess up other modes' use of eldoc-box for whom the defaults are desirable.

BUT: The problem is, eldoc-box--display does run-hook-with-args when the eldoc-box buffer is current -- not mine. So any local hook stuff for my buffer is ignored.

I understand why it runs the hook with the doc buffer current; the current hook functions assume they can just party on current-buffer.

A potential solution for this would be for run-hook-with-args to get the hook value from the previously current buffer, and still run the hook with the doc buffer current. Maybe something like:

(defun eldoc-box--display (str)
  "Display STR in childframe.
STR has to be a proper documentation, not empty string, not nil, etc."
  (let ((orig-buffer (current-buffer)) ; <--- NEW
        (doc-buffer (get-buffer-create eldoc-box--buffer)))
    (with-current-buffer doc-buffer
 ...
      (visual-line-mode)
      (run-hook-with-args (buffer-local-value 'eldoc-box-buffer-hook orig-buffer))) ; <--- CHANGED
    (eldoc-box--get-frame doc-buffer)))

I think this would be backward compatible?

(Another idea would be to run-hook-with-args when the doc buffer is not current, instead the original current buffer. But that would require changing the doc hook functions to take a new doc-buffer arg, which is not backward compatible and would be less convenient for them.)

Any thoughts?


Meanwhile, if you're not interested in changing this, my backup plan is kind of an ugly hack like:

;; We do want to customize `eldoc-box-buffer-hook' because:
;;
;; - We don't want the default markdown adjustments (best case they do
;; unnecessary work, worst case they might mis-format some things)
;;
;; - We want to disable `visual-line-mode'.
;;
;; Although a buffer-local hook is the usual way to handle something
;; like this, it doesn't work for eldoc-box-buffer-hook, because the
;; hook with /its/ buffer current, not ours. So we must resort to
;; munging the global hook, and either doing something special for us,
;; or doing the original default (so it keeps working when used by
;; other modes that do use markdown docs). Sort of the moral
;; equivalent of advising the hook.
(defvar racket--orig-eldoc-box-buffer-hook nil)
(defun racket-xp-eldoc-box-buffer-hook ()
  (if (next-single-property-change (point-min) 'racket-xp-eldoc)
      (visual-line-mode -1)
    (run-hook-with-args 'racket--orig-eldoc-box-buffer-hook)))
(eval-after-load 'eldoc-box
  (when (boundp 'eldoc-box-buffer-hook)
    (unless (member #'racket-xp-eldoc-box-buffer-hook
                    eldoc-box-buffer-hook)
      (setq racket--orig-eldoc-box-buffer-hook
            eldoc-box-buffer-hook)
      (setq eldoc-box-buffer-hook
            (list #'racket-xp-eldoc-box-buffer-hook)))))
greghendershott commented 2 months ago
  • As for avoiding the markdown functions: I can remove-hook with the LOCAL argument true.

Hmm. Actually, no -- (remove-hook hook function t) with t for LOCAL simply says, "no longer add this hook locally".

If the global value for the hook still has the function, then run-hook-with-args still runs it.

The idea seems to be, "run the local hooks, then run all the global hooks".

So in a *scratch* buffer if you do something like this:

(defun ex-print-1 ()
  (message "1"))
(defun ex-print-2 ()
  (message "2"))
(defvar ex-hook (list #'ex-print-1))
(run-hook-with-args 'ex-hook) ;outputs 1, then nil
(with-temp-buffer
  (add-hook 'ex-hook #'ex-print-2 nil t)
  (remove-hook 'ex-hook #'ex-print-1 t)
  (message "%S" ex-hook) ;(ex-print-2 t)
  (run-hook-with-args 'ex-hook)) ;outputs 2, 1, then nil

that ex-print-1 in the global value is always run.

In other words, if the global eldoc-box-buffer-hook contains

                              '(eldoc-box--prettify-markdown-separator
                                eldoc-box--replace-en-space
                                eldoc-box--remove-linked-images
                                eldoc-box--remove-noise-chars
                                eldoc-box--fontify-html
                                eldoc-box--condense-large-newline-gaps)

then those are run, always (plus whatever additional might be locally added in some buffer).

I don't see how a buffer can say, please don't run them, for me, but keep that default for others. (I don't see how, except for something like my hack.)

So... I'm no longer sure the best thing to do here.

greghendershott commented 2 months ago

Just riffing here (apologies if this is annoying):

If the main "consumer" of eldoc-box-mode is (I'm guessing from your README) eglot-managed-mode, then maybe the idea could be:

(defun eldoc-box-set-local-buffer-hook-for-markdown ()
  (dolist (fun '(eldoc-box--prettify-markdown-separator
                 eldoc-box--replace-en-space
                 eldoc-box--remove-linked-images
                 eldoc-box--remove-noise-chars
                 eldoc-box--fontify-html
                 eldoc-box--condense-large-newline-gaps))
    (add-hook 'eldoc-box-buffer-hook fun t t)))

(add-hook 'eldoc-managed-mode-hook
          #'eldoc-box-set-local-buffer-hook-for-markdown)

I'm not 100% sure of all the details -- and again I apologize for thinking out loud -- but I think this is closer to the right idea. Maybe?

casouri commented 2 months ago

Hi, thanks for your report. This is definitely a valid use-case. I added a new variable, eldoc-box-buffer-init-function, which should suit your needs. It's a bit convoluted, I'll admit, but it's flexible enough to let you do anything you need.

greghendershott commented 1 month ago

@casouri Thanks for the reply!

If you're willing to add some new function like this, that's great.

Unfortunately the idea of eldoc-box-init-function doesn't seem quite right: It runs too early and doesn't let me do the two key things:

  1. Undo or avoid (visual-line-mode)
  2. Avoid the (run-hook-with-args 'eldoc-box-buffer-hook).

Because the init function is called early, but then those happen anyway afterward, near the end.

(defun eldoc-box--display (str)
  "Display STR in childframe.
STR has to be a proper documentation, not empty string, not nil, etc."
  (let ((doc-buffer (get-buffer-create eldoc-box--buffer)))
  (let ((doc-buffer (get-buffer-create eldoc-box--buffer))
        (origin-buffer (current-buffer))
        (init-function eldoc-box-buffer-init-function))
    (with-current-buffer doc-buffer
      (funcall init-function origin-buffer)
      (setq mode-line-format nil)
      (setq header-line-format nil)
      ;; WORKAROUND: (issue#66) If cursor-type is ‘box’, sometimes the
      ;; cursor is still shown for some reason.
      (setq-local cursor-type t)
      (when (bound-and-true-p global-tab-line-mode)
        (setq tab-line-format nil))
      ;; Without this, clicking childframe will make doc buffer the
      ;; current buffer and `eldoc-box--maybe-cleanup' in
      ;; `eldoc-box--cleanup-timer' will clear the childframe
      (buffer-face-set 'eldoc-box-body)
      (setq eldoc-box-hover-mode t)
      (erase-buffer)
      (insert str)
      (goto-char (point-min))
      (visual-line-mode)
      (run-hook-with-args 'eldoc-box-buffer-hook))
    (eldoc-box--get-frame doc-buffer)))

Instead, how about something like an eldoc-box-format-function -- a quick sketch:

(defun eldoc-box-format-default (_original-buffer)
  "The default value of `eldoc-box-format-function'.
Enables `visual-line-mode` and runs `eldoc-box-buffer-hook'."
  (goto-char (point-min))
  (visual-line-mode 1)
  (run-hook-with-args 'eldoc-box-buffer-hook))

(defvar-local eldoc-box-format-function #'eldoc-box-format-default
  "A function run in the eldoc-box buffer, after the eldoc string
has been inerted. Intended for use by major modes that need to do
something other than `eldoc-box-format-default'. The function is
run with the eldoc-box child frame buffer current. The function
is given one argument, which is the original current buffer on
the parent frame.")

(defun eldoc-box--display (str)
  "Display STR in childframe.
STR has to be a proper documentation, not empty string, not nil, etc."
  (let ((doc-buffer (get-buffer-create eldoc-box--buffer))
        (origin-buffer (current-buffer))
        (format-function eldoc-box-buffer-format-function))
    (with-current-buffer doc-buffer
      (setq mode-line-format nil)
      (setq header-line-format nil)
      ;; WORKAROUND: (issue#66) If cursor-type is ‘box’, sometimes the
      ;; cursor is still shown for some reason.
      (setq-local cursor-type t)
      (when (bound-and-true-p global-tab-line-mode)
        (setq tab-line-format nil))
      ;; Without this, clicking childframe will make doc buffer the
      ;; current buffer and `eldoc-box--maybe-cleanup' in
      ;; `eldoc-box--cleanup-timer' will clear the childframe
      (buffer-face-set 'eldoc-box-body)
      (setq eldoc-box-hover-mode t)
      (erase-buffer)
      (insert str)
      (funcall format-function origin-buffer))
    (eldoc-box--get-frame doc-buffer)))

What do you think?

greghendershott commented 1 month ago

If you're reading this via email, please note that I edited my comment here on GitHub web UI. I fixed a mistake in my suggested new code.

casouri commented 1 month ago

You can use init function to change the local value of the hook, for visual-line-mode it can disabled in the hook. But whatever, I changed it to eldoc-box-buffer-setup-function, you can do whatever you want with it.

greghendershott commented 1 month ago

OK. I see what you merged. I will follow that approach instead.

Thank you very much for your time.

greghendershott commented 1 month ago

p.s. One tiny point: When you call a minor mode function like visual-line-mode with no arguments, that's the same as using it as an interactive M-x command -- it toggles the mode on/off.

If you definitively want visual-line-mode to be on, you must do (visual-line-mode 1) (or (visual-line-mode -1) to turn it off). For better or for worse, that's the Emacs convention. :smile:

So IIUC in your default setup function I think you want (visual-line-mode 1)?

If you agree, it's probably faster for you to make the change, than to merge a PR from me (but if you want the latter, let me know and I'd be glad to make one).

casouri commented 1 month ago

If you call it from lisp (not interactively), nil arg means enable the mode. So it's fine ;-)

greghendershott commented 1 month ago

Well that's embarrassing. Of course you're correct. Probably years ago I got advice, "All the permutations are confusing; be explicit", which I internalized as a requirement. I'm sorry I presented it that way!

casouri commented 1 month ago

No worries ;-) I honestly have forgotten it too before I read the docstring.