GNU bug report logs -
#59149
Feature Request: Report progress of long requests in Eglot
Previous Next
Full log
View this message in rfc822 format
[Message part 1 (text/plain, inline)]
João Távora <joaotavora <at> gmail.com> writes:
> Danny Freeman <danny <at> 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
[0001-eglot-whitespace-cleanup.patch (text/x-patch, attachment)]
[0002-Eglot-Display-progress-notifications-in-minibuffer-a.patch (text/x-patch, attachment)]
This bug report was last modified 2 years and 161 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.