GNU bug report logs - #70193
eglot: RFE: recenter buffer upon showDocument request

Previous Next

Package: emacs;

Reported by: Alan Donovan <adonovan <at> google.com>

Date: Thu, 4 Apr 2024 13:18:02 UTC

Severity: normal

To reply to this bug, email your comments to 70193 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Thu, 04 Apr 2024 13:18:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Alan Donovan <adonovan <at> google.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Thu, 04 Apr 2024 13:18:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Alan Donovan <adonovan <at> google.com>
To: bug-gnu-emacs <at> gnu.org
Subject: eglot: RFE: recenter buffer upon showDocument request
Date: Thu, 4 Apr 2024 09:17:13 -0400
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)))




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Thu, 04 Apr 2024 13:33:04 GMT) Full text and rfc822 format available.

Message #8 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: Alan Donovan <adonovan <at> google.com>
To: 70193 <at> debbugs.gnu.org
Cc: rudalics <at> gmx.at
Subject: Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon
 showDocument request)
Date: Thu, 4 Apr 2024 09:32:35 -0400
+cc Martin Rudalics (as suggested by Joao Tavora)

On Thu, 4 Apr 2024 at 09:17, Alan Donovan <adonovan <at> 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)))




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Thu, 04 Apr 2024 14:44:03 GMT) Full text and rfc822 format available.

Message #11 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: Felician Nemeth <felician.nemeth <at> gmail.com>
To: 70193 <at> debbugs.gnu.org
Cc: rudalics <at> gmx.at, Alan Donovan <adonovan <at> google.com>
Subject: Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon
 showDocument request)
Date: Thu, 04 Apr 2024 16:43:18 +0200
>> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Fri, 05 Apr 2024 09:09:02 GMT) Full text and rfc822 format available.

Message #14 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Alan Donovan <adonovan <at> google.com>, 70193 <at> debbugs.gnu.org
Subject: Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon
 showDocument request)
Date: Fri, 5 Apr 2024 11:07:58 +0200
[Message part 1 (text/plain, inline)]
> +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
[recenter-region.el (text/x-emacs-lisp, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Fri, 05 Apr 2024 13:54:02 GMT) Full text and rfc822 format available.

Message #17 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: Felician Nemeth <felician.nemeth <at> gmail.com>
To: martin rudalics <rudalics <at> gmx.at>
Cc: Alan Donovan <adonovan <at> google.com>, 70193 <at> debbugs.gnu.org
Subject: Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon
 showDocument request)
Date: Fri, 05 Apr 2024 15:53:04 +0200
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)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Sun, 07 Apr 2024 07:31:01 GMT) Full text and rfc822 format available.

Message #20 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Felician Nemeth <felician.nemeth <at> gmail.com>
Cc: Alan Donovan <adonovan <at> google.com>, 70193 <at> debbugs.gnu.org
Subject: Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon
 showDocument request)
Date: Sun, 7 Apr 2024 09:30:02 +0200
[Message part 1 (text/plain, inline)]
> 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
[recenter-region.el (text/x-emacs-lisp, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Sat, 13 Apr 2024 08:09:02 GMT) Full text and rfc822 format available.

Message #23 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: martin rudalics <rudalics <at> gmx.at>, felician.nemeth <at> gmail.com,
 adonovan <at> google.com
Cc: 70193 <at> debbugs.gnu.org
Subject: Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon
 showDocument request)
Date: Sat, 13 Apr 2024 11:08:28 +0300
> Cc: Alan Donovan <adonovan <at> google.com>, 70193 <at> 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 <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Sat, 13 Apr 2024 09:12:02 GMT) Full text and rfc822 format available.

Message #26 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: Felician Nemeth <felician.nemeth <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>,  adonovan <at> google.com
Cc: martin rudalics <rudalics <at> gmx.at>, 70193 <at> debbugs.gnu.org
Subject: Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon
 showDocument request)
Date: Sat, 13 Apr 2024 11:10:54 +0200
Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: Alan Donovan <adonovan <at> google.com>, 70193 <at> 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 <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Sat, 13 Apr 2024 16:38:02 GMT) Full text and rfc822 format available.

Message #29 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: Alan Donovan <adonovan <at> google.com>
To: Felician Nemeth <felician.nemeth <at> gmail.com>
Cc: martin rudalics <rudalics <at> gmx.at>, Eli Zaretskii <eliz <at> gnu.org>,
 70193 <at> debbugs.gnu.org
Subject: Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer upon
 showDocument request)
Date: Sat, 13 Apr 2024 12:36:41 -0400
>  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 <at> gmail.com> wrote:
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> Cc: Alan Donovan <adonovan <at> google.com>, 70193 <at> 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 <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Wed, 17 Apr 2024 09:16:03 GMT) Full text and rfc822 format available.

