[NonGNU ELPA] new package: clangd-inactive-regions

[NonGNU ELPA] new package: clangd-inactive-regions

[Filippo Argiolas]

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

Hey everyone,

Just wanted to check if there's any interest in including my little
package in NonGNU ELPA:

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

It's an eglot extension that listens to clangd inactiveRegions
notifications and shades inactive code accordingly. It's useful to
highlight disabled code sections in a LSP aware way, honoring current
building options and defines.

It started as little more than an experiment but I've been using it on
the day job with a ifdef-heavy codebase and it has been serving me
well for more than a year. Thought it could be useful to other people
too.

I'm not exactly an elisp expert, so feedback is very welcome!

Cheers,
Filippo

[-- Attachment #2: 0001-elpa-packages-clangd-inactive-regions-New-package.patch --]
[-- Type: application/octet-stream, Size: 843 bytes --]

From 6d68c44584015416111a383d3821886a23e930c5 Mon Sep 17 00:00:00 2001
From: Filippo Argiolas <filippo.argiolas@gmail.com>
Date: Fri, 1 Nov 2024 09:38:13 +0100
Subject: [PATCH] * elpa-packages (clangd-inactive-regions): New package

---
 elpa-packages | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/elpa-packages b/elpa-packages
index 86d753c8da..1089888ae4 100644
--- a/elpa-packages
+++ b/elpa-packages
@@ -99,6 +99,9 @@
   :ignored-files ("LICENSE" "doc" "logo" "refcard" "test")
   :news "CHANGELOG.md")
 
+ (clangd-inactive-regions :url "https://github.com/fargiolas/clangd-inactive-regions.el"
+  :ignored-files ("LICENSE" "screenshots"))
+
  (clojure-mode		:url "https://github.com/clojure-emacs/clojure-mode"
   :ignored-files ("clojure-mode-extra-font-locking.el" "doc" "test" "test.clj")
   :news "CHANGELOG.md")
-- 
2.44.0

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Gerd Möllmann]

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

> I'm not exactly an elisp expert, so feedback is very welcome!

Works fine for me on macOS 15.1/tty with clangd 19.1 from Homebrew.
Thanks, that's a nice feature!

You said you are interested in feedback, so here's my wishlist :-).

- I'd preferred a global minor mode to turn this on and off.
  Which would replace the manual adding to the Eglot hook.

- I think something is to be up with the faces? If I remove

  (clangd-inactive-regions-set-method "darken-foreground")
  (clangd-inactive-regions-set-opacity 0.55))

  from the use-package in my init file, which is

  (use-package clangd-inactive-regions
    :straight (:host github
                     :repo "fargiolas/clangd-inactive-regions.el"
                     :branch "master"
                     :files ("*.el"))
    :init
    (add-hook 'eglot-managed-mode-hook #'clangd-inactive-regions-mode)
    :config
    (clangd-inactive-regions-set-method "darken-foreground")
    (clangd-inactive-regions-set-opacity 0.55))

  the inactive regions are not shown, I think. If I put them in, they
  are.

- Maybe some of the defvar could be defcustoms?

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Filippo Argiolas]

On Sat, Nov 2, 2024 at 10:13 AM Gerd Möllmann <gerd.moellmann@gmail.com> wrote:
> Works fine for me on macOS 15.1/tty with clangd 19.1 from Homebrew.
> Thanks, that's a nice feature!
>
> You said you are interested in feedback, so here's my wishlist :-).
>
> - I'd preferred a global minor mode to turn this on and off.
>   Which would replace the manual adding to the Eglot hook.

Thanks for the very welcome feedback!
My idea was to keep this local as it hooks into fontification and
wanted to limit any problems that may arise only to the buffers where
the mode is enabled.
This by the way was only possible with c-ts-mode as cc-mode chains
into the default function in its fontify-region hook, so if I want to
run my code after theirs it must be inherently global.

Guess the same could be achieved by defining a global minor mode and
adding eglot/c mode hooks myself but I'd like to leave the control to
the user for now.

> - I think something is to be up with the faces? If I remove
>
>   (clangd-inactive-regions-set-method "darken-foreground")
>   (clangd-inactive-regions-set-opacity 0.55))
>
>   the inactive regions are not shown, I think. If I put them in, they
>   are.

