bug#59149: Feature Request: Report progress of long requests in Eglot

bug#59149: Feature Request: Report progress of long requests in Eglot

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

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

Something I think would be nice to have in eglot is some kind of
progress indicator for long running requests. Attached is my attempt at
implementing these. The patch contains links to relevant LSP specs in
the commit message.

Here is a link to an old github discussion about progress notifications:
https://github.com/joaotavora/eglot/discussions/835 

It uses the built in progress-reporter to display progress in the
echo area. Something that may be missing is a way for the user to
enable/disable this. Not sure what the right facilities are for that.
The eglot-stay-out-of pattern maybe? I didn't include that because I'm
not sure what to "stay out of". Maybe the symbol `progress-reporter'?
Happy to add something like that.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: progress notification patch --]
[-- Type: text/x-patch, Size: 4085 bytes --]

From b53753f611045bb44ef4d1d30d6f426be889f277 Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Wed, 9 Nov 2022 08:46:45 -0500
Subject: [PATCH] Eglot: Display progress notifications in minibuffer as they
 arrive

The LSP spec describes methods for reporting progress on long running
jobs to the client:

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#progress
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workDoneProgress

This change reports those notifications in the minibuffer as they come
in. It shows a percent indicator (if the server provides theme), or a
spinner. This change should open the door for writing a "cancel long
running request" command, which are identified by these progress
notifications. See
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_workDoneProgress_cancel
---
 lisp/progmodes/eglot.el | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index ecfede8fa6..b85d9fd445 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -820,6 +820,9 @@ eglot-lsp-server
    (spinner
     :documentation "List (ID DOING-WHAT DONE-P) representing server progress."
     :initform `(nil nil t) :accessor eglot--spinner)
+   (progress-reporter-alist
+    :documentation "Alist of (PROGRESS-TOKEN . PROGRESS-REPORTER)."
+    :accessor eglot--progress-reporter-alist)
    (inhibit-autoreconnect
     :initform t
     :documentation "Generalized boolean inhibiting auto-reconnection if true."
@@ -2035,6 +2038,42 @@ eglot-handle-notification
   (_server (_method (eql telemetry/event)) &rest _any)
   "Handle notification telemetry/event.") ;; noop, use events buffer
 
