bug#70193: eglot: RFE: recenter buffer upon showDocument request

bug#70193: eglot: RFE: recenter buffer upon showDocument request

[Alan Donovan via Bug reports for GNU Emacs, the Swiss army knife of text editors]

In eglot 1.17, the LSP showDocument downcall opens the designated
file, moves the cursor to the designated position, and raises the
frame. One other thing it could do to make it easier to see where the
cursor is would be to recenter the buffer.

The patch below is a minimal fix; the discussion at
https://github.com/joaotavora/eglot/discussions/1382 suggests a couple
of possible refinements.

xtools$ git diff ~/.emacs.d/elpa/eglot-1.17/eglot.el{.bak,}
--- elpa/eglot-1.17/eglot.el.orig
+++ elpa/eglot-1.17/eglot.el
@@ -2460,6 +2460,7 @@ THINGS are either registrations or
unregisterations (sic)."
                ;; function, but `xref--goto-char' happens to have
                ;; exactly the semantics we want vis-a-vis widening.
                (xref--goto-char beg)
+              (recenter)
                (pulse-momentary-highlight-region beg end 'highlight)))))))
      (t (setq success :json-false)))
     `(:success ,success)))

bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[Alan Donovan via Bug reports for GNU Emacs, the Swiss army knife of text editors]

+cc Martin Rudalics (as suggested by Joao Tavora)

On Thu, 4 Apr 2024 at 09:17, Alan Donovan <adonovan@google.com> wrote:
>
> In eglot 1.17, the LSP showDocument downcall opens the designated
> file, moves the cursor to the designated position, and raises the
> frame. One other thing it could do to make it easier to see where the
> cursor is would be to recenter the buffer.
>
> The patch below is a minimal fix; the discussion at
> https://github.com/joaotavora/eglot/discussions/1382 suggests a couple
> of possible refinements.
>
> xtools$ git diff ~/.emacs.d/elpa/eglot-1.17/eglot.el{.bak,}
> --- elpa/eglot-1.17/eglot.el.orig
> +++ elpa/eglot-1.17/eglot.el
> @@ -2460,6 +2460,7 @@ THINGS are either registrations or
> unregisterations (sic)."
>                 ;; function, but `xref--goto-char' happens to have
>                 ;; exactly the semantics we want vis-a-vis widening.
>                 (xref--goto-char beg)
> +              (recenter)
>                 (pulse-momentary-highlight-region beg end 'highlight)))))))
>       (t (setq success :json-false)))
>      `(:success ,success)))

bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[Felician Nemeth]

>> the discussion at
>> https://github.com/joaotavora/eglot/discussions/1382 

I'd like to repeat the patched code may behave worse.  So, I don't think
it is a good idea to merge it in its current form.

bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[martin rudalics via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]

 > +cc Martin Rudalics (as suggested by Joao Tavora)

I refer to the following text from

https://github.com/joaotavora/eglot/discussions/1382

 > A variant of recenter that centers the whole selection if
 > (and only if) it fits might be a useful addition, perhaps to the the
 > core elisp library. In our case, which I suspect is fairly typical, the
 > selection is a single identifier such as a function or type declaration,
 > so the behavior of recenter is ideal, as it allows you to see the doc
 > comments above the declaration and (at least the start of) the function
 > body below it.

The attached 'window-recenter-region-start-position' should address
that.  I am not aware of whether we have a function on master to get the
line height of a specific window as if a specific buffer were displayed
in it so I used 'frame-char-height' for the window in question.  The
rest of the function is straightforward but there might be off-by-one
glitches.

Roughly tested with the also contained 'recenter-region'.

martin

