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" <bug-gnu-emacs@gnu.org>]

[-- 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