Thanks, always had those in the init and never noticed. Should be
fixed now, could you test again?

> - Maybe some of the defvar could be defcustoms?

Agreed, will look into it.

Filippo

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Gerd Möllmann]

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

> On Sat, Nov 2, 2024 at 10:13 AM Gerd Möllmann <gerd.moellmann@gmail.com> wrote:
>> Works fine for me on macOS 15.1/tty with clangd 19.1 from Homebrew.
>> Thanks, that's a nice feature!
>>
>> You said you are interested in feedback, so here's my wishlist :-).
>>
>> - I'd preferred a global minor mode to turn this on and off.
>>   Which would replace the manual adding to the Eglot hook.
>
> Thanks for the very welcome feedback!
> My idea was to keep this local as it hooks into fontification and
> wanted to limit any problems that may arise only to the buffers where
> the mode is enabled.
> This by the way was only possible with c-ts-mode as cc-mode chains
> into the default function in its fontify-region hook, so if I want to
> run my code after theirs it must be inherently global.
>
> Guess the same could be achieved by defining a global minor mode and
> adding eglot/c mode hooks myself but I'd like to leave the control to
> the user for now.

Hm, okay.

>
>> - I think something is to be up with the faces? If I remove
>>
>>   (clangd-inactive-regions-set-method "darken-foreground")
>>   (clangd-inactive-regions-set-opacity 0.55))
>>
>>   the inactive regions are not shown, I think. If I put them in, they
>>   are.
>
> Thanks, always had those in the init and never noticed. Should be
> fixed now, could you test again?

Yes, it works now.

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Stefan Kangas]

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

> Just wanted to check if there's any interest in including my little
> package in NonGNU ELPA:
>
> https://github.com/fargiolas/clangd-inactive-regions.el
>
> It's an eglot extension that listens to clangd inactiveRegions
> notifications and shades inactive code accordingly. It's useful to
> highlight disabled code sections in a LSP aware way, honoring current
> building options and defines.
>
> It started as little more than an experiment but I've been using it on
> the day job with a ifdef-heavy codebase and it has been serving me
> well for more than a year. Thought it could be useful to other people
> too.
>
> I'm not exactly an elisp expert, so feedback is very welcome!

Thanks.  It sounds useful, so I think we should add it.  I'm interested
to see how well this will work in Emacs itself, but I didn't have time
to test it yet.

How would you feel about adding this package to GNU ELPA instead?  The
main requirement would be copyright assignments to the FSF from all
current and future contributors.  I see that you're the sole contributor
for now.  I think that would be useful, since there seems to be a plan
to suggest adding this to the LSP protocol, in which case we'd really
want this in Eglot itself, I think.[1]

One comment from reading the README is that it would be helpful to add a
few sentences there about what the inactiveRegion LSP extension does.
For example, it could say something like this:

    "inactiveRegions" is a clangd-specific extension to the LSP protocol
    that allows LSP clients to hide code that is in a currently inactive
    preprocessor branch.

...or something along those lines.

On a side note, I also read this in the README:

    As far as I know Emacs doesn't have a way to set foreground text
    opacity. Best would be a face attribute so that you can set it in an
    overlay covering each inactive region and be done with
    it. Unfortunately there is no attribute for this yet.

Would such a feature be hard to add to the display engine?  Eli?

Footnotes:
[1]  https://github.com/clangd/clangd/issues/132#issuecomment-1722327517

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Felician Nemeth]

Stefan Kangas <stefankangas@gmail.com> writes:

> Filippo Argiolas <filippo.argiolas@gmail.com> writes:
>
>> Just wanted to check if there's any interest in including my little
>> package in NonGNU ELPA:
>>
>> https://github.com/fargiolas/clangd-inactive-regions.el
>>

> Thanks.  It sounds useful, so I think we should add it.  I'm interested
> to see how well this will work in Emacs itself,

There is an open pull request by Slava Akhmechet in eglot-x that
implements a similar feature for clangd and ccls (which uses a differnet
LSP extension) [1].  There I wondered its compatibility with the
built-in hide-ifdef-mode.