[-- Attachment #2: recenter-region.el --]
[-- Type: text/x-emacs-lisp, Size: 1817 bytes --]

(defun window-recenter-region-start-position (&optional from to window buffer)
  "Return window start position for recentered region.
WINDOW specifies the window whose start position to return and defaults
to the selected window.  BUFFER specifies the buffer that contains the
region and defaults to WINDOW's buffer. FROM and TO specify the
beginning and end of the region in BUFFER and default to the active
region or the entire text of BUFFER."
  (let* ((window (or window (selected-window)))
	 (buffer (or buffer (window-buffer)))
	 (from (or from
		   (and (eq buffer (current-buffer))
			(region-active-p)
			(region-beginning))
		   (with-current-buffer buffer
		     (point-min))))
	 (to (or to
		 (and (eq buffer (current-buffer))
		      (region-active-p)
		      (region-end))
		 (with-current-buffer buffer
		   (point-max))))
	 (body-width (window-body-width window))
	 (body-height (window-body-height window))
	 old-buffer old-start old-point height start)
    (unless (eq (window-buffer window) buffer)
      (setq old-buffer (window-buffer window))
      (setq old-start (window-start window))
      (setq old-point (window-point window))
      (set-window-buffer window buffer))
    (setq height
	  (/ (cdr (window-text-pixel-size
		   window from to body-width body-height))
	     (frame-char-height (window-frame window))))
    (save-excursion
      (goto-char from)
      (when (<= height body-height)
	(forward-line (- (max (/ (- body-height height) 2) 0))))
      (setq start (pos-bol)))
    (when old-buffer
      (set-window-buffer window old-buffer)
      (set-window-point window old-point)
      (set-window-start window old-point))
    start))

(defun recenter-region ()
  (interactive)
  (mark-paragraph)
  (set-window-start
   (selected-window) (window-recenter-region-start-position)))

bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[Felician Nemeth]

Hi Martin,

> The attached 'window-recenter-region-start-position' should address
> that.  I am not aware of whether we have a function on master to get the
> line height of a specific window as if a specific buffer were displayed
> in it so I used 'frame-char-height' for the window in question.  The
> rest of the function is straightforward but there might be off-by-one
> glitches.

I have a not so recent build:
Development version e4d1739a2917 on HEAD branch; build date 2024-03-16.

But when I debug window-recenter-region-start-position, the height is
set to 0.  The patch below seems to fix the problem.

Thanks,
Felicián

--- /tmp/recenter-region.el~    2024-04-05 15:35:24.043123595 +0200
+++ /tmp/recenter-region.el     2024-04-05 15:47:55.430121441 +0200
@@ -19,8 +19,9 @@
                      (region-end))
                 (with-current-buffer buffer
                   (point-max))))
-        (body-width (window-body-width window))
         (body-height (window-body-height window))