+(defun eglot--progress-report-message (title message)
+  "Format a $/progress report message, given a TITLE and/or MESSAGE string."
+  (cond
+   ((and title message) (format "%s %s" title message))
+   (title title)
+   (message message)))
+
+(defun eglot--progress-reporter (server token)
+  "Get a prgress-reporter identified by the progress TOKEN from the SERVER ."
+  (cdr (assoc token (eglot--progress-reporter-alist server))))
+
+(defun eglot--progress-reporter-delete (server token)
+  "Delete progress-reporters identified by the progress TOKEN from the SERVER."
+  (setf (eglot--progress-reporter-alist server)
+        (assoc-delete-all token (eglot--progress-reporter-alist server))))
+
+(cl-defmethod eglot-handle-notification
+  (server (_method (eql $/progress)) &key token value)
+  "Handle a $/progress notification identified by TOKEN from the SERVER."
+  (cl-destructuring-bind (&key kind title percentage message) value
+    (pcase kind
+      ("begin" (let* ((prefix (format (concat "[eglot] %s %s:" (when percentage " "))
+                                      (eglot-project-nickname server) token))
+                      (pr (if percentage
+                              (make-progress-reporter prefix 0 100 percentage 1 0)
+                            (make-progress-reporter prefix nil nil nil 1 0))))
+                 (eglot--progress-reporter-delete server token)
+                 (setf (eglot--progress-reporter-alist server)
+                       (cons (cons token pr) (eglot--progress-reporter-alist server)))
+                 (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
+      ("report" (when-let ((pr (eglot--progress-reporter server token)))
+                  (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
+      ("end" (when-let ((pr (eglot--progress-reporter server token)))
+               (progress-reporter-done pr)
+               (eglot--progress-reporter-delete server token))))))
+
 (cl-defmethod eglot-handle-notification
   (_server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
            &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
-- 
2.38.1


[-- Attachment #3: Type: text/plain, Size: 30 bytes --]


Thank you,
-- 
Danny Freeman

bug#59149: Feature Request: Report progress of long requests in Eglot

[João Távora]

Danny Freeman <danny@dfreeman.email> writes:

> Something I think would be nice to have in eglot is some kind of
> progress indicator for long running requests. Attached is my attempt at
> implementing these. The patch contains links to relevant LSP specs in
> the commit message.
>
> Here is a link to an old github discussion about progress notifications:
> https://github.com/joaotavora/eglot/discussions/835 
>
> It uses the built in progress-reporter to display progress in the
> echo area. Something that may be missing is a way for the user to
> enable/disable this. Not sure what the right facilities are for that.
> The eglot-stay-out-of pattern maybe? I didn't include that because I'm
> not sure what to "stay out of". Maybe the symbol `progress-reporter'?
> Happy to add something like that.

Thanks Danny, this doesn't look bad at all, though I have yet to
understand how it works.  I can half-see why progress status should be a
server property, but it would be better if you provided an automated
test, or maybe just a step-by-step sequence diagram (which can be in
plain text and language) that clarifies this.  An animated gif that
might also work.

Anyway, the way the user opts out of LSP configuration is via the user
variable eglot-ignored-server-capabilities.  So there should be some
point where you check the associated LSP capability of "progress
reporting" with eglot--server-capable.

João

bug#59149: Feature Request: Report progress of long requests in Eglot

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

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

> Thanks Danny, this doesn't look bad at all, though I have yet to
> understand how it works.  I can half-see why progress status should be a
> server property, but it would be better if you provided an automated
> test, or maybe just a step-by-step sequence diagram (which can be in
> plain text and language) that clarifies this.  An animated gif that
> might also work.

I'd love to write some tests, but I can't seem to find any for eglot in
the emacs repository. I see they exist in the github project. Is there
something I'm overlooking?

> Anyway, the way the user opts out of LSP configuration is via the user
> variable eglot-ignored-server-capabilities.  So there should be some
> point where you check the associated LSP capability of "progress
> reporting" with eglot--server-capable.

That's what I'm looking for, thank you. I'll take a look at the prior
art in eglot and work this in.

-- 
Danny Freeman

bug#59149: Feature Request: Report progress of long requests in Eglot

[Stephen Leake]

This works for my needs in ada-mode; + 1.
-- 
-- Stephe

bug#59149: Feature Request: Report progress of long requests in Eglot

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

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> This works for my needs in ada-mode; + 1.

Great to hear!

> > Anyway, the way the user opts out of LSP configuration is via the user
> > variable eglot-ignored-server-capabilities.  So there should be some
> > point where you check the associated LSP capability of "progress
> > reporting" with eglot--server-capable.

I'm trying to work on using the eglot-ignored-server-capabilities
functionality with this, and am having some trouble. What exactly is the
capability to ignore here? The method used itself is `$/progress`. I
don't know how that translates to a "capability".

I believe this needs an entry somewhere in the
`eglot-client-capabilities` hierarchy.

This:
```
(cl-defgeneric eglot-client-capabilities (server)
  "What the Eglot LSP client supports for SERVER."
  (:method (s)
           (list
            :progress t
            ;; or maybe
            :$ (list :progress t)

            :workspace (list ...
            ... )))
```

does not translate into anything when checking
`(eglot--capabilities (eglot-current-server))`. Other capabilities are
listed but not the new one I've added.

nor does `(eglot--server-capable :progress)` ;; returns nil
`(eglot--server-capable :progressHandler)` ;; also nil

next, the entry for `eglot-ignored-server-capabilities` defcustom.
What should it be? something like:

```
(const :tag "Progress notifications" :progressProvider)
```

Any ideas what I'm doing wrong here?

-- 

Danny Freeman

bug#59149: Feature Request: Report progress of long requests in Eglot

[Stephen Leake]

Danny Freeman <danny@dfreeman.email> writes:

> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>
>> This works for my needs in ada-mode; + 1.
>
> Great to hear!
>
>> > Anyway, the way the user opts out of LSP configuration is via the user
>> > variable eglot-ignored-server-capabilities.  So there should be some
>> > point where you check the associated LSP capability of "progress
>> > reporting" with eglot--server-capable.
>
> I'm trying to work on using the eglot-ignored-server-capabilities
> functionality with this, and am having some trouble. What exactly is the
> capability to ignore here? 

The LSP says progress report is part of the base protocol, not optional:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#progress

> I believe this needs an entry somewhere in the
> `eglot-client-capabilities` hierarchy.
>
> This:
[2. application/emacs-lisp]
```
(cl-defgeneric eglot-client-capabilities (server)
  "What the Eglot LSP client supports for SERVER."
  (:method (s)
           (list
            :progress t
            ;; or maybe
            :$ (list :progress t)

            :workspace (list ...
            ... )))
```
>
>
>
> does not translate into anything when checking
> `(eglot--capabilities (eglot-current-server))`. Other capabilities are
> listed but not the new one I've added.

Moving :progress into :workspace worked for me:

            :workspace (list
                        :applyEdit t
                        :executeCommand `(:dynamicRegistration :json-false)
                        :workspaceEdit `(:documentChanges t)
                        :didChangeWatchedFiles
                        `(:dynamicRegistration
                          ,(if (eglot--trampish-p s) :json-false t))
                        :symbol `(:dynamicRegistration :json-false)
                        :configuration t
                        :semanticTokens `(:refreshSupport :json-false)
                        :workspaceFolders t
                        :progress t)

I have no idea why this works when yours does not.

> nor does `(eglot--server-capable :progress)` ;; returns nil
> `(eglot--server-capable :progressHandler)` ;; also nil
>

Note that this is checking the _server_ capabilities; since LSP does not
define "projess" as a capability, no server will ever advertise that
it is supported.

So we can't use eglot-ignores-server-capabilities; we could maybe
introduce eglot-ignored-client-capabilities.

But we already have a mechanism for that; eglot-stay-out-of.

So a user can do:

(add-to-list 'eglot-stay-out-of 'progress)

And in eglot-handle-notification ($/progress) we check for that:

(unless (eglot--stay-out-of-p 'progress)
   ...

There's probably a way to fold that check into the cl-defmethod
dispatching parameters; I did not look into that.

-- 
-- Stephe

bug#59149: Feature Request: Report progress of long requests in Eglot

[Stephen Leake]

I just ran into the progress reporter in eglot--apply-text-edits; I'd
like to turn that off, but leave other progress-reporters on.

So we need something more fine-grained:

(setq eglot-progress-reporter-disable '(apply-text-edits))

-- 
-- Stephe

bug#59149: Feature Request: Report progress of long requests in Eglot

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

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


Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Note that this is checking the _server_ capabilities; since LSP does not
> define "projess" as a capability, no server will ever advertise that
> it is supported.
>
> So we can't use eglot-ignores-server-capabilities; we could maybe
> introduce eglot-ignored-client-capabilities.
>
> But we already have a mechanism for that; eglot-stay-out-of.
>
> So a user can do:
>
> (add-to-list 'eglot-stay-out-of 'progress)
>
> And in eglot-handle-notification ($/progress) we check for that:
>
> (unless (eglot--stay-out-of-p 'progress)

Yeah, I think this will be the way to go as well.


> There's probably a way to fold that check into the cl-defmethod
> dispatching parameters; I did not look into that.

If there is I do not know how to do it either. I think a boolean check
in the body of the function is fine (and probably more clear).

> I just ran into the progress reporter in eglot--apply-text-edits; I'd
> like to turn that off, but leave other progress-reporters on.
> 
> So we need something more fine-grained:
> 
> (setq eglot-progress-reporter-disable '(apply-text-edits))

I believe that is a different progress reporter, unrelated to the one I
would like to introduce. It is not a progress report that come from the
lsp server, so I don't think it would be good to conflate them.

If the user wants to ignore specific progress indicators from the lsp
servers then they could implement an empty version of
eglot-handle-notification that matches their progress token.

```
(cl-defmethod eglot-handle-notification
  (_server (_method (eql $/progress)) &key (token (eql "THE PROGRESS TOKEN")) _value))
```

That would be more targeted. I also suspect it would be a pretty rare
occurrence. I personally think the best way to decide on adding more
configuration variables to eglot is to release this into the wild and
see what kind of feedback we receive.


Responding to João from a couple of threads ago:

> Thanks Danny, this doesn't look bad at all, though I have yet to
> understand how it works.  I can half-see why progress status should be a
> server property, but it would be better if you provided an automated
> test, or maybe just a step-by-step sequence diagram (which can be in
> plain text and language) that clarifies this.  An animated gif that
> might also work.

I think I can explain it with a sample of my server logs:

The first thing that happens is the lsp server sends a progress BEGIN
message, which indicates to the client that it is starting some long
running process.

```
[server-notification] Wed Nov 23 08:55:22 2022:
(:jsonrpc "2.0" :method "$/progress" :params
          (:token "clojure-lsp" :value
                  (:kind "begin" :title "clojure-lsp" :percentage 0)))
```

Following that is a series of REPORT type progress messages. These
indicate to the client that the work is ongoing. They might have a
`percentage` key somewhere between 0 and 100 to show how the work is
progressing.

```
[server-notification] Wed Nov 23 08:55:22 2022:
(:jsonrpc "2.0" :method "$/progress" :params
          (:token "clojure-lsp" :value
                  (:kind "report" :title "Finding kondo config" :percentage 5)))
[server-notification] Wed Nov 23 08:55:22 2022:
(:jsonrpc "2.0" :method "$/progress" :params
          (:token "clojure-lsp" :value
                  (:kind "report" :title "Finding cache" :percentage 10)))
[server-notification] Wed Nov 23 08:55:22 2022:
(:jsonrpc "2.0" :method "$/progress" :params
          (:token "clojure-lsp" :value
                  (:kind "report" :title "Copying kondo configs" :percentage 15)))
[server-notification] Wed Nov 23 08:55:22 2022:
(:jsonrpc "2.0" :method "$/progress" :params
          (:token "clojure-lsp" :value
                  (:kind "report" :title "Resolving config paths" :percentage 15)))
[server-notification] Wed Nov 23 08:55:22 2022:
(:jsonrpc "2.0" :method "$/progress" :params
          (:token "clojure-lsp" :value
                  (:kind "report" :title "Analyzing project files" :percentage 20)))

... this goes on for a while

[server-notification] Wed Nov 23 08:55:22 2022:
(:jsonrpc "2.0" :method "$/progress" :params
          (:token "clojure-lsp" :value
                  (:kind "report" :title "Analyzing project files" :percentage 99)))
```

Finally, the server sends an END type progress message, indicating that
the work is complete, and the client can stop showing the progress
message.

```
[server-notification] Wed Nov 23 08:55:22 2022:
(:jsonrpc "2.0" :method "$/progress" :params
          (:token "clojure-lsp" :value
                  (:kind "end" :title "Project analyzed" :percentage 100)))
```

The result of this patch is a series of messages that display in the
minibuffer:

```
[eglot:website] clojure-lsp:  
[eglot:website] clojure-lsp: 5% Finding kondo config
[eglot:website] clojure-lsp: 10% Finding cache
[eglot:website] clojure-lsp: 15% Copying kondo configs
[eglot:website] clojure-lsp: 20% Analyzing project files
[eglot:website] clojure-lsp: 26% Analyzing project files
[eglot:website] clojure-lsp: 33% Analyzing project files
[eglot:website] clojure-lsp: 39% Analyzing project files
[eglot:website] clojure-lsp: 46% Analyzing project files
[eglot:website] clojure-lsp: 52% Analyzing project files
[eglot:website] clojure-lsp: 59% Analyzing project files
[eglot:website] clojure-lsp: 66% Analyzing project files
[eglot:website] clojure-lsp: 72% Analyzing project files
[eglot:website] clojure-lsp: 79% Analyzing project files
[eglot:website] clojure-lsp: 85% Analyzing project files
[eglot:website] clojure-lsp: 92% Analyzing project files
[eglot:website] clojure-lsp: 99% Analyzing project files
[eglot:website] clojure-lsp: done
```

Does that help?

Attached is an updated patch that uses `eglot-stay-out-of`


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: progress reporter patch --]
[-- Type: text/x-patch, Size: 4114 bytes --]

From 54bc6b70c8a6da8853db1bcc45ca51796b98b6c2 Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Wed, 9 Nov 2022 08:46:45 -0500
Subject: [PATCH] Eglot: Display progress notifications in minibuffer as they
 arrive

The LSP spec describes methods for reporting progress on long running
jobs to the client:

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#progress
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workDoneProgress

This change reports those notifications in the minibuffer as they come
in. It shows a percent indicator (if the server provides theme), or a
spinner. This change should open the door for writing a "cancel long
running request" command, which are identified by these progress
notifications. See
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_workDoneProgress_cancel
---
 lisp/progmodes/eglot.el | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index e057b12e0e..3dc71b148c 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -821,6 +821,9 @@ eglot-lsp-server
    (project
     :documentation "Project associated with server."
     :accessor eglot--project)
+   (progress-reporter-alist
+    :documentation "Alist of (PROGRESS-TOKEN . PROGRESS-REPORTER)."
+    :accessor eglot--progress-reporter-alist)
    (inhibit-autoreconnect
     :initform t
     :documentation "Generalized boolean inhibiting auto-reconnection if true."
@@ -2037,6 +2040,43 @@ eglot-handle-notification
   (_server (_method (eql telemetry/event)) &rest _any)
   "Handle notification telemetry/event.") ;; noop, use events buffer
 
+(defun eglot--progress-report-message (title message)
+  "Format a $/progress report message, given a TITLE and/or MESSAGE string."
+  (cond
+   ((and title message) (format "%s %s" title message))
+   (title title)
+   (message message)))
+
+(defun eglot--progress-reporter (server token)
+  "Get a prgress-reporter identified by the progress TOKEN from the SERVER ."
+  (cdr (assoc token (eglot--progress-reporter-alist server))))
+
+(defun eglot--progress-reporter-delete (server token)
+  "Delete progress-reporters identified by the progress TOKEN from the SERVER."
+  (setf (eglot--progress-reporter-alist server)
+        (assoc-delete-all token (eglot--progress-reporter-alist server))))
+
+(cl-defmethod eglot-handle-notification
+  (server (_method (eql $/progress)) &key token value)
+  "Handle a $/progress notification identified by TOKEN from the SERVER."
+  (unless (eglot--stay-out-of-p 'progress)
+    (cl-destructuring-bind (&key kind title percentage message) value
+      (pcase kind
+        ("begin" (let* ((prefix (format (concat "[eglot] %s %s:" (when percentage " "))
+                                        (eglot-project-nickname server) token))
+                        (pr (if percentage
+                                (make-progress-reporter prefix 0 100 percentage 1 0)
+                              (make-progress-reporter prefix nil nil nil 1 0))))
+                   (eglot--progress-reporter-delete server token)
+                   (setf (eglot--progress-reporter-alist server)
+                         (cons (cons token pr) (eglot--progress-reporter-alist server)))
+                   (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
+        ("report" (when-let ((pr (eglot--progress-reporter server token)))
+                    (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
+        ("end" (when-let ((pr (eglot--progress-reporter server token)))
+                 (progress-reporter-done pr)
+                 (eglot--progress-reporter-delete server token)))))))
+
 (cl-defmethod eglot-handle-notification
   (_server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
            &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
-- 
2.38.1


[-- Attachment #3: Type: text/plain, Size: 19 bytes --]


-- 
Danny Freeman

bug#59149: Feature Request: Report progress of long requests in Eglot

[Stephen Leake]

Danny Freeman <danny@dfreeman.email> writes:

> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>
>> I just ran into the progress reporter in eglot--apply-text-edits; I'd
>> like to turn that off, but leave other progress-reporters on.
>> 
>> So we need something more fine-grained:
>> 
>> (setq eglot-progress-reporter-disable '(apply-text-edits))
>
> I believe that is a different progress reporter, unrelated to the one I
> would like to introduce. It is not a progress report that come from the
> lsp server, so I don't think it would be good to conflate them.

True, but how is the user to know where it comes from? We could provide
something like:

eglot-server-progress-reporter-disable
eglot-client-progress-reporter-disable

How does the user know which to set? They'll just set both, so there
might as well be only one.

> If the user wants to ignore specific progress indicators from the lsp
> servers then they could implement an empty version of
> eglot-handle-notification that matches their progress token.

I guess that's a reasonable starting point, but it doesn't work for
apply-text-edits.

> The result of this patch is a series of messages that display in the
> minibuffer:
>
> ```
> [eglot:website] clojure-lsp:  
> [eglot:website] clojure-lsp: 5% Finding kondo config

...

And the user can still use the minibuffer for find-file etc; the
messages are displayed to the right. Which can be confusing at first,
but works nicely.

-- 
-- Stephe

bug#59149: Feature Request: Report progress of long requests in Eglot

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

Stephen Leake <stephen_leake@stephe-leake.org> writes:

>> I believe that is a different progress reporter, unrelated to the one I
>> would like to introduce. It is not a progress report that come from the
>> lsp server, so I don't think it would be good to conflate them.
>
> True, but how is the user to know where it comes from? We could provide
> something like:
>
> eglot-server-progress-reporter-disable
> eglot-client-progress-reporter-disable
>
> How does the user know which to set? They'll just set both, so there
> might as well be only one.

I see your point here. I'm not quite sure what the right thing to do is.
Maybe it is a new "stay-out-of-progress" var. Maybe it we keep using
"stay-out-of" and have one symbol for general "progress" notifications,
and another distinct one for the "apply-text-edits-progress" one you
don't like. 

Probably should get João's opinion on this, he would have some good
ideas about this.

P.S.
I am copying him on this email, as I realize he wasn't on this branch of
the email chain. João, please see the previous emails in the thread for
more context! Here is a link for convenience:
https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-11/msg01619.html

-- 
Danny Freeman

bug#59149: Feature Request: Report progress of long requests in Eglot

[João Távora]

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

On Wed, Nov 23, 2022 at 7:45 PM Danny Freeman <danny@dfreeman.email> wrote:
>
>
> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>
> I am copying him on this email, as I realize he wasn't on this branch of
> the email chain. João, please see the previous emails in the thread for
> more context! Here is a link for convenience:
> https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-11/msg01619.html

Yep this is all news to me, but no problem, I just read through the chain.

If, like Stephen says, $progress is part of the base protocol, then there's
no capability associated indeed, but I think we should just make up one
like `:$progress`, and use change eglot--server-capable-p to be able
to respond unequivocally 't' to those special built in capabilities, but
only after checking if they're not in the eglot-ignored-server-capabilities
list.

I think this is more consistent with the other kinds of feature checks we
have elsewhere.

João

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

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

[Stephen Leake]

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

> On Wed, Nov 23, 2022 at 7:45 PM Danny Freeman <danny@dfreeman.email> wrote:
>>
>>
>> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>>
>> I am copying him on this email, as I realize he wasn't on this branch of
>> the email chain. João, please see the previous emails in the thread for
>> more context! Here is a link for convenience:
>> https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-11/msg01619.html
>
> Yep this is all news to me, but no problem, I just read through the chain.
>
> If, like Stephen says, $progress is part of the base protocol, then there's
> no capability associated indeed, but I think we should just make up one
> like `:$progress`, and use change eglot--server-capable-p to be able
> to respond unequivocally 't' to those special built in capabilities, but
> only after checking if they're not in the eglot-ignored-server-capabilities
> list.

One way to do that is to simply add (:$progresss t) in eglot--connect
where eglot--capabilities is set.


But apply-text-edits creates a progress reporter that is not driven by
messages from the lsp server; it's just processing a local list of
edits. And I would like to be able to turn that off, but keep (at least
some of) the lsp progress messages.

LSP capabilities handle this by adding items in categories, so we could
add (:$progress (:apply-edit t)) to eglot--capabilities, and then I
could add :apply-edit to eglot-ignored-server-capabilities. We'd have to
do the same for any other progress message somebody wants to turn off.

Except that would also turn off apply-edit itself, not just the progress
messages for it; it seems we need to allow
eglot-ignored-server-capabilities to contain (:$progress :apply-edit).
Or use :apply-edit-progress instead.

-- 
-- Stephe

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

[João Távora]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> On Wed, Nov 23, 2022 at 7:45 PM Danny Freeman <danny@dfreeman.email> wrote:
>>>
>>>
>>> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>>>
>>> I am copying him on this email, as I realize he wasn't on this branch of
>>> the email chain. João, please see the previous emails in the thread for
>>> more context! Here is a link for convenience:
>>> https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-11/msg01619.html
>>
>> Yep this is all news to me, but no problem, I just read through the chain.
>>
>> If, like Stephen says, $progress is part of the base protocol, then there's
>> no capability associated indeed, but I think we should just make up one
>> like `:$progress`, and use change eglot--server-capable-p to be able
>> to respond unequivocally 't' to those special built in capabilities, but
>> only after checking if they're not in the eglot-ignored-server-capabilities
>> list.
>
> One way to do that is to simply add (:$progresss t) in eglot--connect
> where eglot--capabilities is set.

Yes that would probably do it as an implementation trick.

> But apply-text-edits creates a progress reporter that is not driven by
> messages from the lsp server; it's just processing a local list of
> edits. And I would like to be able to turn that off, but keep (at least
> some of) the lsp progress messages.
>
> LSP capabilities handle this by adding items in categories, so we could
> add (:$progress (:apply-edit t)) to eglot--capabilities, and then I
> could add :apply-edit to eglot-ignored-server-capabilities. We'd have to
> do the same for any other progress message somebody wants to turn off.
>
> Except that would also turn off apply-edit itself, not just the progress
> messages for it; it seems we need to allow
> eglot-ignored-server-capabilities to contain (:$progress :apply-edit).
> Or use :apply-edit-progress instead.

At this point, I'm not sure the eglot-ignored-server-capabilities is the
right tool for the job.  In fact, even my original suggestion is a bit
sketchy, because it is creating these pseudo-capabilities that don't
really exist.  Abusing it even further is probably not good.

So, here three possible things we can do:

1. Get rid of the :apply-edit progress reporter entirely. To be honest,
   I don't think it's doing much.  We could just as well have a call to
   message there, or nothing at all.

2. Do my original "sketchy" suggestion, where :$progress is considered a
   built-in ignorable capability (and checked with eglot--server-capable
   in the new code that Danny is proposing).  Stephen's eglot-connect
   trick is an acceptable technique.

3. Add a boolean user varible eglot-report-progress.  I don't like to
   add user variables unless they represent things directly related to
   the fundamental LSP logic, and not its customization or evolution.
   Since this seems to be of those fundamental things, I think it's
   acceptable.

The alternatives are:

a: 1+2
b: 1+3
c: 2
d: 3

Stephen, you request to shoosh that particular apply-edits progress
reporter is another separate request, we shouldn't let it block Danny's
effort to support $progress messages.  So I think we should do either
'c' or 'd' for now, and we can always address your request later.

João

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

[Stephen Leake]

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

> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>
>> João Távora <joaotavora@gmail.com> writes:
>>
> 1. Get rid of the :apply-edit progress reporter entirely. To be honest,
>    I don't think it's doing much.  We could just as well have a call to
>    message there, or nothing at all.

It might be worth keeping a "debug message" there, gated by a new
variable eglot--debug (boolean or integer). So we can turn on messages
when we need to debug something.

Or just keep it commented out; it's easy enough to run eval-defun after
uncommenting.

> 2. Do my original "sketchy" suggestion, where :$progress is considered a
>    built-in ignorable capability (and checked with eglot--server-capable
>    in the new code that Danny is proposing).  Stephen's eglot-connect
>    trick is an acceptable technique.
>
> 3. Add a boolean user varible eglot-report-progress.  I don't like to
>    add user variables unless they represent things directly related to
>    the fundamental LSP logic, and not its customization or evolution.
>    Since this seems to be of those fundamental things, I think it's
>    acceptable.
>
> The alternatives are:
>
> a: 1+2
> b: 1+3
> c: 2
> d: 3
>
> Stephen, you request to shoosh that particular apply-edits progress
> reporter is another separate request, we shouldn't let it block Danny's
> effort to support $progress messages.  
> So I think we should do either 'c' or 'd' for now, and we can always
> address your request later.

Ok. Since that rules out b, I vote for d.
 

-- 
-- Stephe

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

[João Távora]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Or just keep it commented out; it's easy enough to run eval-defun after
> uncommenting.

Both approaches make sense.

>> So I think we should do either 'c' or 'd' for now, and we can always
>> address your request later.
>
> Ok. Since that rules out b, I vote for d.

OK.  Let's have Danny's opinion on it, especially on the name
'eglot-report-progress'.  In the simplest case, this variable is a
boolean, but I am curious if other types of progress reporting can be
envisioned.  If they are, they should be controlled by the least amount
of user variables possible.

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

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

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


Stephen Leake <stephen_leake@stephe-leake.org> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> 1. Get rid of the :apply-edit progress reporter entirely. To be honest,
>>    I don't think it's doing much.  We could just as well have a call to
>>    message there, or nothing at all.
>> 2. Do my original "sketchy" suggestion, where :$progress is considered a
>>    built-in ignorable capability (and checked with eglot--server-capable
>>    in the new code that Danny is proposing).  Stephen's eglot-connect
>>    trick is an acceptable technique.
>>
>> 3. Add a boolean user varible eglot-report-progress.  I don't like to
>>    add user variables unless they represent things directly related to
>>    the fundamental LSP logic, and not its customization or evolution.
>>    Since this seems to be of those fundamental things, I think it's
>>    acceptable.
>>
>> The alternatives are:
>>
>> a: 1+2
>> b: 1+3
>> c: 2
>> d: 3
>>
>> Stephen, you request to shoosh that particular apply-edits progress
>> reporter is another separate request, we shouldn't let it block Danny's
>> effort to support $progress messages.  
>> So I think we should do either 'c' or 'd' for now, and we can always
>> address your request later.
>
> Ok. Since that rules out b, I vote for d.

I am more partial to option d as well. From a user's perspective it
seems more straight forward than having to figure out the pseudo server
capability. I've attached an updated patch

-- 
Danny Freeman


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: eglot progress report patch --]
[-- Type: text/x-patch, Size: 5577 bytes --]

From 2787503eda061f934eebd23e7ee661d3f5ea547d Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Wed, 9 Nov 2022 08:46:45 -0500
Subject: [PATCH] Eglot: Display progress notifications in minibuffer as they
 arrive

* lisp/progmodes/eglot.el (eglot-report-progress): New custom variable.
(eglot-lsp-server): New slot for tracking active progress reporters.
(eglot-handle-notification): New impl for $/progress server responses.
(eglot--format-markup): replace tab with spaces.
(eglot--TextDocumentItem): replace tab with spaces.

The LSP spec describes methods for reporting progress on long running
jobs to the client:

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#progress
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workDoneProgress

This change reports those notifications in the minibuffer as they come
in. It shows a percent indicator (if the server provides theme), or a
spinner. This change should open the door for writing a "cancel long
running request" command, which are identified by these progress
notifications. See
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_workDoneProgress_cancel
---
 lisp/progmodes/eglot.el | 48 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index a0fb253e10..f01e6542af 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -386,6 +386,10 @@ eglot-menu-string
   "String displayed in mode line when Eglot is active."
   :type 'string)
 
+(defcustom eglot-report-progress t
+  "If non-nil, show progress of long running server work in the minibuffer."
+  :type 'boolean)
+
 (defvar eglot-withhold-process-id nil
   "If non-nil, Eglot will not send the Emacs process id to the language server.
 This can be useful when using docker to run a language server.")
@@ -831,6 +835,9 @@ eglot-lsp-server
    (project
     :documentation "Project associated with server."
     :accessor eglot--project)
+   (progress-reporter-alist
+    :documentation "Alist of (PROGRESS-TOKEN . PROGRESS-REPORTER)."
+    :accessor eglot--progress-reporter-alist)
    (inhibit-autoreconnect
     :initform t
     :documentation "Generalized boolean inhibiting auto-reconnection if true."
@@ -1569,7 +1576,7 @@ eglot--format-markup
       (setq-local markdown-fontify-code-blocks-natively t)
       (insert string)
       (let ((inhibit-message t)
-	    (message-log-max nil))
+            (message-log-max nil))
         (ignore-errors (delay-mode-hooks (funcall mode))))
       (font-lock-ensure)
       (string-trim (buffer-string)))))
@@ -2049,6 +2056,43 @@ eglot-handle-notification
   (_server (_method (eql telemetry/event)) &rest _any)
   "Handle notification telemetry/event.") ;; noop, use events buffer
 
+(defun eglot--progress-report-message (title message)
+  "Format a $/progress report message, given a TITLE and/or MESSAGE string."
+  (cond
+   ((and title message) (format "%s %s" title message))
+   (title title)
+   (message message)))
+
+(defun eglot--progress-reporter (server token)
+  "Get a prgress-reporter identified by the progress TOKEN from the SERVER ."
+  (cdr (assoc token (eglot--progress-reporter-alist server))))
+
+(defun eglot--progress-reporter-delete (server token)
+  "Delete progress-reporters identified by the progress TOKEN from the SERVER."
+  (setf (eglot--progress-reporter-alist server)
+        (assoc-delete-all token (eglot--progress-reporter-alist server))))
+
+(cl-defmethod eglot-handle-notification
+  (server (_method (eql $/progress)) &key token value)
+  "Handle a $/progress notification identified by TOKEN from the SERVER."
+  (when eglot-report-progress
+    (cl-destructuring-bind (&key kind title percentage message) value
+      (pcase kind
+        ("begin" (let* ((prefix (format (concat "[eglot] %s %s:" (when percentage " "))
+                                        (eglot-project-nickname server) token))
+                        (pr (if percentage
+                                (make-progress-reporter prefix 0 100 percentage 1 0)
+                              (make-progress-reporter prefix nil nil nil 1 0))))
+                   (eglot--progress-reporter-delete server token)
+                   (setf (eglot--progress-reporter-alist server)
+                         (cons (cons token pr) (eglot--progress-reporter-alist server)))
+                   (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
+        ("report" (when-let ((pr (eglot--progress-reporter server token)))
+                    (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
+        ("end" (when-let ((pr (eglot--progress-reporter server token)))
+                 (progress-reporter-done pr)
+                 (eglot--progress-reporter-delete server token)))))))
+
 (cl-defmethod eglot-handle-notification
   (_server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
            &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
@@ -2172,7 +2216,7 @@ eglot--TextDocumentItem
   (append
    (eglot--VersionedTextDocumentIdentifier)
    (list :languageId
-	 (eglot--language-id (eglot--current-server-or-lose))
+         (eglot--language-id (eglot--current-server-or-lose))
          :text
          (eglot--widening
           (buffer-substring-no-properties (point-min) (point-max))))))
-- 
2.38.1

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

[Eli Zaretskii]

> Cc: 59149@debbugs.gnu.org, joaotavora@gmail.com
> Date: Fri, 25 Nov 2022 11:15:33 -0500
> From:  Danny Freeman via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> +(defcustom eglot-report-progress t
> +  "If non-nil, show progress of long running server work in the minibuffer."
> +  :type 'boolean)

Please add a :version tag.

Thanks.

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

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

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


Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 59149@debbugs.gnu.org, joaotavora@gmail.com
>> Date: Fri, 25 Nov 2022 11:15:33 -0500
>> From:  Danny Freeman via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> +(defcustom eglot-report-progress t
>> +  "If non-nil, show progress of long running server work in the minibuffer."
>> +  :type 'boolean)
>
> Please add a :version tag.
>
> Thanks.

My bad, attached is a revised patch.

-- 
Danny Freeman


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Eglot progress report, now with version number! --]
[-- Type: text/x-patch, Size: 5596 bytes --]

From 95a7ca04d1c3b573cbc1cba3378f50450f090ab4 Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Wed, 9 Nov 2022 08:46:45 -0500
Subject: [PATCH] Eglot: Display progress notifications in minibuffer as they
 arrive

* lisp/progmodes/eglot.el (eglot-report-progress): New custom variable.
(eglot-lsp-server): New slot for tracking active progress reporters.
(eglot-handle-notification): New impl for $/progress server responses.
(eglot--format-markup): replace tab with spaces.
(eglot--TextDocumentItem): replace tab with spaces.

The LSP spec describes methods for reporting progress on long running
jobs to the client:

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#progress
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workDoneProgress

This change reports those notifications in the minibuffer as they come
in. It shows a percent indicator (if the server provides theme), or a
spinner. This change should open the door for writing a "cancel long
running request" command, which are identified by these progress
notifications. See
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_workDoneProgress_cancel
---
 lisp/progmodes/eglot.el | 49 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index a0fb253e10..1c991e5c35 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -386,6 +386,11 @@ eglot-menu-string
   "String displayed in mode line when Eglot is active."
   :type 'string)
 
+(defcustom eglot-report-progress t
+  "If non-nil, show progress of long running server work in the minibuffer."
+  :type 'boolean
+  :version "29.1")
+
 (defvar eglot-withhold-process-id nil
   "If non-nil, Eglot will not send the Emacs process id to the language server.
 This can be useful when using docker to run a language server.")
@@ -831,6 +836,9 @@ eglot-lsp-server
    (project
     :documentation "Project associated with server."
     :accessor eglot--project)
+   (progress-reporter-alist
+    :documentation "Alist of (PROGRESS-TOKEN . PROGRESS-REPORTER)."
+    :accessor eglot--progress-reporter-alist)
    (inhibit-autoreconnect
     :initform t
     :documentation "Generalized boolean inhibiting auto-reconnection if true."
@@ -1569,7 +1577,7 @@ eglot--format-markup
       (setq-local markdown-fontify-code-blocks-natively t)
       (insert string)
       (let ((inhibit-message t)
-	    (message-log-max nil))
+            (message-log-max nil))
         (ignore-errors (delay-mode-hooks (funcall mode))))
       (font-lock-ensure)
       (string-trim (buffer-string)))))
@@ -2049,6 +2057,43 @@ eglot-handle-notification
   (_server (_method (eql telemetry/event)) &rest _any)
   "Handle notification telemetry/event.") ;; noop, use events buffer
 
+(defun eglot--progress-report-message (title message)
+  "Format a $/progress report message, given a TITLE and/or MESSAGE string."
+  (cond
+   ((and title message) (format "%s %s" title message))
+   (title title)
+   (message message)))
+
+(defun eglot--progress-reporter (server token)
+  "Get a prgress-reporter identified by the progress TOKEN from the SERVER ."
+  (cdr (assoc token (eglot--progress-reporter-alist server))))
+
+(defun eglot--progress-reporter-delete (server token)
+  "Delete progress-reporters identified by the progress TOKEN from the SERVER."
+  (setf (eglot--progress-reporter-alist server)
+        (assoc-delete-all token (eglot--progress-reporter-alist server))))
+
+(cl-defmethod eglot-handle-notification
+  (server (_method (eql $/progress)) &key token value)
+  "Handle a $/progress notification identified by TOKEN from the SERVER."
+  (when eglot-report-progress
+    (cl-destructuring-bind (&key kind title percentage message) value
+      (pcase kind
+        ("begin" (let* ((prefix (format (concat "[eglot] %s %s:" (when percentage " "))
+                                        (eglot-project-nickname server) token))
+                        (pr (if percentage
+                                (make-progress-reporter prefix 0 100 percentage 1 0)
+                              (make-progress-reporter prefix nil nil nil 1 0))))
+                   (eglot--progress-reporter-delete server token)
+                   (setf (eglot--progress-reporter-alist server)
+                         (cons (cons token pr) (eglot--progress-reporter-alist server)))
+                   (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
+        ("report" (when-let ((pr (eglot--progress-reporter server token)))
+                    (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
+        ("end" (when-let ((pr (eglot--progress-reporter server token)))
+                 (progress-reporter-done pr)
+                 (eglot--progress-reporter-delete server token)))))))
+
 (cl-defmethod eglot-handle-notification
   (_server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
            &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
@@ -2172,7 +2217,7 @@ eglot--TextDocumentItem
   (append
    (eglot--VersionedTextDocumentIdentifier)
    (list :languageId
-	 (eglot--language-id (eglot--current-server-or-lose))
+         (eglot--language-id (eglot--current-server-or-lose))
          :text
          (eglot--widening
           (buffer-substring-no-properties (point-min) (point-max))))))
-- 
2.38.1

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

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

I meant to reply to this and Eli's feedback in one email.

> OK.  Let's have Danny's opinion on it, especially on the name
> 'eglot-report-progress'.  In the simplest case, this variable is a
> boolean, but I am curious if other types of progress reporting can be
> envisioned.  If they are, they should be controlled by the least amount
> of user variables possible.

I think a boolean will be sufficient, but I'm not sure how this will
evolve in the future.

From the perspective of the LSP specification, there is only ONE type of
progress message defined in the spec, and that is "Work Done Progress",
which is what my patch supports. The spec does leave room for other
usage of the progress message, including for "streaming of results",
which I have never seen used in practice.

From the spec:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#progress
"""
The base protocol offers also support to report progress in a generic
fashion. This mechanism can be used to report any kind of progress
including work done progress (usually used to report progress in the
user interface using a progress bar) and partial result progress to
support streaming of results.
"""

What kind of variable might allow Eglot to naturally evolve with this
part of the spec? Maybe a list similar to `eglot-stay-out-of`?

It may also help to keep in mind that some of these "Work Done Progress"
requests can be canceled by the user. Later I think it would be nice to
expose a command that would send the server a cancel request while one
of these "progress" things is in flight. I'm not sure how that might
affect a custom var here, but thought I would mention it.

-- 
Danny Freeman

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

[João Távora]

Danny Freeman <danny@dfreeman.email> writes:


> +(defcustom eglot-report-progress t
> +  "If non-nil, show progress of long running server work in the minibuffer."
> +  :type 'boolean
> +  :version "29.1")

The usual question: is the version number here Eglot, the ELPA package's
upcoming version number, or is it Emacs's upcoming version number.  I
think Stefan Kangas had something to say about that.

> +
> +            (message-log-max nil))

I know that indentation was off here, and you fixed it.  But please
revert this, and feel free to offer a separate cosmetic patch fixing
this and other violations.

>          (ignore-errors (delay-mode-hooks (funcall mode))))
>        (font-lock-ensure)
>        (string-trim (buffer-string)))))
> @@ -2049,6 +2057,43 @@ eglot-handle-notification
>    (_server (_method (eql telemetry/event)) &rest _any)
>    "Handle notification telemetry/event.") ;; noop, use events buffer
>  
> +(defun eglot--progress-report-message (title message)
> +  "Format a $/progress report message, given a TITLE and/or MESSAGE string."
> +  (cond
> +   ((and title message) (format "%s %s" title message))
> +   (title title)
> +   (message message)))
> +
> +(defun eglot--progress-reporter (server token)
> +  "Get a prgress-reporter identified by the progress TOKEN from the SERVER ."
> +  (cdr (assoc token (eglot--progress-reporter-alist server))))
> +
> +(defun eglot--progress-reporter-delete (server token)
> +  "Delete progress-reporters identified by the progress TOKEN from the SERVER."
> +  (setf (eglot--progress-reporter-alist server)
> +        (assoc-delete-all token (eglot--progress-reporter-alist > server))))

This is just a stylistic suggestion, but these functions could all be
local inside the following eglot-handle-notification.  Then you could
give them less mouthfully names.  The whole progress stuff could be
examined by looking only at one function.

> +
> +(cl-defmethod eglot-handle-notification
> +  (server (_method (eql $/progress)) &key token value)
> +  "Handle a $/progress notification identified by TOKEN from the SERVER."
> +  (when eglot-report-progress
> +    (cl-destructuring-bind (&key kind title percentage message) value

I think eglot-dbind is more appropriate here.  See the file for how it's
used.

> +      (pcase kind
> +        ("begin" (let* ((prefix (format (concat "[eglot] %s %s:" (when percentage " "))
> +                                        (eglot-project-nickname server) token))
> +                        (pr (if percentage
> +                                (make-progress-reporter prefix 0 100 percentage 1 0)
> +                              (make-progress-reporter prefix nil nil nil 1 0))))
> +                   (eglot--progress-reporter-delete server token)
> +                   (setf (eglot--progress-reporter-alist server)
> +                         (cons (cons token pr) (eglot--progress-reporter-alist server)))
> +                   (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
> +        ("report" (when-let ((pr (eglot--progress-reporter server token)))
> +                    (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
> +        ("end" (when-let ((pr (eglot--progress-reporter server token)))
> +                 (progress-reporter-done pr)
> +                 (eglot--progress-reporter-delete server token)))))))

This lines are a bit too long, I think.  Try to stick to 80 columns.
M-x whitespace-mode helps in seeing that.

I also think this could probably be simplified a bit or jumbled around
to feel less repetitive.  But it's not really bad as it stands, so feel
free to disregard.

> +
>  (cl-defmethod eglot-handle-notification
>    (_server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
>             &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
> @@ -2172,7 +2217,7 @@ eglot--TextDocumentItem
>    (append
>     (eglot--VersionedTextDocumentIdentifier)
>     (list :languageId
> -	 (eglot--language-id (eglot--current-server-or-lose))
> +         (eglot--language-id (eglot--current-server-or-lose))

Same here re: indentation.

>           :text
>           (eglot--widening
>            (buffer-substring-no-properties (point-min) (point-max))))))


The patch looks generally good: thanks!  If you want to do the fixes I
suggested, go ahead.  Else give me some days to test it and I will
push it.

João

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

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

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


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

> Danny Freeman <danny@dfreeman.email> writes:
>
>
>> +(defcustom eglot-report-progress t
>> +  "If non-nil, show progress of long running server work in the minibuffer."
>> +  :type 'boolean
>> +  :version "29.1")
>
> The usual question: is the version number here Eglot, the ELPA package's
> upcoming version number, or is it Emacs's upcoming version number.  I
> think Stefan Kangas had something to say about that.

Yeah, I've left this as in my updated patch. The answer is beyond my
qualifications :)

>
>> +
>> +            (message-log-max nil))
>
> I know that indentation was off here, and you fixed it.  But please
> revert this, and feel free to offer a separate cosmetic patch fixing
> this and other violations.

No problem. I have attached a second patch with only whitespace changes.
Feel free to take it or leave it. 

>
>>          (ignore-errors (delay-mode-hooks (funcall mode))))
>>        (font-lock-ensure)
>>        (string-trim (buffer-string)))))
>> @@ -2049,6 +2057,43 @@ eglot-handle-notification
>>    (_server (_method (eql telemetry/event)) &rest _any)
>>    "Handle notification telemetry/event.") ;; noop, use events buffer
>>  
>> +(defun eglot--progress-report-message (title message)
>> +  "Format a $/progress report message, given a TITLE and/or MESSAGE string."
>> +  (cond
>> +   ((and title message) (format "%s %s" title message))
>> +   (title title)
>> +   (message message)))
>> +
>> +(defun eglot--progress-reporter (server token)
>> +  "Get a prgress-reporter identified by the progress TOKEN from the SERVER ."
>> +  (cdr (assoc token (eglot--progress-reporter-alist server))))
>> +
>> +(defun eglot--progress-reporter-delete (server token)
>> +  "Delete progress-reporters identified by the progress TOKEN from the SERVER."
>> +  (setf (eglot--progress-reporter-alist server)
>> +        (assoc-delete-all token (eglot--progress-reporter-alist > server))))
>
> This is just a stylistic suggestion, but these functions could all be
> local inside the following eglot-handle-notification.  Then you could
> give them less mouthfully names.  The whole progress stuff could be
> examined by looking only at one function.

I have inlined them with cl-flet.

>> +
>> +(cl-defmethod eglot-handle-notification
>> +  (server (_method (eql $/progress)) &key token value)
>> +  "Handle a $/progress notification identified by TOKEN from the SERVER."
>> +  (when eglot-report-progress
>> +    (cl-destructuring-bind (&key kind title percentage message) value
>
> I think eglot-dbind is more appropriate here.  See the file for how it's
> used.

Done, that's a pretty neat macro.

>> +      (pcase kind
>> +        ("begin" (let* ((prefix (format (concat "[eglot] %s %s:" (when percentage " "))
>> +                                        (eglot-project-nickname server) token))
>> +                        (pr (if percentage
>> +                                (make-progress-reporter prefix 0 100 percentage 1 0)
>> +                              (make-progress-reporter prefix nil nil nil 1 0))))
>> +                   (eglot--progress-reporter-delete server token)
>> +                   (setf (eglot--progress-reporter-alist server)
>> +                         (cons (cons token pr) (eglot--progress-reporter-alist server)))
>> +                   (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
>> +        ("report" (when-let ((pr (eglot--progress-reporter server token)))
>> +                    (progress-reporter-update pr percentage (eglot--progress-report-message title message))))
>> +        ("end" (when-let ((pr (eglot--progress-reporter server token)))
>> +                 (progress-reporter-done pr)
>> +                 (eglot--progress-reporter-delete server token)))))))
>
> This lines are a bit too long, I think.  Try to stick to 80 columns.
> M-x whitespace-mode helps in seeing that.

I've scaled back the width of the function a good bit. Some of the
parens still spill over in some places, happy to take a second stab at
it.

> I also think this could probably be simplified a bit or jumbled around
> to feel less repetitive.  But it's not really bad as it stands, so feel
> free to disregard.

I'm not sure what I could do to scale this back beyond what is there
now. If there is anything specific let me know.

>> +
>>  (cl-defmethod eglot-handle-notification
>>    (_server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
>>             &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
>> @@ -2172,7 +2217,7 @@ eglot--TextDocumentItem
>>    (append
>>     (eglot--VersionedTextDocumentIdentifier)
>>     (list :languageId
>> -	 (eglot--language-id (eglot--current-server-or-lose))
>> +         (eglot--language-id (eglot--current-server-or-lose))
>
> Same here re: indentation.

This was one of the lines that had a tab, I was more intentional with
the indentation in the whitespace patch.

>>           :text
>>           (eglot--widening
>>            (buffer-substring-no-properties (point-min) (point-max))))))
>
>
> The patch looks generally good: thanks!  If you want to do the fixes I
> suggested, go ahead.  Else give me some days to test it and I will
> push it.
>
> João

Thank you! Let me know if you have any other questions or feedback and I
will be happy to address it.

-- 
Danny Freeman


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: whitespace only patch --]
[-- Type: text/x-patch, Size: 2637 bytes --]

From 7227f9fdf53a1af7c3793c94904e153d49527372 Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Sat, 26 Nov 2022 13:31:50 -0500
Subject: [PATCH 1/2] ; eglot whitespace cleanup

* lisp/progmodes/eglot (eglot--check-object): trailing whitespace.
(eglot--format-markup): replace tab with spaces.
(eglot--make-diag): trailing whitespace.
(eglot--TextDocumentItem): replace tab with spaces.
(eglot--lsp-xrefs-for-method): trailing whitespace.
---
 lisp/progmodes/eglot.el | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 120d4feb95..e6e97fd8c8 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -540,7 +540,7 @@ eglot--check-object
        for type = (or (cdr (assoc k types)) t) ;; FIXME: enforce nil type?
        unless (cl-typep v type)
        do (eglot--error "A `%s' must have a %s as %s, but has %s"
-                        interface-name )))
+                        interface-name)))
     t))
 
 (eval-and-compile
@@ -1569,7 +1569,7 @@ eglot--format-markup
       (setq-local markdown-fontify-code-blocks-natively t)
       (insert string)
       (let ((inhibit-message t)
-	    (message-log-max nil))
+            (message-log-max nil))
         (ignore-errors (delay-mode-hooks (funcall mode))))
       (font-lock-ensure)
       (string-trim (buffer-string)))))
@@ -1987,7 +1987,7 @@ 'eglot--make-diag
 (defalias 'eglot--diag-data 'flymake-diagnostic-data)
 
 (cl-loop for i from 1
-         for type in '(eglot-note eglot-warning eglot-error )
+         for type in '(eglot-note eglot-warning eglot-error)
          do (put type 'flymake-overlay-control
                  `((mouse-face . highlight)
                    (priority . ,(+ 50 i))
@@ -2172,7 +2172,7 @@ eglot--TextDocumentItem
   (append
    (eglot--VersionedTextDocumentIdentifier)
    (list :languageId
-	 (eglot--language-id (eglot--current-server-or-lose))
+         (eglot--language-id (eglot--current-server-or-lose))
          :text
          (eglot--widening
           (buffer-substring-no-properties (point-min) (point-max))))))
@@ -2641,7 +2641,7 @@ eglot--lsp-xrefs-for-method
                                                uri range))))))
        (if (vectorp response) response (and response (list response)))))))
 
-(cl-defun eglot--lsp-xref-helper (method &key extra-params capability )
+(cl-defun eglot--lsp-xref-helper (method &key extra-params capability)
   "Helper for `eglot-find-declaration' & friends."
   (let ((eglot--lsp-xref-refs (eglot--lsp-xrefs-for-method
                                method
-- 
2.38.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: updated progress reporter patch --]
[-- Type: text/x-patch, Size: 5183 bytes --]

From a4d7c5028d49cd5fd6d5599e91d2bcfb250851dc Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Wed, 9 Nov 2022 08:46:45 -0500
Subject: [PATCH 2/2] Eglot: Display progress notifications in minibuffer as
 they arrive

* lisp/progmodes/eglot.el (eglot-report-progress): New custom variable.
(eglot-lsp-server): New slot for tracking active progress reporters.
(eglot-handle-notification): New impl for $/progress server responses.

The LSP spec describes methods for reporting progress on long running
jobs to the client:

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#progress
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workDoneProgress

This change reports those notifications in the minibuffer as they come
in. It shows a percent indicator (if the server provides theme), or a
spinner. This change should open the door for writing a "cancel long
running request" command, which are identified by these progress
notifications. See
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_workDoneProgress_cancel
---
 lisp/progmodes/eglot.el | 50 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index e6e97fd8c8..e6673c37a1 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -386,6 +386,11 @@ eglot-menu-string
   "String displayed in mode line when Eglot is active."
   :type 'string)
 
+(defcustom eglot-report-progress t
+  "If non-nil, show progress of long running server work in the minibuffer."
+  :type 'boolean
+  :version "29.1")
+
 (defvar eglot-withhold-process-id nil
   "If non-nil, Eglot will not send the Emacs process id to the language server.
 This can be useful when using docker to run a language server.")
@@ -470,6 +475,7 @@ eglot--executable-find
       (TextDocumentEdit (:textDocument :edits) ())
       (TextEdit (:range :newText))
       (VersionedTextDocumentIdentifier (:uri :version) ())
+      (WorkDoneProgress (:kind) (:title :message :percentage :cancellable))
       (WorkspaceEdit () (:changes :documentChanges))
       (WorkspaceSymbol (:name :kind) (:containerName :location :data)))
     "Alist (INTERFACE-NAME . INTERFACE) of known external LSP interfaces.
@@ -831,6 +837,9 @@ eglot-lsp-server
    (project
     :documentation "Project associated with server."
     :accessor eglot--project)
+   (progress-reporter-alist
+    :documentation "Alist of (PROGRESS-TOKEN . PROGRESS-REPORTER)."
+    :accessor eglot--progress-reporter-alist)
    (inhibit-autoreconnect
     :initform t
     :documentation "Generalized boolean inhibiting auto-reconnection if true."
@@ -2049,6 +2058,47 @@ eglot-handle-notification
   (_server (_method (eql telemetry/event)) &rest _any)
   "Handle notification telemetry/event.") ;; noop, use events buffer
 
+(cl-defmethod eglot-handle-notification
+  (server (_method (eql $/progress)) &key token value)
+  "Handle a $/progress notification identified by TOKEN from the SERVER."
+  (when eglot-report-progress
+    (cl-flet ((eglot--pr-message (title message)
+                (cond
+                 ((and title message) (format "%s %s" title message))
+                 (title title)
+                 (message message)))
+              (eglot--get-pr (server token)
+                 (cdr (assoc token (eglot--progress-reporter-alist server))))
+              (eglot--delete-pr (server token)
+                (setf (eglot--progress-reporter-alist server)
+                      (assoc-delete-all
+                       token (eglot--progress-reporter-alist server)))))
+      (eglot--dbind ((WorkDoneProgress) kind title percentage message) value
+        (pcase kind
+          ("begin"
+           (let* ((prefix (format (concat "[eglot] %s %s:"
+                                          (when percentage " "))
+                                  (eglot-project-nickname server)
+                                  token))
+                  (pr (if percentage
+                          (make-progress-reporter
+                           prefix 0 100 percentage 1 0)
+                        (make-progress-reporter
+                         prefix nil nil nil 1 0))))
+             (eglot--delete-pr server token)
+             (setf (eglot--progress-reporter-alist server)
+                   (cons (cons token pr) (eglot--progress-reporter-alist server)))
+             (progress-reporter-update
+              pr percentage (eglot--pr-message title message))))
+          ("report"
+           (when-let ((pr (eglot--get-pr server token)))
+             (progress-reporter-update
+              pr percentage (eglot--pr-message title message))))
+          ("end"
+           (when-let ((pr (eglot--get-pr server token)))
+             (progress-reporter-done pr)
+             (eglot--delete-pr server token))))))))
+
 (cl-defmethod eglot-handle-notification
   (_server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
            &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
-- 
2.38.1

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

[Stefan Kangas]

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

>> +(defcustom eglot-report-progress t
>> +  "If non-nil, show progress of long running server work in the minibuffer."
>> +  :type 'boolean
>> +  :version "29.1")
>
> The usual question: is the version number here Eglot, the ELPA package's
> upcoming version number, or is it Emacs's upcoming version number.  I
> think Stefan Kangas had something to say about that.

AFAIU, there are two correct ways to do it:

1. Use :version and the upcoming Emacs version number.  Most :core
   packages do it this way, for example flymake.el.

2. Use :package-version and the upcoming Eglot version number.  This
   requires adding Eglot to `customize-package-emacs-version-alist', and
   then keeping that variable up-to-date as new versions are released.
   See the docstring of the `defcustom' :package-version keyword, and
   `customize-package-emacs-version-alist'.

I think whichever one we pick, we should use it consistently.  Arguably
#1 is a bit easier, but #2 is a bit more correct.  So I think it's
ultimately your decision which way to go for Eglot, João.

Background: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=13824#27

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

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

Sending a friendly reminder in case this patch has fallen off your
radar. 

-- 
Danny Freeman

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

[João Távora]

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

Hi Danny,

Thanks, no it hasn't.  But I have a lot of little things to do and
limited free time. Your patch should be next in the queue,
though.

João

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

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

[João Távora]

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

I just pushed this to the emacs-29 branch, as it's a feature within
a new feature for that branch already.

Danny, I also tweaked the implementation slightly to use a hash table
instead of an alist.  I didn't actually test this, as I have no server handy
that supplies these $/progress notifications, but it should be functionally
equivalent.  let me know if it's not, it's easy to tweak.

Also, I added a missing entry in the eglot.texi manual.

João

On Sat, Dec 3, 2022 at 1:23 PM João Távora <joaotavora@gmail.com> wrote:

> Hi Danny,
>
> Thanks, no it hasn't.  But I have a lot of little things to do and
> limited free time. Your patch should be next in the queue,
> though.
>
> João
>


-- 
João Távora

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

bug#59149: [SPAM UNSURE] Re: bug#59149: Feature Request: Report progress of long requests in Eglot

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

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

> I just pushed this to the emacs-29 branch, as it's a feature within
> a new feature for that branch already.

That is very exciting to hear, thank you!

> Danny, I also tweaked the implementation slightly to use a hash table
> instead of an alist.  I didn't actually test this, as I have no server handy
> that supplies these $/progress notifications, but it should be functionally
> equivalent.  let me know if it's not, it's easy to tweak.
>
> Also, I added a missing entry in the eglot.texi manual.

I just pulled the changes down and tested it. It works great. Thank you
for cleaning it up.

I'm very excited to see this make it in to 29. As always, I appreciate
your help and patience with me.

-- 
Danny Freeman