Slava originally sent the pull request to the Eglot's repository [2],
but João said "Eglot is, by design, only for LSP nor for extensions to
the protocol."

By the way, can you, please, check whether Slava Akhmechet has completed
his copyright paperwork?  Thank you.

Footnotes:
[1] https://github.com/nemethf/eglot-x/pull/7
[2] https://github.com/joaotavora/eglot/pull/1343

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Stefan Kangas]

Felician Nemeth <felician.nemeth@gmail.com> writes:

> There is an open pull request by Slava Akhmechet in eglot-x that
> implements a similar feature for clangd and ccls (which uses a differnet
> LSP extension) [1].  There I wondered its compatibility with the
> built-in hide-ifdef-mode.
>
> Slava originally sent the pull request to the Eglot's repository [2],
> but João said "Eglot is, by design, only for LSP nor for extensions to
> the protocol."

I see the logic in João's arguments for wanting to limit eglot.el to the
LSP standard only.  But then the natural question is: where does
functionality covering LSP extensions belong?

One option is to have separate packages for every LSP extension out
there, or indeed a catch-all like eglot-x.el.  Maybe this is okay.
However, does Eglot's design goals really mean that we will _never_ want
these features in Emacs core?  I'm not so sure.

This is why I'm proposing to submit the new clangd-inactive-regions
package to GNU ELPA instead.  That way we could more easily integrate
this into Emacs later (i.e. without having to worry about copyright
assignments).  We would still need to figure some things out, of course,
such as where this functionality goes: to some new `eglot-extras.el', to
various major modes, or what.

Regarding eglot-x.el, of which you Felician seems to be the only major
contributor, I would actually say the exact same thing.  Why isn't this
package on GNU ELPA?  I suggest that you to submit it.  :-)

I'm copying in João, in case he has anything to add here.

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Filippo Argiolas]

On Sun, Nov 3, 2024 at 2:22 AM Stefan Kangas <stefankangas@gmail.com> wrote:
>
> Felician Nemeth <felician.nemeth@gmail.com> writes:
>
> > There is an open pull request by Slava Akhmechet in eglot-x that
> > implements a similar feature for clangd and ccls (which uses a differnet
> > LSP extension) [1].  There I wondered its compatibility with the
> > built-in hide-ifdef-mode.
> >
> > Slava originally sent the pull request to the Eglot's repository [2],
> > but João said "Eglot is, by design, only for LSP nor for extensions to
> > the protocol."
>
> I see the logic in João's arguments for wanting to limit eglot.el to the
> LSP standard only.  But then the natural question is: where does
> functionality covering LSP extensions belong?
>
> One option is to have separate packages for every LSP extension out
> there, or indeed a catch-all like eglot-x.el.  Maybe this is okay.
> However, does Eglot's design goals really mean that we will _never_ want
> these features in Emacs core?  I'm not so sure.
>
> This is why I'm proposing to submit the new clangd-inactive-regions
> package to GNU ELPA instead.  That way we could more easily integrate
> this into Emacs later (i.e. without having to worry about copyright
> assignments).  We would still need to figure some things out, of course,
> such as where this functionality goes: to some new `eglot-extras.el', to
> various major modes, or what.
>
> Regarding eglot-x.el, of which you Felician seems to be the only major
> contributor, I would actually say the exact same thing.  Why isn't this
> package on GNU ELPA?  I suggest that you to submit it.  :-)
>
> I'm copying in João, in case he has anything to add here.

Thanks everyone for the feedback!

Before creating this package I had some discussion about this with
João, don't remember if it was in debbugs or here.
His stance then too was that this was server specific and did not
belong in eglot.
The output from that discussion was adding a section into eglot
manual[1] that describes how to extend eglot using this very same use
case as an example.
If one wanted to just use the shadow face the code is all there and
it's the same I'm using.

Most of the code in my package is for the trick to darken the inactive
regions after fontification. But I'd say a proper opacity attribute
would be way more suited to emacs core than my ugly hack :-)

About GNU ELPA, I can look into it, could you send the paperwork
off-list? I'm ok with it as long as I don't need to involve my
employer.


