bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Filippo Argiolas]

Hi,
I've recently been working in some big C project with ton of ifdefs
and inactive code regions.

One of the few annoyances (maybe the last) remaining with emacs+eglot
is the inability to show these regions as grayed out in a LSP aware
way.

This was maybe possibile until now with clangd with semantic tokens
but they've recently introduced a new protocol extension that should
solve this problem in a much easier to implement way.

It's been introduced in clangd 17 with the review [1].

It should be pretty straightforward to implement as it's just a server
notification that sends inactive regions ranges once enabled.

In [2] you can find the pull request that added support for this in
vscode clangd extension.

I did some experiment on my own but I'm still not able to see the
notifications. Probably I'm missing something within eglot code base,
my impression was that enabling the inactiveRegions capability would
get me some Unknown notification message but I wasn't able to see
them.

Anyways it would be great if eglot could support this. Not sure
UI-wise what would be the best approach, vscode patch reduces the
opacity of the inactive regions but rendering them as comment would
also be nice at first.


1. https://reviews.llvm.org/D143974
2. https://github.com/clangd/vscode-clangd/pull/193/commits/ef75f637e7f79f94064369368ca665861836e482

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Philip Kaludercic]

Filippo Argiolas <filippo.argiolas@gmail.com> writes:

> Anyways it would be great if eglot could support this. Not sure
> UI-wise what would be the best approach, vscode patch reduces the
> opacity of the inactive regions but rendering them as comment would
> also be nice at first.