Message #32 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: Felician Nemeth <felician.nemeth <at> gmail.com>
To: joaotavora <at> gmail.com
Cc: martin rudalics <rudalics <at> gmx.at>, Eli Zaretskii <eliz <at> gnu.org>,
 Alan Donovan <adonovan <at> google.com>, 70193 <at> debbugs.gnu.org
Subject: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE: recenter buffer
 upon showDocument request)
Date: Wed, 17 Apr 2024 11:15:25 +0200
[Message part 1 (text/plain, inline)]
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.


[0001-Eglot-Recenter-buffer-upon-showDocument-request-bug-.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Wed, 17 Apr 2024 12:14:02 GMT) Full text and rfc822 format available.

Message #35 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Felician Nemeth <felician.nemeth <at> gmail.com>
Cc: "Philip K." <philipk <at> posteo.net>, martin rudalics <rudalics <at> gmx.at>,
 Eli Zaretskii <eliz <at> gnu.org>, Alan Donovan <adonovan <at> google.com>,
 70193 <at> debbugs.gnu.org
Subject: Re: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE: recenter
 buffer upon showDocument request)
Date: Wed, 17 Apr 2024 13:13:10 +0100
On Wed, Apr 17, 2024 at 10:15 AM Felician Nemeth
<felician.nemeth <at> 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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Tue, 23 Apr 2024 07:45:01 GMT) Full text and rfc822 format available.

Message #38 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: Felician Nemeth <felician.nemeth <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: "Philip K." <philipk <at> posteo.net>, martin rudalics <rudalics <at> gmx.at>,
 Eli Zaretskii <eliz <at> gnu.org>, Alan Donovan <adonovan <at> google.com>,
 70193 <at> debbugs.gnu.org
Subject: Re: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE: recenter
 buffer upon showDocument request)
Date: Tue, 23 Apr 2024 09:43:47 +0200
João Távora <joaotavora <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Tue, 23 Apr 2024 09:29:03 GMT) Full text and rfc822 format available.

Message #41 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Felician Nemeth <felician.nemeth <at> gmail.com>
Cc: "Philip K." <philipk <at> posteo.net>, martin rudalics <rudalics <at> gmx.at>,
 Eli Zaretskii <eliz <at> gnu.org>, Alan Donovan <adonovan <at> google.com>,
 70193 <at> debbugs.gnu.org
Subject: Re: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE: recenter
 buffer upon showDocument request)
Date: Tue, 23 Apr 2024 10:27:37 +0100
[Message part 1 (text/plain, inline)]
On Tue, Apr 23, 2024 at 8:43 AM Felician Nemeth <felician.nemeth <at> gmail.com>
wrote:

> João Távora <joaotavora <at> 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
[Message part 2 (text/html, inline)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Wed, 24 Apr 2024 16:57:04 GMT) Full text and rfc822 format available.

Message #44 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: João Távora <joaotavora <at> gmail.com>
Cc: "Philip K." <philipk <at> posteo.net>,
 Felician Nemeth <felician.nemeth <at> gmail.com>, martin rudalics <rudalics <at> gmx.at>,
 Alan Donovan <adonovan <at> google.com>, Eli Zaretskii <eliz <at> gnu.org>,
 70193 <at> debbugs.gnu.org
Subject: Re: bug#70193: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE:
 recenter buffer upon showDocument request)
Date: Wed, 24 Apr 2024 19:37:23 +0300
>     > 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Wed, 24 Apr 2024 20:15:06 GMT) Full text and rfc822 format available.

Message #47 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: "Philip K." <philipk <at> posteo.net>,
 Felician Nemeth <felician.nemeth <at> gmail.com>, martin rudalics <rudalics <at> gmx.at>,
 Alan Donovan <adonovan <at> google.com>, Eli Zaretskii <eliz <at> gnu.org>,
 70193 <at> debbugs.gnu.org
Subject: Re: bug#70193: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE:
 recenter buffer upon showDocument request)
Date: Wed, 24 Apr 2024 21:14:09 +0100
[Message part 1 (text/plain, inline)]
On Wed, Apr 24, 2024, 17:56 Juri Linkov <juri <at> 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.
>

:)

>
[Message part 2 (text/html, inline)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#70193; Package emacs. (Sat, 27 Apr 2024 10:26:05 GMT) Full text and rfc822 format available.

Message #50 received at 70193 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: João Távora <joaotavora <at> gmail.com>
Cc: martin rudalics <rudalics <at> gmx.at>, Eli Zaretskii <eliz <at> gnu.org>,
 Felician Nemeth <felician.nemeth <at> gmail.com>,
 Alan Donovan <adonovan <at> google.com>, 70193 <at> debbugs.gnu.org
Subject: Re: [PATCH] Re: bug#70193: Acknowledgement (eglot: RFE: recenter
 buffer upon showDocument request)
Date: Sat, 27 Apr 2024 10:25:16 +0000
João Távora <joaotavora <at> 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




This bug report was last modified 209 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.