bug#81514: 32.0.50; Eglot messages progress reporter shows errors

Stéphane Marks <[email protected]> writes:

I've pushed my simpler patch

   commit cf98d9d725f034b3dd89f6f92a1c8205e778f98c
   Author: João Távora <[email protected]>
   Date:   Sat Aug 1 10:54:10 2026 +0100
    
     Eglot: do more orthodox use of progress reporters (bug#81514)
     
     The progress reporters on recent Emacs has backward incompatible
     changes, but our use of them wasn't too correct either.

but please read on.

>  The previous reporter or the fixed one?  And before or
>  after your changes? 
> It is good the system-taskbar progress bar displays but it is bad that
> it pulses for a known 0-100 progress reporter

What do you mean by "pulsing"?  It's been a while since I've cared about
these OSX/Windows/GUI paraphernalia.  You seem to mean "pulses when it
should be xxxing".  What is xxx?

> and very bad that the stateful system-taskbar progress bars need to
> disappear when percent gets to 100 (or times out or whatever) but
> don't disappear, sitting at the last remaining reported percentage.

Understood.

>  Anyway, can you try the simpler patch after my sig?  It should create
>  the reporter with 0-100 and also call 'progress-reporter-done' at the
>  appropriate time (and be a bit less intrusive).
>
> The only issue with the simpler patch is that if the LSP server sends "end" without first reporting 100, any stateful
> progress reporter back ends like system-taskbar's are never cleared.  

If the server sends 'end' and no percentage my patch (which btw I've just
pushed to emacs-31) throws a 100 in there and you should get a call to
progress-reporter-done.  What else is missing there?

> Same if the LSP server dies.  Are those conditions we should account
> for?

The first condition you mention I've tried to address.  As to the
"server death", I don't think either of our patches handles it.  What is
an appropriate cleanup function for a progress reporter, just a
'progress-reporter-done' or something else?

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 0f0791d37ab..285041653d1 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1491,6 +1491,11 @@ eglot--on-shutdown
          (lambda (x) (eq server
                          (get-text-property 0 'eglot--server (car x))))
          flymake-list-only-diagnostics))
+  ;; Cleanup progress reporters
+  (maphash (lambda (_ r)
+             (unless (eq (car r) 'eglot--mode-line-reporter )
+               (progress-reporter-done r)))
+           (eglot--progress-reporters server))
   (cond ((eglot--shutdown-requested server)
          t)
         ((not (eglot--inhibit-autoreconnect server))

> Did you want to address Microsoft's suggestion to ignore erroneous
> progress values?  Clearly, they know that LSP servers misbehave and/or
> are misimplemented.

I'll protect the code if I find actual evidence of misbehavior.  Up to
now, no such reports existed.  In fact this bug was bisected to your
changes to the progress reporters ;-) But feel please conjecture a
hypothetical scenario where we both can clearly tell there would be
(serious?) misbehavior.

> For the sake of documentation posterity, I'll include the official LSP
> documentation reference here.
>
> https://github.com/microsoft/language-server-protocol/blob/b7f5132c95261c0898ae5124e7a91707abc48fcd/_specifications/specification-3-16.md?plain=1#L1455
>
>     The value range is [0, 100]. The value should be steadily
> rising. Clients are free to ignore values that are not following this
> rule.

Why do you keep highlighting that clients are free to ignore this
monotony.  Is it of particular relevance to this bug or proposed patch?

>  Finally, can you spare a sentence or two about what the new progress
>  reporter enables?  Alternatively, point me a relevant NEWS entry?
>
> See
> https://github.com/emacs-mirror/emacs/blob/99a9307235ca7e833744adb19e58473b86fc76e8/etc/NEWS#L4089

Thanks.

João