1. https://joaotavora.github.io/eglot/#Extending-Eglot-1

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Stefan Kangas]

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

> About GNU ELPA, I can look into it, could you send the paperwork
> off-list? I'm ok with it as long as I don't need to involve my
> employer.

Form sent off-list, thank you.

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Felician Nemeth]

Stefan Kangas <stefankangas@gmail.com> writes:

> Regarding eglot-x.el, of which you Felician seems to be the only major
> contributor, I would actually say the exact same thing.  Why isn't this
> package on GNU ELPA?  I suggest that you to submit it.  :-)

I've already submitted it:
https://lists.gnu.org/archive/html/emacs-devel/2023-05/msg00135.html

One piece of the feedback I got was that the package should work with
the built-in version of Eglot.  I have not worked on this goal, but once
Emacs version 30 is released, this issue is automatically solved.

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Eli Zaretskii]

> From: Felician Nemeth <felician.nemeth@gmail.com>
> Cc: Filippo Argiolas <filippo.argiolas@gmail.com>,  emacs-devel@gnu.org,
>   Eli Zaretskii <eliz@gnu.org>
> Date: Sat, 02 Nov 2024 23:35:28 +0100
> 
> By the way, can you, please, check whether Slava Akhmechet has completed
> his copyright paperwork?

Not under that name, no.  Are you sure you got the name right?  And
what is the email address?

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Felician Nemeth]

On Sun, Nov 03 2024, Eli Zaretskii wrote:

>> From: Felician Nemeth <felician.nemeth@gmail.com>
>> Cc: Filippo Argiolas <filippo.argiolas@gmail.com>,  emacs-devel@gnu.org,
>>   Eli Zaretskii <eliz@gnu.org>
>> Date: Sat, 02 Nov 2024 23:35:28 +0100
>> 
>> By the way, can you, please, check whether Slava Akhmechet has completed
>> his copyright paperwork?
>
> Not under that name, no.  Are you sure you got the name right?  And
> what is the email address?

I've now CC'd him.  He submitted this pull request last year with these
headers:

Author: Vyacheslav Akhmechet <coffeemug@gmail.com>
Committer: Slava Akhmechet <coffeemug@gmail.com>

Thanks again.

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 2 Nov 2024 14:52:11 -0700
> Cc: Eli Zaretskii <eliz@gnu.org>
> 
>     As far as I know Emacs doesn't have a way to set foreground text
>     opacity. Best would be a face attribute so that you can set it in an
>     overlay covering each inactive region and be done with
>     it. Unfortunately there is no attribute for this yet.
> 
> Would such a feature be hard to add to the display engine?  Eli?

I don't know the answer, sorry.  Someone who knows how opacity should
be implemented in display back-ends we support should tell.  If this
is going to be a face attribute, we should also decide how to merge
two faces when each one specifies some opacity value.

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > It's an eglot extension that listens to clangd inactiveRegions
  > > notifications and shades inactive code accordingly. It's useful to
  > > highlight disabled code sections in a LSP aware way, honoring current
  > > building options and defines.

Does "inactive region" mean unreachable code?  I am guessin so.
 
Is there a standard way for lamguage servers to report unreachable code?
Could we make Emacs recognize that way?  Then eglot could handle it
for any language, and it would not be specific to clangd.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Filippo Argiolas]

On Mon, Nov 4, 2024 at 6:34 AM Richard Stallman <rms@gnu.org> wrote:
>   > > It's an eglot extension that listens to clangd inactiveRegions
>   > > notifications and shades inactive code accordingly. It's useful to
>   > > highlight disabled code sections in a LSP aware way, honoring current
>   > > building options and defines.
>
> Does "inactive region" mean unreachable code?  I am guessin so.
>
> Is there a standard way for lamguage servers to report unreachable code?
> Could we make Emacs recognize that way?  Then eglot could handle it
> for any language, and it would not be specific to clangd.

In this context it means unreachable code as disabled by the
preprocessor stage, e.g. code under a currently disabled ifdef branch.
LSP advantage here is providing knowledge about how a file is compiled
with proper build time flags and defines for current build
configuration.

