casouri / eldoc-box

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

Handle pos-visible-in-window-p being nil #94

Closed LemonBreezes closed 7 months ago

LemonBreezes commented 9 months ago

I'm using EXWM with persp-mode and have noticed eldoc-box emitting an error when pos-visible-in-window-p is nil. This commit works around that error by just no-oping if pos-visible-in-window-p is nil.

casouri commented 7 months ago

Thanks for the patch! But when does the error happen? I'd rather fix the underlying problem, if there is one.

LemonBreezes commented 7 months ago

Thanks for the patch! But when does the error happen? I'd rather fix the underlying problem, if there is one.

Basically, the underlying issue is that the window-buffer is not necessarily in sync with the current-buffer because some commands such as view-echo-area-messages and set-window-configuration (used by workspace packages such as persp-mode) change the window-buffer without updating the current-buffer.

If you take the following example:

(let ((temp1 (get-buffer-create (make-temp-name "buffer1")))
      (temp2 (get-buffer-create (make-temp-name "buffer2"))))
  (with-current-buffer temp1
    (cl-dotimes (_ 10)
      (insert "Hello, world!\n")
      (goto-char (point-max))))
  (set-window-buffer nil temp2)
  (set-buffer temp1)
  (pos-visible-in-window-p nil nil t))

Here, pos-visible-in-window-p returns nil because the value of (point) is taken from the (current-buffer) whereas the buffer used for the visibility check is from (window-buffer), and these two can be different, as in the above example.

If you check out this excerpt of the C code for pos-visible-in-window-p,

  w = decode_live_window (window);
  buf = XBUFFER (w->contents);
  SET_TEXT_POS_FROM_MARKER (top, w->start);

  if (EQ (pos, Qt))
    posint = -1;
  else if (!NILP (pos))
    posint = fix_position (pos);
  else if (w == XWINDOW (selected_window))
    posint = PT;

this confirms what I am saying, because buf is the window's buffer whereas PT is defined as

#define PT (current_buffer->pt + 0)

and as I said before, these two can be different after certain commands like when you're switching workspaces with persp-mode or using view-echo-area-messages.

LemonBreezes commented 7 months ago

This is related to a similar PR I made to Magit: https://github.com/magit/magit/pull/5098 and to a bug report I submitted to Emacs https://debbugs.gnu.org/cgi/bugreport.cgi?bug=69259.

LemonBreezes commented 7 months ago

Thanks for the patch! But when does the error happen? I'd rather fix the underlying problem, if there is one.

Hi. Using the new information I presented, I have created an alternative fix. Simply (set-buffer (window-buffer)). It's considered a bug if the (current-buffer) is left out of sync with the (window-buffer) anyways (unless doing so temporarily in the middle of some computation).

casouri commented 7 months ago

That's great! Thanks for working on this. Do you mind changing the commit message as well? I don't have enough context to write it myself.

LemonBreezes commented 7 months ago

That's great! Thanks for working on this. Do you mind changing the commit message as well? I don't have enough context to write it myself.

Sure. I just rewrote the commit message.

casouri commented 7 months ago

Merged, thanks!

LemonBreezes commented 7 months ago

Merged, thanks!

Strangely enough, it did not merge the latest version of the code change. Maybe something with the repo settings, not sure.

casouri commented 7 months ago

Right. Thanks for catching that. I reverted the wrong commit and pushed the latest one.