casouri / eldoc-box

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

Manually trigggering eldoc-box-help-at-point will often show previous/outdated docs #96

Open ashlineldridge opened 8 months ago

ashlineldridge commented 8 months ago

This is related to https://github.com/casouri/eldoc-box/issues/92 but the author of that issue seemed to reach a resolution to their issue so I've created a new one.

When eldoc-box is triggered manually via eldoc-box-help-at-point (rather than enabling one of the hover modes), it will show outdated documentation if value of the eldoc--doc-buffer buffer hasn't yet updated.

This issue can be reproduced by moving point to a symbol and quickly calling eldoc-box-help-at-point. If it is the first time calling eldoc-box-help-at-point and the eldoc--doc-buffer buffer hasn't been updated then eldoc-box will present an empty popup. If you call it again after a small delay then the correct documentation will be copied from the eldoc--doc-buffer buffer. However, if you move point to another symbol and quickly call eldoc-box-help-at-point then the old documentation from the previous symbol will be displayed.

In my own config, I've worked around this issue with the following approach which hooks into eldoc-display-functions to update eldoc-box if Eldoc updates. It's not perfect, however, as the outdated documentation will be shown briefly followed by an update of the popup to the correct documentation, but at least the correct documentation is eventually displayed.

(use-package eldoc-box
  :preface
  (defun my/eldoc-box-visible-p ()
    "Return whether the `eldoc-box' popup is visible."
    (and eldoc-box--frame (frame-visible-p eldoc-box--frame)))

  (defun my/eldoc-box-toggle ()
    "Toggle the `eldoc-box-help-at-point' popup."
    (interactive)
    (require 'eldoc-box)
    (if (my/eldoc-box-visible-p)
        (eldoc-box-quit-frame)
      (eldoc-box-help-at-point)))

  (defun my/eldoc-display-in-eldoc-box (&rest _)
    "Display latest Eldoc buffer in `eldoc-box' if visible."
    (when (my/eldoc-box-visible-p)
      (eldoc-box-help-at-point)))

  :bind
  ("M-p" . my/eldoc-box-toggle)
  :custom
  (eldoc-box-clear-with-C-g t)
  :config
  (add-to-list 'eldoc-display-functions #'my/eldoc-display-in-eldoc-box))
aguynamedben commented 7 months ago

I saw this too with

     Status: Installed in ‘eldoc-box-20231115.519/’ (unsigned).
    Version: 20231115.519
     Commit: c36f31074b09930e8425963f39d5508da6d2c32d
    Summary: Display documentation in childframe
   Requires: emacs-27.1
    Website: https://github.com/casouri/eldoc-box
 Maintainer: Yuan Fu <casouri@gmail.com>
     Author: Yuan Fu <casouri@gmail.com>
Other versions: 20231115.519 (melpa).
casouri commented 7 months ago

Hmmm, right. I found this function eldoc-print-current-symbol-info. Maybe this would work better? (untested)

(defun my/eldoc-display-in-eldoc-box (&rest _)
  "Display latest Eldoc buffer in `eldoc-box' if visible."
  (eldoc-print-current-symbol-info)
  (eldoc-box-help-at-point))
ashlineldridge commented 7 months ago

Thanks for the suggestion, @casouri. Unfortunately, eldoc-print-current-symbol-info doesn't seem to force a synchronous update of eldoc--doc-buffer. Below is my current config for anyone else encountering this issue - the config I posted above doesn't take ordering of eldoc-display-functions hooks into account:

(use-package eldoc-box
  :preface
  (declare-function eldoc-box-help-at-point "eldoc-box")
  (declare-function eldoc-box-quit-frame "eldoc-box")

  (defun my/eldoc-box-visible-p ()
    "Return whether the `eldoc-box' popup is visible."
    (and eldoc-box--frame (frame-visible-p eldoc-box--frame)))

  (defun my/eldoc-box-toggle ()
    "Toggle the `eldoc-box-help-at-point' popup."
    (interactive)
    (require 'eldoc-box)
    (if (my/eldoc-box-visible-p)
        (eldoc-box-quit-frame)
      (eldoc-box-help-at-point)))

  ;; Workaround to ensure the correct documentation is shown by the `eldoc-box'
  ;; popup if Eldoc is updated. See: https://github.com/casouri/eldoc-box/issues/96.
  (defun my/eldoc-display-in-eldoc-box (&rest _)
    "Display latest Eldoc buffer in `eldoc-box' if visible."
    (when (my/eldoc-box-visible-p)
      (eldoc-box-help-at-point)))

  :bind
  ("M-p" . my/eldoc-box-toggle)
  :custom
  (eldoc-box-clear-with-C-g t)
  :config
  ;; The function `my/eldoc-display-in-eldoc-box' needs to be called after
  ;; `eldoc-display-in-buffer' to get the new value of  `eldoc--doc-buffer'.
  (remove-hook 'eldoc-display-functions #'eldoc-display-in-buffer)
  (add-hook 'eldoc-display-functions #'my/eldoc-display-in-eldoc-box)
  (add-hook 'eldoc-display-functions #'eldoc-display-in-buffer))
qcfu-bu commented 7 months ago

One hack that seems to help me with this issue is setting eldoc-idle-delay to 0 and disabling eldoc in the minibuffer.

shipmints commented 5 months ago

This works for me:

(use-package eldoc
...
  :init
  (defun my/eldoc-clear-doc-hook ()
    ;; (setq eldoc-last-message nil) ; needed?
    (when (bound-and-true-p eldoc--doc-buffer)
      (with-current-buffer eldoc--doc-buffer
        (let ((inhibit-read-only t))
          (erase-buffer)))))
  (add-hook 'window-state-change-hook #'my/eldoc-clear-doc-hook)
...

This option really should be a core eldoc feature to clear its buffer on context change.

One change eldoc-box needs to make, though, in eldoc-box-help-at-point is in addition to (when (boundp 'eldoc--doc-buffer) it should test for an empty buffer so no blank child frame is displayed (= (buffer-size buffer) 0).

I've bound the following function to this ("M-h" . #'my/eldoc-box-help-at-point) in lieu of the change.

  (defun my/eldoc-box-help-at-point ()
    (interactive)
    (when (bound-and-true-p eldoc--doc-buffer)
      (when (> (buffer-size eldoc--doc-buffer) 0)
        (eldoc-box-help-at-point))))

One other related thing is that eldoc-box-help-at-point does not clear the eldoc buffer when point gets moved around and whatever is in the eldoc buffer gets displayed without a context refresh. e.g., fill eldoc with something and then move point to a blank line in an unrelated part of the buffer and invoke eldoc-box-help-at-point and boom the old content is still there.

casouri commented 5 months ago

I think I know what's going on now. I pushed a change, could you guys see if it fixes the problem?

As for not showing blank popup, I believe showing a blank popup is better than not showing a popup—if we don't show a popup with no explanation, the user will think something went wrong.

ashlineldridge commented 5 months ago

@casouri This fixes the issue for me! Thank you 🙇🏽

shipmints commented 5 months ago

Greetings, @casouri and thank you for taking the time to look into this again.

The change seems to speed up eldoc-box (I use it in on-demand mode not hover) which is good.

BUT it does not prevent stale eldoc buffers; e.g., when switching buffers and without moving point in the newly displayed buffer, whatever was documented at point in the old buffer is still stale. I will be keeping my eldoc hook to clear the buffer on window state change.

I will also keep my eldoc-box check for an empty eldoc buffer and stop the empty child frame from appearing. Perhaps this could be an eldoc-box option for users to inhibit if they'd prefer. Something like eldoc-box-inhibit-empty-childframe.

P.S. I agree with @joostkremers https://github.com/casouri/eldoc-box/commit/7892a467dd1e4cc95bbf6ac86c1dd65e3c0c4445#commitcomment-142502195 that the memq check is not necessary as add-hook itself prevents duplicates.

casouri commented 5 months ago

BUT it does not prevent stale eldoc buffers; e.g., when switching buffers and without moving point in the newly displayed buffer, whatever was documented at point in the old buffer is still stale. I will be keeping my eldoc hook to clear the buffer on window state change.

Yeah the fix I made doesn't fix that particular issue. And as you said, the fix is about eldoc, not eldoc-box.

I will also keep my eldoc-box check for an empty eldoc buffer and stop the empty child frame from appearing. Perhaps this could be an eldoc-box option for users to inhibit if they'd prefer. Something like eldoc-box-inhibit-empty-childframe.

P.S. I agree with @joostkremers https://github.com/casouri/eldoc-box/commit/7892a467dd1e4cc95bbf6ac86c1dd65e3c0c4445#commitcomment-142502195 that the memq check is not necessary as add-hook itself prevents duplicates.

I pushed a change that makes eldoc-box show some message when there's no doc to show at point. I also removed the memq check :-)

shipmints commented 5 months ago

@casouri there is an issue with eldoc-box--help-at-point-async-update

error in process filter: eldoc-box--help-at-point-async-update: Wrong type argument: frame-live-p, nil
error in process filter: Wrong type argument: frame-live-p, nil

This started coming up for me as I'm experimenting with tab-bar-mode. I didn't dig into this further but async update needs to check for a nil child frame?

casouri commented 5 months ago

Yep, it should've checked for nil childframe. I pushed a fix. Thanks for letting me know!