At the moment it's a clangd specific extension, there was some plan[1]
for standard LSP inclusion but I'm not sure it went anywhere.
Also, does this specific meaning make sense for non c/c++/preprocessed
languages?

I believe unreachable in the control flow/static analysis sense can
already be achieved with normal LSP diagnostics. If I am not mistaken
clangd already provides diagnostics for this when clang-tidy is
enabled.


1. https://github.com/clangd/clangd/issues/132#issuecomment-1722327517

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Philip Kaludercic]

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

> On Mon, Nov 4, 2024 at 6:34 AM Richard Stallman <rms@gnu.org> wrote:
>>   > > It's an eglot extension that listens to clangd inactiveRegions
>>   > > notifications and shades inactive code accordingly. It's useful to
>>   > > highlight disabled code sections in a LSP aware way, honoring current
>>   > > building options and defines.
>>
>> Does "inactive region" mean unreachable code?  I am guessin so.
>>
>> Is there a standard way for lamguage servers to report unreachable code?
>> Could we make Emacs recognize that way?  Then eglot could handle it
>> for any language, and it would not be specific to clangd.
>
> In this context it means unreachable code as disabled by the
> preprocessor stage, e.g. code under a currently disabled ifdef branch.
> LSP advantage here is providing knowledge about how a file is compiled
> with proper build time flags and defines for current build
> configuration.
>
> At the moment it's a clangd specific extension, there was some plan[1]
> for standard LSP inclusion but I'm not sure it went anywhere.
> Also, does this specific meaning make sense for non c/c++/preprocessed
> languages?

It might not be a bad idea to anticipate future standardisations, or
other LSP servers supporting similar functionality, and branding the
package as eglot-inactive-regions instead of the more specific
clangd-inactive-regions (which also doesn't hint that it is eglot
specific).

> I believe unreachable in the control flow/static analysis sense can
> already be achieved with normal LSP diagnostics. If I am not mistaken
> clangd already provides diagnostics for this when clang-tidy is
> enabled.
>
>
> 1. https://github.com/clangd/clangd/issues/132#issuecomment-1722327517
>
>

-- 
	Philip Kaludercic on siskin

Re: [NonGNU ELPA] new package: clangd-inactive-regions

[Filippo Argiolas]

On Mon, Nov 4, 2024 at 1:02 PM Philip Kaludercic <philipk@posteo.net> wrote:
>
> Filippo Argiolas <filippo.argiolas@gmail.com> writes:
>
> > On Mon, Nov 4, 2024 at 6:34 AM Richard Stallman <rms@gnu.org> wrote:
> >>   > > It's an eglot extension that listens to clangd inactiveRegions
> >>   > > notifications and shades inactive code accordingly. It's useful to
> >>   > > highlight disabled code sections in a LSP aware way, honoring current
> >>   > > building options and defines.
> >>
> >> Does "inactive region" mean unreachable code?  I am guessin so.
> >>
> >> Is there a standard way for lamguage servers to report unreachable code?
> >> Could we make Emacs recognize that way?  Then eglot could handle it
> >> for any language, and it would not be specific to clangd.
> >
> > In this context it means unreachable code as disabled by the
> > preprocessor stage, e.g. code under a currently disabled ifdef branch.
> > LSP advantage here is providing knowledge about how a file is compiled
> > with proper build time flags and defines for current build
> > configuration.
> >
> > At the moment it's a clangd specific extension, there was some plan[1]
> > for standard LSP inclusion but I'm not sure it went anywhere.
> > Also, does this specific meaning make sense for non c/c++/preprocessed
> > languages?
>
> It might not be a bad idea to anticipate future standardisations, or
> other LSP servers supporting similar functionality, and branding the
> package as eglot-inactive-regions instead of the more specific
> clangd-inactive-regions (which also doesn't hint that it is eglot
> specific).
>

Never really liked the name but there was no way to convey everything
in a short name... eglot extension, specific to clangd, inactive
regions etc.
I never used ccls my self but I see from the PR linked by Felician
that ccls support for their skipped ranges extension would be already
easy enough to add.
So a rename might actually make sense.
Will think about it and report back!

Filippo