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