I guess the `shadow' interface would be a viable option as well.

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Felician Nemeth]

Filippo Argiolas <filippo.argiolas@gmail.com> writes:

> [...] This was maybe possibile until now with clangd with semantic
> tokens but they've recently introduced a new protocol extension that
> should solve this problem in a much easier to implement way.

In the past, João wasn't keen on supporting non-standard features in
Eglot.  I Cc'd him anyway as he is the maintainer of Eglot.

> Not sure UI-wise what would be the best approach, vscode patch reduces
> the opacity of the inactive regions but rendering them as comment
> would also be nice at first.

Maybe hide-ifdef-mode is good for providing inspiration.

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Filippo Argiolas]

On Mon, Aug 21, 2023 at 9:04 PM Felician Nemeth
<felician.nemeth@gmail.com> wrote:
>
> Filippo Argiolas <filippo.argiolas@gmail.com> writes:
>
> > [...] This was maybe possibile until now with clangd with semantic
> > tokens but they've recently introduced a new protocol extension that
> > should solve this problem in a much easier to implement way.
>
> In the past, João wasn't keen on supporting non-standard features in
> Eglot.  I Cc'd him anyway as he is the maintainer of Eglot.

Let's wait for feedback from João then. I'd say this could warrant an
exception on the basis that disabling code with the preprocessor is
something specific enough to C/C++. It makes sense to have it as a
server extension.

Also this IMHO would solve quite an important problem with C
development, not sure if it's worth waiting while we could solve it
now with the extension and move to the standard protocol if and once
the LSP spec will support this.

> > Not sure UI-wise what would be the best approach, vscode patch reduces
> > the opacity of the inactive regions but rendering them as comment
> > would also be nice at first.
>
> Maybe hide-ifdef-mode is good for providing inspiration.

No idea how easy that would be and if that makes sense but we could
maybe follow the eglot philosophy of working with existing components
and just forward inactive regions to hide-ifdef-mode?

Filippo

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[João Távora]

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

On Tue, Aug 22, 2023, 08:09 Filippo Argiolas <filippo.argiolas@gmail.com>
wrote:

> In the past, João wasn't keen on supporting non-standard features in
> > Eglot.  I Cc'd him anyway as he is the maintainer of Eglot.
>

Right, but I'm not for making it hard either. Although this is
non-standard, it's similar to a bunch of "code-painting" situations, some
of which are standard, and some of which, like inlay hints are already in
Eglot.

So it shouldn't be a problem in itself to supply support for this in
c-ts-mode or some eglot-c-extra.el or, if done well enough, even an example
snippet of how to support a typical extension in your .emacs.

I'm more worried that this isn't even out yet. Afaik Filippo you compiled a
Clangd 17 with a patch, right? I have done that in the past, but it's not
very practical every time, so either we wait for this to stabilize or you
have to tell me where to grab the patched Clangd and llvm toolchain
somewhere.

Also I'm not 100% happy with my inlay hints implementation based on jit
font lock, which misses the mark more often than I wished. I had an
earlier, less efficient (but afair more correct) implementation before Eli
suggested this one. The fault is not 100% on Emacs side though, and the LSP
spec itself leaves much to be desired regarding invalidating regions. This
is an opportunity to revisit these matters.

Also this IMHO would solve quite an important problem with C
> development, not sure if it's worth waiting while we could solve it
> now with the extension and move to the standard protocol if and once
> the LSP spec will support this.
>

I'm also personally interested in this feature. But how likely is it that
this makes it into the LSP standard, in your opinion?

FWIW, other languages have similar features. Common Lisp has read-time
conditionals for example, which are similar if not identical in function
(and obviously not as rotten as C macros).

By the way, if you didn't know this silly trick, if you're in a #ifdef web,
a half-decent way to know whether a given point is active is to try and
find a definition inside it or type some syntactically correct code. If
Eglot jumps to target or highlights variable names, the region is active,
else it probably isn't.

No idea how easy that would be and if that makes sense but we could
> maybe follow the eglot philosophy of working with existing components
> and just forward inactive regions to hide-ifdef-mode?
>

Yes, that is the philosophy, but these "standard components" differ in
quality immensely. So it's on a case-by-case.

João

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

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Filippo Argiolas]

On Tue, Aug 22, 2023 at 10:56 AM João Távora <joaotavora@gmail.com> wrote:
> I'm more worried that this isn't even out yet. Afaik Filippo you compiled a Clangd 17 with a patch, right? I have done that in the past, but it's not very practical every time, so either we wait for this to stabilize or you have to tell me where to grab the patched Clangd and llvm toolchain somewhere.

It's much easier than that! They release unstable git snapshots in
github. You can find them at https://github.com/clangd/clangd/releases
Clangd is released as a static binary you can just copy in your path
(e.g. in ~/.local/bin). From what I can tell you don't need to upgrade
the whole toolchain, the binary runs fine on its own. Latest one for
linux is at https://github.com/clangd/clangd/releases/download/snapshot_20230820/clangd-linux-snapshot_20230820.zip

It would be great if you could test it and see if you can get server
notifications for inactiveRegions.

I think the interface should be already stable, reading the review the
only thing that is probably going to change at some point is dropping
the support to generate semantic tokens for inactive regions as
comments, which they are keeping for backwards compatibility.

>> Also this IMHO would solve quite an important problem with C
>> development, not sure if it's worth waiting while we could solve it
>> now with the extension and move to the standard protocol if and once
>> the LSP spec will support this.
>
> I'm also personally interested in this feature. But how likely is it that this makes it into the LSP standard, in your opinion?

Honestly I don't know :-) Only information I could find is this vscode
issue https://github.com/microsoft/vscode/issues/123352 where they
seemed to agree it's not something that belongs in semantic tokens but
then the discussion stalled.


> FWIW, other languages have similar features. Common Lisp has read-time conditionals for example, which are similar if not identical in function (and obviously not as rotten as C macros).
>
> By the way, if you didn't know this silly trick, if you're in a #ifdef web, a half-decent way to know whether a given point is active is to try and find a definition inside it or type some syntactically correct code. If Eglot jumps to target or highlights variable names, the region is active, else it probably isn't.

Thanks! that's actually a great way to see if the highlighted code is
active! Still greying it out or dimming the colors would be great to
see at a glance if a region is dead, especially for big regions. It's
also one of those few features that Emacs still misses when compared
to vscode.

Filippo

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[João Távora]

Filippo Argiolas <filippo.argiolas@gmail.com> writes:

> On Tue, Aug 22, 2023 at 10:56 AM João Távora <joaotavora@gmail.com> wrote:
>> I'm more worried that this isn't even out yet. Afaik Filippo you
>> compiled a Clangd 17 with a patch, right? I have done that in the
>> past, but it's not very practical every time, so either we wait for
>> this to stabilize or you have to tell me where to grab the patched
>> Clangd and llvm toolchain somewhere.
>
> It's much easier than that! They release unstable git snapshots in
> github. You can find them at https://github.com/clangd/clangd/releases
> Clangd is released as a static binary you can just copy in your path
> (e.g. in ~/.local/bin). From what I can tell you don't need to upgrade
> the whole toolchain, the binary runs fine on its own. Latest one for
> linux is at
> https://github.com/clangd/clangd/releases/download/snapshot_20230820/clangd-linux-snapshot_20230820.zip
>
> It would be great if you could test it and see if you can get server
> notifications for inactiveRegions.

OK, after fetching that git snapshot today, I've done this:

   ;;; eglot-clangd-inactive-region.el -*- lexical-binding: t; -*-
    
   (require 'eglot)
   (require 'cl-lib)
    
   (cl-defmethod eglot-client-capabilities :around (server)
     (let ((base (cl-call-next-method)))
       (when (cl-find "clangd" (process-command (jsonrpc--process server))
                      :test #'string-match)
         (setf (cl-getf (cl-getf base :textDocument)
                        :inactiveRegionsCapabilities)
               '(:inactiveRegions t)))
       base))
    
   (defvar-local eglot-clangd-inactive-region-overlays '())
    
   (cl-defmethod eglot-handle-notification
     (_server (_method (eql textDocument/inactiveRegions))
              &key regions textDocument &allow-other-keys)
     (if-let* ((path (expand-file-name (eglot--uri-to-path
                                        (cl-getf textDocument :uri))))
               (buffer (find-buffer-visiting path)))
         (with-current-buffer buffer
           (mapc #'delete-overlay eglot-clangd-inactive-region-overlays)
       (cl-loop
        for r across regions
        for (beg . end) = (eglot--range-region r)
        for ov = (make-overlay beg end)
        do
        (overlay-put ov 'face 'shadow)
        (push ov eglot-clangd-inactive-region-overlays)))))

Be sure to evaluate with lexical-binding (easiest if you save it to a
file first).

It's bare-bone but it works, because the method for communicating
"inactive regions" is very basic (and similar to unsolicited
diagnostics).

Only minimally tested, so YMMV.

This serves as a good example of how to support unofficial LSP
extensions using Eglot as an API.  Could well be in the manual.

The method for providing a client-side capability based on a server is
crude.  Servers do identify themselves properly via LSP, but only after
being initialized, so it's too late and I had to use an heuritic based
on the command.  We could also use a proper subclass for clangd servers,
but that's too verbose and overkill IMHO.

The usage of some '--' symbols like 'eglot--uri-to-path' and
'eglot--range-region' points out that these symbols should probably be
promoted to be part of the Eglot API.  Patches welcome for that.

João

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Filippo Argiolas]

On Fri, Aug 25, 2023 at 2:16 PM João Távora <joaotavora@gmail.com> wrote:
>
> Filippo Argiolas <filippo.argiolas@gmail.com> writes:
>
> OK, after fetching that git snapshot today, I've done this:
>
> It's bare-bone but it works, because the method for communicating
> "inactive regions" is very basic (and similar to unsolicited
> diagnostics).
>
> Only minimally tested, so YMMV.
>
> This serves as a good example of how to support unofficial LSP
> extensions using Eglot as an API.  Could well be in the manual.
>
> The method for providing a client-side capability based on a server is
> crude.  Servers do identify themselves properly via LSP, but only after
> being initialized, so it's too late and I had to use an heuritic based
> on the command.  We could also use a proper subclass for clangd servers,
> but that's too verbose and overkill IMHO.

That's great! Definitely owe you a beer or a bottle of your favorite beverage!
Still testing it but so far it seems to work perfectly! Love that
clangd is smart enough to classify as inactive also other stuff beyond
ifdefs, like headers included but not used.

About the heuristic would it be that bad to just include
inactiveRegions in the general client capabilities? Guess it would be
just ignored by other servers not supporting it, wouldn't it? Kind of
surprised clangd doesn't use some kind of namespacing convention for
their protocol extensions.

Anyway, it would be great if this could be a part of eglot but I can
understand being careful given it's not in the standard protocol and
not even in a released clangd yet.
It would be great though if this example was included in the docs. It
says a lot about how easy to extend and well designed eglot is.

One thing about UI, all the themes I tried seem to render shadow as
grey-ish but it was my impression reading the docs that it would be a
dim version of the current face, so it would still have syntax
highlighting. Is it just a theme limitation (probably because shadow
wasn't used for something like this before) or it's not technically
possible?

Thanks a lot again,
Filippo

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[João Távora]

Filippo Argiolas <filippo.argiolas@gmail.com> writes:

> On Fri, Aug 25, 2023 at 2:16 PM João Távora <joaotavora@gmail.com> wrote:
>>
>> Filippo Argiolas <filippo.argiolas@gmail.com> writes:
>>
>> OK, after fetching that git snapshot today, I've done this:
>>
>> It's bare-bone but it works, because the method for communicating
>> "inactive regions" is very basic (and similar to unsolicited
>> diagnostics).
>>
>> Only minimally tested, so YMMV.
>>
>> This serves as a good example of how to support unofficial LSP
>> extensions using Eglot as an API.  Could well be in the manual.
>>
>> The method for providing a client-side capability based on a server is
>> crude.  Servers do identify themselves properly via LSP, but only after
>> being initialized, so it's too late and I had to use an heuritic based
>> on the command.  We could also use a proper subclass for clangd servers,
>> but that's too verbose and overkill IMHO.
>
> That's great! Definitely owe you a beer or a bottle of your favorite
> beverage!

Glad to help.  You're lucky I'm not some kind of wine connoisseur ;-)

> About the heuristic would it be that bad to just include
> inactiveRegions in the general client capabilities? Guess it would be
> just ignored by other servers not supporting it, wouldn't it?

Yes, but you would still need the extra add-on code.  It just doesn't
belong in Eglot.  It could belong in cc-mode.el (as could
eglot-server-programs's clangd entry, btw) but I heavily doubt the
author of that package would accept it.  The brand new c++-ts-mode and
c-ts-mode is a different story though.  Or it could just be a separate
file to require.  Or a separate GNU ELPA package.

> Kind of surprised clangd doesn't use some kind of namespacing
> convention for their protocol extensions.

Yes.  What's worse, it supports multiple different versions outside and
inside of the standard of the same feature.  I've just seen this with
the textDocument/typeHierarchy method, which is now officially called
textDocument/prepareTypeHierarchy.  So I don't want to add two methods
to support basically the same thing to Eglot.

> Anyway, it would be great if this could be a part of eglot but I can
> understand being careful given it's not in the standard protocol and
> not even in a released clangd yet.
> It would be great though if this example was included in the docs. It
> says a lot about how easy to extend and well designed eglot is.

Make a patch for that (and remember to also include the exportation of
the '--' symbols).

> One thing about UI, all the themes I tried seem to render shadow as
> grey-ish but it was my impression reading the docs that it would be a
> dim version of the current face, so it would still have syntax
> highlighting. Is it just a theme limitation (probably because shadow
> wasn't used for something like this before) or it's not technically
> possible?

I'm fairly sure it's technically possible, even if perhaps not easy.
You can investigate or ask this on emacs-devel.

João

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Filippo Argiolas]

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

On Sun, Aug 27, 2023 at 3:58 PM João Távora <joaotavora@gmail.com> wrote:
> Glad to help.  You're lucky I'm not some kind of wine connoisseur ;-)
> Make a patch for that (and remember to also include the exportation of
> the '--' symbols).
>

Many thanks again! That code definitely made this work week more enjoyable ;-)

Sorry for the delay with the patch, little time left these days for
side projects or even trivial contributions like this.
See if the attached patches could do!

> > One thing about UI, all the themes I tried seem to render shadow as
> > grey-ish but it was my impression reading the docs that it would be a
> > dim version of the current face, so it would still have syntax
> > highlighting. Is it just a theme limitation (probably because shadow
> > wasn't used for something like this before) or it's not technically
> > possible?
>
> I'm fairly sure it's technically possible, even if perhaps not easy.
> You can investigate or ask this on emacs-devel.

Agreed, will move the discussion about this there.

Filippo

[-- Attachment #2: 0002-Promote-eglot-methods-used-in-examples-to-public.patch --]
[-- Type: application/octet-stream, Size: 8948 bytes --]

From 18340104293ef004c38305ac99839e5ca9faedc5 Mon Sep 17 00:00:00 2001
From: Filippo Argiolas <filippo.argiolas@gmail.com>
Date: Thu, 31 Aug 2023 19:09:47 +0200
Subject: [PATCH 2/2] Promote eglot methods used in examples to public

* lisp/progmodes/eglot.el: promote eglot--uri-to-path and
eglot--range-region to public API as they're being used in eglot docs
as example about extending eglot functionality

* doc/misc/eglot.texi: update Extending Eglot example with new public methods
---
 doc/misc/eglot.texi     |  2 +-
 lisp/progmodes/eglot.el | 32 ++++++++++++++++----------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/doc/misc/eglot.texi b/doc/misc/eglot.texi
index ed1456725ae..dc668ae7960 100644
--- a/doc/misc/eglot.texi
+++ b/doc/misc/eglot.texi
@@ -1316,7 +1316,7 @@ Extending Eglot
 (cl-defmethod eglot-handle-notification
   (_server (_method (eql textDocument/inactiveRegions))
            &key regions textDocument &allow-other-keys)
-  (if-let* ((path (expand-file-name (eglot--uri-to-path
+  (if-let* ((path (expand-file-name (eglot-uri-to-path
                                      (cl-getf textDocument :uri))))
             (buffer (find-buffer-visiting path)))
       (with-current-buffer buffer
diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 65daa0941d5..3190a9fe762 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1671,7 +1671,7 @@ eglot--path-to-uri
                eglot--uri-path-allowed-chars)))))
 
 (declare-function w32-long-file-name "w32proc.c" (fn))
-(defun eglot--uri-to-path (uri)
+(defun eglot-uri-to-path (uri)
   "Convert URI to file path, helped by `eglot--current-server'."
   (when (keywordp uri) (setq uri (substring (symbol-name uri) 1)))
   (let* ((server (eglot-current-server))
@@ -1780,7 +1780,7 @@ eglot--server-capable-or-lose
                     (mapconcat #'symbol-name feats " ")))
     retval))
 
-(defun eglot--range-region (range &optional markers)
+(defun eglot-range-region (range &optional markers)
   "Return region (BEG . END) that represents LSP RANGE.
 If optional MARKERS, make markers."
   (let* ((st (plist-get range :start))
@@ -2291,7 +2291,7 @@ eglot-handle-notification
                     (t          'eglot-note)))
             (mess (source code message)
               (concat source (and code (format " [%s]" code)) ": " message)))
-    (if-let* ((path (expand-file-name (eglot--uri-to-path uri)))
+    (if-let* ((path (expand-file-name (eglot-uri-to-path uri)))
               (buffer (find-buffer-visiting path)))
         (with-current-buffer buffer
           (cl-loop
@@ -2303,7 +2303,7 @@ eglot-handle-notification
                        diag-spec
                      (setq message (mess source code message))
                      (pcase-let
-                         ((`(,beg . ,end) (eglot--range-region range)))
+                         ((`(,beg . ,end) (eglot-range-region range)))
                        ;; Fallback to `flymake-diag-region' if server
                        ;; botched the range
                        (when (= beg end)
@@ -2395,7 +2395,7 @@ eglot-handle-request
         (filename))
     (cond
      ((eq external t) (browse-url uri))
-     ((file-readable-p (setq filename (eglot--uri-to-path uri)))
+     ((file-readable-p (setq filename (eglot-uri-to-path uri)))
       ;; Use run-with-timer to avoid nested client requests like the
       ;; "synchronous imenu" floated in bug#62116 presumably caused by
       ;; which-func-mode.
@@ -2408,7 +2408,7 @@ eglot-handle-request
                   (select-frame-set-input-focus (selected-frame)))
                  ((display-buffer (current-buffer))))
            (when selection
-             (pcase-let ((`(,beg . ,end) (eglot--range-region selection)))
+             (pcase-let ((`(,beg . ,end) (eglot-range-region selection)))
                ;; FIXME: it is very naughty to use someone else's `--'
                ;; function, but `xref--goto-char' happens to have
                ;; exactly the semantics we want vis-a-vis widening.
@@ -2643,7 +2643,7 @@ eglot-handle-request
          (mapcar
           (eglot--lambda ((ConfigurationItem) scopeUri section)
             (cl-loop
-             with scope-uri-path = (and scopeUri (eglot--uri-to-path scopeUri))
+             with scope-uri-path = (and scopeUri (eglot-uri-to-path scopeUri))
              for (wsection o)
              on (eglot--workspace-configuration-plist server scope-uri-path)
              by #'cddr
@@ -2774,12 +2774,12 @@ eglot--xref-make-match
   "Like `xref-make-match' but with LSP's NAME, URI and RANGE.
 Try to visit the target file for a richer summary line."
   (pcase-let*
-      ((file (eglot--uri-to-path uri))
+      ((file (eglot-uri-to-path uri))
        (visiting (or (find-buffer-visiting file)
                      (gethash uri eglot--temp-location-buffers)))
        (collect (lambda ()
                   (eglot--widening
-                   (pcase-let* ((`(,beg . ,end) (eglot--range-region range))
+                   (pcase-let* ((`(,beg . ,end) (eglot-range-region range))
                                 (bol (progn (goto-char beg) (eglot--bol)))
                                 (substring (buffer-substring bol (line-end-position)))
                                 (hi-beg (- beg bol))
@@ -3190,7 +3190,7 @@ eglot-completion-at-point
                         (delete-region orig-pos (point))
                         (eglot--dbind ((TextEdit) range newText) textEdit
                           (pcase-let ((`(,beg . ,end)
-                                       (eglot--range-region range)))
+                                       (eglot-range-region range)))
                             (delete-region beg end)
                             (goto-char beg)
                             (funcall (or snippet-fn #'insert) newText))))
@@ -3331,7 +3331,7 @@ eglot--highlight-piggyback
                  (mapcar
                   (eglot--lambda ((DocumentHighlight) range)
                     (pcase-let ((`(,beg . ,end)
-                                 (eglot--range-region range)))
+                                 (eglot-range-region range)))
                       (let ((ov (make-overlay beg end)))
                         (overlay-put ov 'face 'eglot-highlight-symbol-face)
                         (overlay-put ov 'modification-hooks
@@ -3351,7 +3351,7 @@ eglot--imenu-SymbolInformation
        (pcase-lambda (`(,container . ,objs))
          (let ((elems (mapcar
                        (eglot--lambda ((SymbolInformation) kind name location)
-                         (let ((reg (eglot--range-region
+                         (let ((reg (eglot-range-region
                                      (plist-get location :range)))
                                (kind (alist-get kind eglot--symbol-kind-names)))
                            (cons (propertize name
@@ -3367,7 +3367,7 @@ eglot--imenu-SymbolInformation
 (defun eglot--imenu-DocumentSymbol (res)
   "Compute `imenu--index-alist' for RES vector of DocumentSymbol."
   (cl-labels ((dfs (&key name children range kind &allow-other-keys)
-                (let* ((reg (eglot--range-region range))
+                (let* ((reg (eglot-range-region range))
                        (kind (alist-get kind eglot--symbol-kind-names))
                        (name (propertize name
                                          'breadcrumb-region reg
@@ -3423,7 +3423,7 @@ eglot--apply-text-edits
                       (when reporter
                         (eglot--reporter-update reporter (cl-incf done))))))))
             (mapcar (eglot--lambda ((TextEdit) range newText)
-                      (cons newText (eglot--range-region range 'markers)))
+                      (cons newText (eglot-range-region range 'markers)))
                     (reverse edits)))
       (undo-amalgamate-change-group change-group)
       (when reporter
@@ -3436,14 +3436,14 @@ eglot--apply-workspace-edit
            (mapcar (eglot--lambda ((TextDocumentEdit) textDocument edits)
                      (eglot--dbind ((VersionedTextDocumentIdentifier) uri version)
                          textDocument
-                       (list (eglot--uri-to-path uri) edits version)))
+                       (list (eglot-uri-to-path uri) edits version)))
                    documentChanges)))
       (unless (and changes documentChanges)
         ;; We don't want double edits, and some servers send both
         ;; changes and documentChanges.  This unless ensures that we
         ;; prefer documentChanges over changes.
         (cl-loop for (uri edits) on changes by #'cddr
-                 do (push (list (eglot--uri-to-path uri) edits) prepared)))
+                 do (push (list (eglot-uri-to-path uri) edits) prepared)))
       (if (or confirm
               (cl-notevery #'find-buffer-visiting
                            (mapcar #'car prepared)))
-- 
2.41.0


[-- Attachment #3: 0001-Add-documentation-about-extending-Eglot.patch --]
[-- Type: application/octet-stream, Size: 4061 bytes --]

From e860bab2ac22fb9ea99e12bb095f72ef4c9b774a Mon Sep 17 00:00:00 2001
From: Filippo Argiolas <filippo.argiolas@gmail.com>
Date: Thu, 31 Aug 2023 07:22:05 +0200
Subject: [PATCH 1/2] Add documentation about extending Eglot
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* doc/misc/eglot.texi: add a simple example about extending Eglot
implementing its generic methods. The example, from João Távora, adds
support to inactiveRegions clangd protocol extension rendering
inactive code regions as shadowed in a LSP aware way (see discussion
in bug#65418)
---
 doc/misc/eglot.texi | 70 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/doc/misc/eglot.texi b/doc/misc/eglot.texi
index 6eb212ca841..ed1456725ae 100644
--- a/doc/misc/eglot.texi
+++ b/doc/misc/eglot.texi
@@ -985,6 +985,7 @@ Advanced server configuration
 * Project-specific configuration::
 * User-specific configuration::
 * JSONRPC objects in Elisp::
+* Extending Eglot::
 @end menu
 
 It's important to note that not all servers allow both kinds of
@@ -1262,6 +1263,75 @@ JSONRPC objects in Elisp
 @}
 @end example
 
+@node Extending Eglot
+@section Extending Eglot
+Sometimes it may be useful to extend existing Eglot functionality
+using its public methods.  A good example of when this need may arise
+is adding support for a custom LSP protocol extension only implemented
+by a specific server.
+
+The best source of documentation for this is probably Eglot source
+code itself. Most of the functionality is implemented with generic
+methods that can be easily extended or overridden and the code itself
+shows many examples about how to do this.
+
+Here's a simple example that adds support for clangd
+@code{inactiveRegions} extension introduced in clangd 17.
+
+When @code{inactiveRegions} capability is enabled the server will send
+a @code{textDocument/inactiveRegions} notification with a list of
+inactive code (e.g. code under ifdef macros) regions to the client.
+
+The first method extends @code{eglot-client-capabilities} using a
+simple heuristic to detect if current server is @command{clangd} and
+enables the @code{inactiveRegion} capability.
+
+The second method implements @code{eglot-handle-notification} to
+process the correspondent server notification and for each region
+received it creates an overlay applying the @code{shadow} face to the
+region. Overlays are recreated every time a new notification is
+received.
+
+Result will be that all the inactive code in the buffer will be nicely
+greyed out using the LSP server knowledge about current compile time
+preprocessor defines.
+
+@lisp
+;;; eglot-clangd-inactive-region.el -*- lexical-binding: t; -*-
+
+(require 'eglot)
+(require 'cl-lib)
+
+(cl-defmethod eglot-client-capabilities :around (server)
+  (let ((base (cl-call-next-method)))
+    (when (cl-find "clangd" (process-command (jsonrpc--process server))
+                   :test #'string-match)
+      (setf (cl-getf (cl-getf base :textDocument)
+                     :inactiveRegionsCapabilities)
+            '(:inactiveRegions t)))
+    base))
+
+(defvar-local eglot-clangd-inactive-region-overlays '())
+
+(cl-defmethod eglot-handle-notification
+  (_server (_method (eql textDocument/inactiveRegions))
+           &key regions textDocument &allow-other-keys)
+  (if-let* ((path (expand-file-name (eglot--uri-to-path
+                                     (cl-getf textDocument :uri))))
+            (buffer (find-buffer-visiting path)))
+      (with-current-buffer buffer
+        (mapc #'delete-overlay eglot-clangd-inactive-region-overlays)
+        (cl-loop
+         for r across regions
+         for (beg . end) = (eglot--range-region r)
+         for ov = (make-overlay beg end)
+         do
+         (overlay-put ov 'face 'shadow)
+         (push ov eglot-clangd-inactive-region-overlays)))))
+
+(provide 'eglot-clangd-inactive-region)
+@end lisp
+
 @node Troubleshooting Eglot
 @chapter Troubleshooting Eglot
 @cindex troubleshooting Eglot
-- 
2.41.0

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Filippo Argiolas]

On Sun, Aug 27, 2023 at 3:58 PM João Távora <joaotavora@gmail.com> wrote:
> > One thing about UI, all the themes I tried seem to render shadow as
> > grey-ish but it was my impression reading the docs that it would be a
> > dim version of the current face, so it would still have syntax
> > highlighting. Is it just a theme limitation (probably because shadow
> > wasn't used for something like this before) or it's not technically
> > possible?
>
> I'm fairly sure it's technically possible, even if perhaps not easy.
> You can investigate or ask this on emacs-devel.

Hi again,
I ended up investigating this on my own and managed to get something working.

https://github.com/fargiolas/eglot-clangd-inactive-regions/blob/master/eglot-clangd-inactive-regions.el

Still experimenting on it, the idea is to iterate over each inactive
region, detect face changes and create an overlay with a darkened
version of them, blending foreground and background colors.
No idea if that's the best approach but it works so far and I'm loving
the result!

Comments/criticisms/patches/PRs welcome (maybe outside of this bug report).
Despite being an emacs user for maybe 15+ years, that's the first time
I ever wrote more than two lines of elisp.

Thanks,
Filippo

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[João Távora]

On Thu, Aug 31, 2023 at 6:28 PM Filippo Argiolas
<filippo.argiolas@gmail.com> wrote:
>
> On Sun, Aug 27, 2023 at 3:58 PM João Távora <joaotavora@gmail.com> wrote:
> > Glad to help.  You're lucky I'm not some kind of wine connoisseur ;-)
> > Make a patch for that (and remember to also include the exportation of
> > the '--' symbols).
> >
>
> Many thanks again! That code definitely made this work week more enjoyable ;-)
>
> Sorry for the delay with the patch, little time left these days for
> side projects or even trivial contributions like this.
> See if the attached patches could do!

They were very helpful.  I just pushed heavily edited versions of
them to master.  I used them as a starting point and credited you as
co-author (I could have done the other way round, crediting myself
 as co-author, but the changes I did were a bit too extensive for
that).

João

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[João Távora]

On Mon, Sep 4, 2023 at 2:05 AM João Távora <joaotavora@gmail.com> wrote:
>
> On Thu, Aug 31, 2023 at 6:28 PM Filippo Argiolas
> <filippo.argiolas@gmail.com> wrote:
> >
> > On Sun, Aug 27, 2023 at 3:58 PM João Távora <joaotavora@gmail.com> wrote:
> > > Glad to help.  You're lucky I'm not some kind of wine connoisseur ;-)
> > > Make a patch for that (and remember to also include the exportation of
> > > the '--' symbols).
> > >
> >
> > Many thanks again! That code definitely made this work week more enjoyable ;-)
> >
> > Sorry for the delay with the patch, little time left these days for
> > side projects or even trivial contributions like this.
> > See if the attached patches could do!
>
> They were very helpful.  I just pushed heavily edited versions of
> them to master.  I used them as a starting point and credited you as
> co-author (I could have done the other way round, crediting myself
>  as co-author, but the changes I did were a bit too extensive for
> that).

Drats, I just noticed a very significant detail.  I should have
checked first, but you don't seem to have a FSF Copyright Assignment
for Emacs.  So I think I'll have to revert these two commits until we
get one from you.

I'm assuming you don't have a problem with that, else, we'll have to
find another solution.  Eli, can we send Filippo a CA request form?

João

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Filippo Argiolas]

On Mon, Sep 4, 2023 at 3:06 AM João Távora <joaotavora@gmail.com> wrote:
>
> On Mon, Sep 4, 2023 at 2:05 AM João Távora <joaotavora@gmail.com> wrote:
> >
> > On Thu, Aug 31, 2023 at 6:28 PM Filippo Argiolas
> > <filippo.argiolas@gmail.com> wrote:
> > >
> > > On Sun, Aug 27, 2023 at 3:58 PM João Távora <joaotavora@gmail.com> wrote:
> > > > Glad to help.  You're lucky I'm not some kind of wine connoisseur ;-)
> > > > Make a patch for that (and remember to also include the exportation of
> > > > the '--' symbols).
> > > >
> > >
> > > Many thanks again! That code definitely made this work week more enjoyable ;-)
> > >
> > > Sorry for the delay with the patch, little time left these days for
> > > side projects or even trivial contributions like this.
> > > See if the attached patches could do!
> >
> > They were very helpful.  I just pushed heavily edited versions of
> > them to master.  I used them as a starting point and credited you as
> > co-author (I could have done the other way round, crediting myself
> >  as co-author, but the changes I did were a bit too extensive for
> > that).

No problem for me, edit as much as you want! just glad to help.
I'd be just as happy if you just thanked me in the commit message.

> Drats, I just noticed a very significant detail.  I should have
> checked first, but you don't seem to have a FSF Copyright Assignment
> for Emacs.  So I think I'll have to revert these two commits until we
> get one from you.
>
> I'm assuming you don't have a problem with that, else, we'll have to
> find another solution.  Eli, can we send Filippo a CA request form?

I believe this contribution is trivial enough to not deserve any legal
paperwork.
It's just code written by yourself with a very few lines of docs you
used as a starting point to write your own.
I think it can be safely committed without issues, thanking me in the commit.

I don't think I have any problem with CA but I never saw the actual
forms, a bit skeptical if I'll have to involve my employer.

I'd say we consider this one as trivial enough and I could still sign
them to have them ready if I ever contribute something more
meaningful.

Filippo

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Filippo Argiolas]

On Mon, Sep 4, 2023 at 5:59 AM Filippo Argiolas
<filippo.argiolas@gmail.com> wrote:
>
> On Mon, Sep 4, 2023 at 3:06 AM João Távora <joaotavora@gmail.com> wrote:
> >
> > On Mon, Sep 4, 2023 at 2:05 AM João Távora <joaotavora@gmail.com> wrote:
> > >
> > > On Thu, Aug 31, 2023 at 6:28 PM Filippo Argiolas
> > > <filippo.argiolas@gmail.com> wrote:
> > > >
> > > > On Sun, Aug 27, 2023 at 3:58 PM João Távora <joaotavora@gmail.com> wrote:
> > > > > Glad to help.  You're lucky I'm not some kind of wine connoisseur ;-)
> > > > > Make a patch for that (and remember to also include the exportation of
> > > > > the '--' symbols).
> > > > >
> > > >
> > > > Many thanks again! That code definitely made this work week more enjoyable ;-)
> > > >
> > > > Sorry for the delay with the patch, little time left these days for
> > > > side projects or even trivial contributions like this.
> > > > See if the attached patches could do!
> > >
> > > They were very helpful.  I just pushed heavily edited versions of
> > > them to master.  I used them as a starting point and credited you as
> > > co-author (I could have done the other way round, crediting myself
> > >  as co-author, but the changes I did were a bit too extensive for
> > > that).
>
> No problem for me, edit as much as you want! just glad to help.
> I'd be just as happy if you just thanked me in the commit message.

By the way I looked at your doc version and very much approve it. The
one about public API I think I don't even deserve co-authorship :-D

Filippo

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[João Távora]

On Mon, Sep 4, 2023 at 5:09 AM Filippo Argiolas
<filippo.argiolas@gmail.com> wrote:
>
> On Mon, Sep 4, 2023 at 5:59 AM Filippo Argiolas
> <filippo.argiolas@gmail.com> wrote:
> >
> > On Mon, Sep 4, 2023 at 3:06 AM João Távora <joaotavora@gmail.com> wrote:
> > >
> > > On Mon, Sep 4, 2023 at 2:05 AM João Távora <joaotavora@gmail.com> wrote:
> > > >
> > > > On Thu, Aug 31, 2023 at 6:28 PM Filippo Argiolas
> > > > <filippo.argiolas@gmail.com> wrote:
> > > > >
> > > > > On Sun, Aug 27, 2023 at 3:58 PM João Távora <joaotavora@gmail.com> wrote:
> > > > > > Glad to help.  You're lucky I'm not some kind of wine connoisseur ;-)
> > > > > > Make a patch for that (and remember to also include the exportation of
> > > > > > the '--' symbols).
> > > > > >
> > > > >
> > > > > Many thanks again! That code definitely made this work week more enjoyable ;-)
> > > > >
> > > > > Sorry for the delay with the patch, little time left these days for
> > > > > side projects or even trivial contributions like this.
> > > > > See if the attached patches could do!
> > > >
> > > > They were very helpful.  I just pushed heavily edited versions of
> > > > them to master.  I used them as a starting point and credited you as
> > > > co-author (I could have done the other way round, crediting myself
> > > >  as co-author, but the changes I did were a bit too extensive for
> > > > that).
> >
> > No problem for me, edit as much as you want! just glad to help.
> > I'd be just as happy if you just thanked me in the commit message.
>
> By the way I looked at your doc version and very much approve it. The
> one about public API I think I don't even deserve co-authorship :-D

It's mainly because i started by applying your patch then doing multiple
changes to them.

My intuition also tells me that the remnants of both your
contribution are indeed "trivial" (as measured both in absolute LOC
and complexity)

But  I admit I'm not fully aware of how these things are counted
"officially", so better safe than sorry and I reverted them.  But
I would very much like to re-revert of course, as this was a lot
of work and is a pretty valuable change, IMO.  Let's wait for Eli to
explain how to proceed.

João

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 4 Sep 2023 02:08:47 +0100
> Cc: Felician Nemeth <felician.nemeth@gmail.com>, 65418@debbugs.gnu.org, 
> 	Philip Kaludercic <philipk@posteo.net>
> 
> I'm assuming you don't have a problem with that, else, we'll have to
> find another solution.  Eli, can we send Filippo a CA request form?

Done off-list.

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 4 Sep 2023 11:51:33 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, Felician Nemeth <felician.nemeth@gmail.com>, 65418@debbugs.gnu.org, 
> 	Philip Kaludercic <philipk@posteo.net>
> 
> On Mon, Sep 4, 2023 at 5:09 AM Filippo Argiolas
> <filippo.argiolas@gmail.com> wrote:
> >
> > On Mon, Sep 4, 2023 at 5:59 AM Filippo Argiolas
> > <filippo.argiolas@gmail.com> wrote:
> > >
> > > On Mon, Sep 4, 2023 at 3:06 AM João Távora <joaotavora@gmail.com> wrote:
> > > >
> > > > On Mon, Sep 4, 2023 at 2:05 AM João Távora <joaotavora@gmail.com> wrote:
> > > > >
> > > > > On Thu, Aug 31, 2023 at 6:28 PM Filippo Argiolas
> > > > > <filippo.argiolas@gmail.com> wrote:
> > > > > >
> > > > > > On Sun, Aug 27, 2023 at 3:58 PM João Távora <joaotavora@gmail.com> wrote:
> > > > > > > Glad to help.  You're lucky I'm not some kind of wine connoisseur ;-)
> > > > > > > Make a patch for that (and remember to also include the exportation of
> > > > > > > the '--' symbols).
> > > > > > >
> > > > > >
> > > > > > Many thanks again! That code definitely made this work week more enjoyable ;-)
> > > > > >
> > > > > > Sorry for the delay with the patch, little time left these days for
> > > > > > side projects or even trivial contributions like this.
> > > > > > See if the attached patches could do!
> > > > >
> > > > > They were very helpful.  I just pushed heavily edited versions of
> > > > > them to master.  I used them as a starting point and credited you as
> > > > > co-author (I could have done the other way round, crediting myself
> > > > >  as co-author, but the changes I did were a bit too extensive for
> > > > > that).
> > >
> > > No problem for me, edit as much as you want! just glad to help.
> > > I'd be just as happy if you just thanked me in the commit message.
> >
> > By the way I looked at your doc version and very much approve it. The
> > one about public API I think I don't even deserve co-authorship :-D
> 
> It's mainly because i started by applying your patch then doing multiple
> changes to them.
> 
> My intuition also tells me that the remnants of both your
> contribution are indeed "trivial" (as measured both in absolute LOC
> and complexity)
> 
> But  I admit I'm not fully aware of how these things are counted
> "officially", so better safe than sorry and I reverted them.  But
> I would very much like to re-revert of course, as this was a lot
> of work and is a pretty valuable change, IMO.  Let's wait for Eli to
> explain how to proceed.

To provide a helpful answer I need to know how much code was provided
by Filippo.  Where can I see that?

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[João Távora]

On Mon, Sep 4, 2023 at 1:44 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > But  I admit I'm not fully aware of how these things are counted
> > "officially", so better safe than sorry and I reverted them.  But
> > I would very much like to re-revert of course, as this was a lot
> > of work and is a pretty valuable change, IMO.  Let's wait for Eli to
> > explain how to proceed.
>
> To provide a helpful answer I need to know how much code was provided
> by Filippo.  Where can I see that?

Some 4 or 5 messages ago in this thread.  Here:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65418#32

Two patches are there and they correspond more or less to the two
(very heavily modified) commits I did on top of them.

João

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 4 Sep 2023 13:49:51 +0100
> Cc: filippo.argiolas@gmail.com, felician.nemeth@gmail.com, 
> 	65418@debbugs.gnu.org, philipk@posteo.net
> 
> On Mon, Sep 4, 2023 at 1:44 PM Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > > But  I admit I'm not fully aware of how these things are counted
> > > "officially", so better safe than sorry and I reverted them.  But
> > > I would very much like to re-revert of course, as this was a lot
> > > of work and is a pretty valuable change, IMO.  Let's wait for Eli to
> > > explain how to proceed.
> >
> > To provide a helpful answer I need to know how much code was provided
> > by Filippo.  Where can I see that?
> 
> Some 4 or 5 messages ago in this thread.  Here:
> 
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65418#32
> 
> Two patches are there and they correspond more or less to the two
> (very heavily modified) commits I did on top of them.

The first of these 2 patches is trivial.  The second one is not
trivial, but it looks like you've rewritten the text almost from
scratch, so I indeed think we don't need any legal paperwork from
Filippo for these changes, and you can install those on master.  (I
would still encourage Filippo to start the paperwork rolling, so that
we could accept his contributions in the future without limitations.)

Thanks.

bug#65418: 29.1; Eglot: support clangd inactiveRegions extension

[João Távora]

On Mon, Sep 4, 2023 at 5:18 PM Eli Zaretskii <eliz@gnu.org> wrote:

> The first of these 2 patches is trivial.  The second one is not
> trivial, but it looks like you've rewritten the text almost from
> scratch, so I indeed think we don't need any legal paperwork from
> Filippo for these changes, and you can install those on master.  (I
> would still encourage Filippo to start the paperwork rolling, so that
> we could accept his contributions in the future without limitations.)

Thanks very much for checking.  I just re-pushed the two commits.

And with this, closing the bug.