+        (body-pixel-width (window-body-width window t))
+        (body-pixel-height (window-body-height window t))
         old-buffer old-start old-point height start)
     (unless (eq (window-buffer window) buffer)
       (setq old-buffer (window-buffer window))
@@ -29,7 +30,7 @@
       (set-window-buffer window buffer))
     (setq height
          (/ (cdr (window-text-pixel-size
-                  window from to body-width body-height))
+                  window from to body-pixel-width body-pixel-height))
             (frame-char-height (window-frame window))))
     (save-excursion
       (goto-char from)

bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[martin rudalics via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 711 bytes --]

 > But when I debug window-recenter-region-start-position, the height is
 > set to 0.  The patch below seems to fix the problem.
[...]
 > +        (body-pixel-width (window-body-width window t))
 > +        (body-pixel-height (window-body-height window t))

You're right.  I meanwhile fixed the code to calculate how many lines to
step backwards by using 'window-text-pixel-size' there too.  So now this
should work with text scaling and varying line heights too.

If you want to test it with 'recenter-region', then a rough estimate is
that the number of lines shown after the first "L:" should be equal to
or one less than the number of lines shown after the third "L:" in each
message issued.

Thanks, martin

[-- Attachment #2: recenter-region.el --]
[-- Type: text/x-emacs-lisp, Size: 3561 bytes --]

(defun window-recenter-region-start-position (&optional from to window buffer)
  "Return window start position for recentered region.
WINDOW specifies the window whose start position to return and defaults
to the selected window.  BUFFER specifies the buffer supposed to contain
the region and defaults to WINDOW's buffer. FROM and TO specify the
beginning and end of the region in BUFFER and default to the active
region or the entire text of BUFFER."
  (let* ((window (or window (selected-window)))
	 (buffer (or buffer (window-buffer window)))
	 (from (or from
		   (and (eq buffer (current-buffer))
			(region-active-p)
			(region-beginning))
		   (with-current-buffer buffer
		     (point-min))))
	 (to (or to
		 (and (eq buffer (current-buffer))
		      (region-active-p)
		      (region-end))
		 (with-current-buffer buffer
		   (point-max))))
	 (body-width (window-body-width window t))
	 (body-height (window-body-height window t))
	 old-buffer old-start old-point
	 remainder height start prev)
    ;; If WINDOW doesn't show BUFFEER, svae its buffeer, start and point
    ;; positions.
    (unless (eq (window-buffer window) buffer)
      (setq old-buffer (window-buffer window))
      (setq old-start (window-start window))
      (setq old-point (window-point window))
      (set-window-buffer window buffer))

    ;; The amount of pixels by which the region can be scrolled down in
    ;; the window.  Initially, half of the difference of the height of
    ;; the window's body and that of the region.
    (setq remainder
	  (round
	   (/ (- body-height
		 (cdr (window-text-pixel-size
		       window from to body-width body-height)))
	      2.0)))

    ;; Now move back and subtract the height of one line preceding the
    ;; region until the remainder has been used.
    (save-excursion
      (goto-char from)
      (setq start (pos-bol))
      (while (and (not (bobp))
		  (>= remainder 0)
		  (setq prev (pos-bol 0))
		  (setq height (cdr (window-text-pixel-size
				   window prev start
				   body-width body-height)))
		  ;; At least half of the line should fit.
		  (>= remainder (/ height 2.0)))
	(setq remainder (- remainder height))
	(setq start prev)
	(goto-char start)))

    ;; Restore WINDOW's old buffer, start and point.
    (when old-buffer
      (set-window-buffer window old-buffer)
      (set-window-point window old-point)
      (set-window-start window old-point))

    ;; Return the new start position.
    start))

(defun recenter-region ()
  (interactive)
  (mark-paragraph)
  (let ((from (region-beginning))
	(to (region-end))
	(body-width (window-body-width nil t))
	(body-height (window-body-height nil t))
	window-start window-end
	before at after)

    (set-window-start
     (selected-window) (window-recenter-region-start-position))
    (setq window-start (window-start))
    (setq window-end (window-end nil t))
    (message "%s..%s L: %s P: %s - L: %s P: %s - %s..%s L: %s P: %s - L: %s P: %s B: %s S: %s"
	     window-start from (count-lines window-start from)
	     (setq before (cdr (window-text-pixel-size
				nil window-start from body-width body-height)))
	     (count-lines from to)
	     (setq at (cdr (window-text-pixel-size
			    nil from to body-width body-height nil t)))
	     to window-end (count-lines to window-end)
	     (setq after (cdr (window-text-pixel-size
			       nil to window-end body-width body-height)))
	     (count-lines window-start window-end)
	     (cdr (window-text-pixel-size
		   nil window-start window-end body-width body-height))
	     body-height (+ before at after))))

bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[Eli Zaretskii]

> Cc: Alan Donovan <adonovan@google.com>, 70193@debbugs.gnu.org
> Date: Sun, 7 Apr 2024 09:30:02 +0200
> From:  martin rudalics via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
>  > But when I debug window-recenter-region-start-position, the height is
>  > set to 0.  The patch below seems to fix the problem.
> [...]
>  > +        (body-pixel-width (window-body-width window t))
>  > +        (body-pixel-height (window-body-height window t))
> 
> You're right.  I meanwhile fixed the code to calculate how many lines to
> step backwards by using 'window-text-pixel-size' there too.  So now this
> should work with text scaling and varying line heights too.
> 
> If you want to test it with 'recenter-region', then a rough estimate is
> that the number of lines shown after the first "L:" should be equal to
> or one less than the number of lines shown after the third "L:" in each
> message issued.

I'm unsure how to proceed with this bug report.  Should it be closed,
or is there anything left to be done here, and if the latter, then
what has to be done to resolve the issues?

Thanks.

bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[Felician Nemeth]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Alan Donovan <adonovan@google.com>, 70193@debbugs.gnu.org
>> Date: Sun, 7 Apr 2024 09:30:02 +0200
>> From:  martin rudalics via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>>  > But when I debug window-recenter-region-start-position, the height is
>>  > set to 0.  The patch below seems to fix the problem.
>> [...]
>>  > +        (body-pixel-width (window-body-width window t))
>>  > +        (body-pixel-height (window-body-height window t))
>> 
>> You're right.  I meanwhile fixed the code to calculate how many lines to
>> step backwards by using 'window-text-pixel-size' there too.  So now this
>> should work with text scaling and varying line heights too.
>> 
>> If you want to test it with 'recenter-region', then a rough estimate is
>> that the number of lines shown after the first "L:" should be equal to
>> or one less than the number of lines shown after the third "L:" in each
>> message issued.
>
> I'm unsure how to proceed with this bug report.  Should it be closed,
> or is there anything left to be done here, and if the latter, then
> what has to be done to resolve the issues?

Sorry, I meant to write back earlier.  I've done some limited test for
varying line heights as well, the patch seems to work well.  The
question, I think, is whether this is generally useful enough to have a
polished window-recenter-region to be part of Emacs, or should it just
be added to Eglot.

In the original report, showDocument requested to show a source code
file, where I think `reposition-window' would be more useful.  Alan, can
you check whether your use-case is better served with
`reposition-window' than with `recenter'?  However, the LSP
specification does not guarantee that the target of showDocument is a
source file, so Eglot needs window-recenter-region for completeness.
Also I don't know if reposition-window supports every programing
language or "go" in particular.

bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[Alan Donovan via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>  Alan, can you check whether your use-case is better served with `reposition-window' than with `recenter'?  However, the LSP specification does not guarantee that the target of showDocument is a source file, so Eglot needs window-recenter-region for completeness. Also I don't know if reposition-window supports every programming language or "go" in particular.

Thanks, I wasn't aware of `reposition-window', but it looks like
exactly what I want. I just tried it, and found that it positions the
point at the top of the frame, but is sufficiently aware of the syntax
of the language that if the point is in a declaration (as in my case)
then it uses the start of the preceding doc comment, if any.

It is clearly superior to recenter for my needs, and I would be quite
happy to use it instead.


On Sat, 13 Apr 2024 at 05:10, Felician Nemeth <felician.nemeth@gmail.com> wrote:
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> Cc: Alan Donovan <adonovan@google.com>, 70193@debbugs.gnu.org
> >> Date: Sun, 7 Apr 2024 09:30:02 +0200
> >> From:  martin rudalics via "Bug reports for GNU Emacs,
> >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >>
> >>  > But when I debug window-recenter-region-start-position, the height is
> >>  > set to 0.  The patch below seems to fix the problem.
> >> [...]
> >>  > +        (body-pixel-width (window-body-width window t))
> >>  > +        (body-pixel-height (window-body-height window t))
> >>
> >> You're right.  I meanwhile fixed the code to calculate how many lines to
> >> step backwards by using 'window-text-pixel-size' there too.  So now this
> >> should work with text scaling and varying line heights too.
> >>
> >> If you want to test it with 'recenter-region', then a rough estimate is
> >> that the number of lines shown after the first "L:" should be equal to
> >> or one less than the number of lines shown after the third "L:" in each
> >> message issued.
> >
> > I'm unsure how to proceed with this bug report.  Should it be closed,
> > or is there anything left to be done here, and if the latter, then
> > what has to be done to resolve the issues?
>
> Sorry, I meant to write back earlier.  I've done some limited test for
> varying line heights as well, the patch seems to work well.  The
> question, I think, is whether this is generally useful enough to have a
> polished window-recenter-region to be part of Emacs, or should it just
> be added to Eglot.
>
> In the original report, showDocument requested to show a source code
> file, where I think `reposition-window' would be more useful.  Alan, can
> you check whether your use-case is better served with
> `reposition-window' than with `recenter'?  However, the LSP
> specification does not guarantee that the target of showDocument is a
> source file, so Eglot needs window-recenter-region for completeness.
> Also I don't know if reposition-window supports every programing
> language or "go" in particular.

bug#70193: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[Felician Nemeth]

[-- Attachment #1: Type: text/plain, Size: 566 bytes --]

Hi João,

I've attached a patch fixing this issue.  It includes Martin's new
recenter-region.

This is the original report:

> In eglot 1.17, the LSP showDocument downcall opens the designated
> file, moves the cursor to the designated position, and raises the
> frame. One other thing it could do to make it easier to see where the
> cursor is would be to recenter the buffer.
> 
> The patch below is a minimal fix; the discussion at
> https://github.com/joaotavora/eglot/discussions/1382 suggests a couple
> of possible refinements.

Thanks.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Eglot-Recenter-buffer-upon-showDocument-request-bug-.patch --]
[-- Type: text/x-diff, Size: 5166 bytes --]

From 9496ee48c77f60c17387b4331911ba7ad5642c0e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Felici=C3=A1n=20N=C3=A9meth?= <felician.nemeth@gmail.com>
Date: Wed, 17 Apr 2024 10:47:03 +0200
Subject: [PATCH] Eglot: Recenter buffer upon showDocument request (bug#70193)

* lisp/progmodes/eglot.el (eglot--window-recenter-region): New
defun.
(eglot-handle-request window/showDocument): Try to show the whole
selection centered; in prog modes, try to show preceding comments
as well.

Co-authored-by: Martin Rudalics <rudalics@gmx.at>
---
 lisp/progmodes/eglot.el | 91 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 87 insertions(+), 4 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 5e4f7bba679..cbadd495b28 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -2488,6 +2488,76 @@ eglot-handle-request
   "Handle server request workspace/workspaceFolders."
   (eglot-workspace-folders server))
 
+(defun eglot--window-recenter-region (&optional from to window buffer)
+  "Center region and return window start position for recentered region.
+WINDOW specifies the window whose start position to return and defaults
+to the selected window.  BUFFER specifies the buffer supposed to contain
+the region and defaults to WINDOW's buffer. FROM and TO specify the
+beginning and end of the region in BUFFER and default to the active
+region or the entire text of BUFFER."
+  (let* ((window (or window (selected-window)))
+	 (buffer (or buffer (window-buffer window)))
+	 (from (or from
+		   (and (eq buffer (current-buffer))
+			(region-active-p)
+			(region-beginning))
+		   (with-current-buffer buffer
+		     (point-min))))
+	 (to (or to
+		 (and (eq buffer (current-buffer))
+		      (region-active-p)
+		      (region-end))
+		 (with-current-buffer buffer
+		   (point-max))))
+	 (body-width (window-body-width window t))
+	 (body-height (window-body-height window t))
+	 old-buffer old-point
+	 remainder height start prev)
+    ;; If WINDOW doesn't show BUFFER, save its buffer, start and point
+    ;; positions.
+    (unless (eq (window-buffer window) buffer)
+      (setq old-buffer (window-buffer window))
+      (setq old-point (window-point window))
+      (set-window-buffer window buffer))
+
+    ;; The amount of pixels by which the region can be scrolled down in
+    ;; the window.  Initially, half of the difference of the height of
+    ;; the window's body and that of the region.
+    (setq remainder
+	  (round
+	   (/ (- body-height
+		 (cdr (window-text-pixel-size
+		       window from to body-width body-height)))
+	      2.0)))
+
+    ;; Now move back and subtract the height of one line preceding the
+    ;; region until the remainder has been used.
+    (save-excursion
+      (goto-char from)
+      (setq start (pos-bol))
+      (while (and (not (bobp))
+		  (>= remainder 0)
+		  (setq prev (pos-bol 0))
+		  (setq height (cdr (window-text-pixel-size
+				   window prev start
+				   body-width body-height)))
+		  ;; At least half of the line should fit.
+		  (>= remainder (/ height 2.0)))
+	(setq remainder (- remainder height))
+	(setq start prev)
+	(goto-char start)))
+
+    ;; Restore WINDOW's old buffer, start and point.
+    (when old-buffer
+      (set-window-buffer window old-buffer)
+      (set-window-point window old-point)
+      (set-window-start window old-point))
+
+    (set-window-start window start)
+
+    ;; Return the new start position.
+    start))
+
 (cl-defmethod eglot-handle-request
   (_server (_method (eql window/showDocument)) &key
            uri external takeFocus selection)
@@ -2510,10 +2580,23 @@ eglot-handle-request
                  ((display-buffer (current-buffer))))
            (when selection
              (pcase-let ((`(,beg . ,end) (eglot-range-region selection)))
-               ;; FIXME: it is very naughty to use someone else's `--'
-               ;; function, but `xref--goto-char' happens to have
-               ;; exactly the semantics we want vis-a-vis widening.
-               (xref--goto-char beg)
+               (with-selected-window (get-buffer-window (current-buffer))
+                 ;; FIXME: it is very naughty to use someone else's `--'
+                 ;; function, but `xref--goto-char' happens to have
+                 ;; exactly the semantics we want vis-a-vis widening.
+                 (xref--goto-char end)
+                 (xref--goto-char beg)
+                 (eglot--window-recenter-region beg end)
+
+                 (when (derived-mode-p 'prog-mode)
+                   (let ((recentered-visible (pos-visible-in-window-p end))
+                         (recentered-start (window-start)))
+                     (reposition-window) ; Try to show preceding comments.
+                     (when (and recentered-visible
+                                (not (pos-visible-in-window-p end)))
+                       ;; reposition-window ruined visibility of selection.
+                       (set-window-start nil recentered-start)))))
+
                (pulse-momentary-highlight-region beg end 'highlight)))))))
      (t (setq success :json-false)))
     `(:success ,success)))
-- 
2.39.2

bug#70193: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[João Távora]

On Wed, Apr 17, 2024 at 10:15 AM Felician Nemeth
<felician.nemeth@gmail.com> wrote:
>
> Hi João,
>
> I've attached a patch fixing this issue.  It includes Martin's new
> recenter-region.

Thanks.  I'm sure the patch is correct but 50+ lines of
window-management code in eglot.el?  Just for that one user?

There's nothing LSP-specific in the new function
`eglot--window-recenter-region`,  it's pure window management code.

The derived-mode-p specifically bit also looks very much out
of place in Eglot.  What is it accomplishing, and why is this
prog-mode exception not in the preceding function itself
(maybe as an optional toggle)

In fact, there's nothing intrinsically LSP-specific about having
clients request Emacs shows the user a given part of a file.
I can't believe this obscure LSP interface is its only client.
Is it really?

So, if this new impeccably documented function designed by Emacs's
window management expert :-) does what it says, I think we should
put it somewhere else.

In fact, I think I've independently implemented parts of it in my
SLY and Zapp extensions at some point in time, where they also don't
belong so it would definitely be useful.

Practical matters: there's the usual problem with Eglot
compatibility with older Emacsen, but there's:

* compat.el (I think Phil has already made Eglot use compat.el recently)
* there's the strategy of using (or creating) another GNU ELPA :core package
(like external-completion.el and many other ones)
* there' the strategy of 'fboundp' where this nice-to-have "excellent
recentering" feature would only appear to Eglot users running it on
a recent Emacs.

I think all of these are preferable to bloating Eglot's code by this much
motivated by this minuscule use case.

All that said: I'll let you make the call Felicián :-)  If this is
another baby step towards you becoming Eglot maintainer, I'm happy :-)

João



João

bug#70193: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[Felician Nemeth]

João Távora <joaotavora@gmail.com> writes:

> I'm sure the patch is correct but 50+ lines of window-management code
> in eglot.el? 

I agree it can go elsewhere.

> Just for that one user?

I don't think that should matter.

> There's nothing LSP-specific in the new function
> `eglot--window-recenter-region`,  it's pure window management code.
>
> The derived-mode-p specifically bit also looks very much out
> of place in Eglot.  What is it accomplishing, and why is this
> prog-mode exception not in the preceding function itself
> (maybe as an optional toggle)

reposition-window relies on `beginning-of-defun' and `end-of-defun', but
it is potentially better than window-recenter-region, because if the
selection of showDocument is inside a function, then it tries to show
the preceding comments as well.

> In fact, there's nothing intrinsically LSP-specific about having
> clients request Emacs shows the user a given part of a file.
> I can't believe this obscure LSP interface is its only client.
> Is it really?

Maybe textDocument/publishDiagnostics is somewhat similar.  Flymake
shows the region of the diagnostic messages with as little care as Eglot
currently shows the selection of showDocument.

> So, if this new impeccably documented function designed by Emacs's
> window management expert :-) does what it says, I think we should
> put it somewhere else.

I agree.

> Practical matters: there's the usual problem with Eglot
> compatibility with older Emacsen, but there's:
>
> * compat.el (I think Phil has already made Eglot use compat.el recently)
> * there's the strategy of using (or creating) another GNU ELPA :core package
> (like external-completion.el and many other ones)
> * there' the strategy of 'fboundp' where this nice-to-have "excellent
> recentering" feature would only appear to Eglot users running it on
> a recent Emacs.

I think the third option is good enough in this case.

The current patch can be regarded as a prof-of-concept implementation if
anyone is interested in continuing to work on this.

bug#70193: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 2572 bytes --]

On Tue, Apr 23, 2024 at 8:43 AM Felician Nemeth <felician.nemeth@gmail.com>
wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > I'm sure the patch is correct but 50+ lines of window-management code
> > in eglot.el?
>
> I agree it can go elsewhere.
>

window.el seem like a good place.


> > Just for that one user?
>
> I don't think that should matter.
>

What I meant is that if it goes into eglot.el it will be "just for that one
[Eglot]
user" .  If it goes somewhere else, it's for more users.

> There's nothing LSP-specific in the new function
> > `eglot--window-recenter-region`,  it's pure window management code.
> >
> > The derived-mode-p specifically bit also looks very much out
> > of place in Eglot.  What is it accomplishing, and why is this
> > prog-mode exception not in the preceding function itself
> > (maybe as an optional toggle)
>
> reposition-window relies on `beginning-of-defun' and `end-of-defun', but
> it is potentially better than window-recenter-region, because if the
> selection of showDocument is inside a function, then it tries to show
> the preceding comments as well.
>

I see, more or less.  So it's a further refinement specific to
window-recenter-region specific to prog-mode buffers.  Can it operate
independently  or does it have to come necessarily after a call to
window-recenter-region?  Regardless of the answer, I think it
would be best placed in prog-mode.el or whereabouts.


> > In fact, there's nothing intrinsically LSP-specific about having
> > clients request Emacs shows the user a given part of a file.
> > I can't believe this obscure LSP interface is its only client.
> > Is it really?
>
> Maybe textDocument/publishDiagnostics is somewhat similar.  Flymake
> shows the region of the diagnostic messages with as little care as Eglot
> currently shows the selection of showDocument.
>

Eh.  Guilty as charged.  So another user, showing Flymake diagnostics
(not necessarily from LSP publishDiagnostics, mind you).


> > * compat.el (I think Phil has already made Eglot use compat.el recently)
> > * there's the strategy of using (or creating) another GNU ELPA :core
> package
> > (like external-completion.el and many other ones)
> > * there' the strategy of 'fboundp' where this nice-to-have "excellent
> > recentering" feature would only appear to Eglot users running it on
> > a recent Emacs.
>
> I think the third option is good enough in this case.
>

It's a nice one.  I think compat.el is not in Eglot just yet anyway.

João

[-- Attachment #2: Type: text/html, Size: 4277 bytes --]

bug#70193: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[Juri Linkov]

>     > I'm sure the patch is correct but 50+ lines of window-management code
>     > in eglot.el?
>
>     I agree it can go elsewhere.
>
> window.el seem like a good place.

window.el is already full, there is no free space left in window.el.
However, reposition.el is almost empty, there is only 1 function in it.

bug#70193: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

On Wed, Apr 24, 2024, 17:56 Juri Linkov <juri@linkov.net> wrote:

> >     > I'm sure the patch is correct but 50+ lines of window-management
> code
> >     > in eglot.el?
> >
> >     I agree it can go elsewhere.
> >
> > window.el seem like a good place.
>
> window.el is already full, there is no free space left in window.el.
> However, reposition.el is almost empty, there is only 1 function in it.
>

:)

>

[-- Attachment #2: Type: text/html, Size: 948 bytes --]

bug#70193: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon showDocument request)

[Philip Kaludercic]

João Távora <joaotavora@gmail.com> writes:


[...]

> Practical matters: there's the usual problem with Eglot
> compatibility with older Emacsen, but there's:
>
> * compat.el (I think Phil has already made Eglot use compat.el recently)

FTR: This is true, but the changes haven't been pushed yet, since I have to
find the time to integrate them into Eglot's GitHub system.

-- 
	Philip Kaludercic on peregrine