nemethf / eglot-x

Protocol extensions for Eglot
GNU General Public License v3.0
114 stars 6 forks source link

Add support for inactive code notifications. #7

Open coffeemug opened 6 months ago

coffeemug commented 6 months ago

CCLS and clangd publish skipped region notifications for inactive code. An example of this is C preprocessor-- if you're in the middle of an $ifdef SOME_PLATFORM ... #endif block and SOME_PLATFORM isn't defined, the language servers inform the client that the section is inactive. This pull request adds support for these notifications, which allows for a nicer developer experience.

Eglot-x + ccls/clangd without this pull request: image

Eglot-x + ccls/clangd with this pull request: image

In big c/c++ files, this hinting can make enormous difference in dev experience quality.

coffeemug commented 6 months ago
joaotavora commented 6 months ago

Re hide-ifdef-mode, I didn't know about it (of course there is a mode!) My personal view is that it's better not to integrate. I looked at the code and it does a lot of stuff; given modern language servers it feels like it's nearly all legacy. Integrations like these always confuse me as a user. "So eglot-x supports inactive code regions, but do I need to enable hide-ifdef-mode? How do they interact exactly? Etc." In my view whenever the modern approach takes such a radical departure, it's almost always less confusing to the user to keep legacy modes completely separate.

That's what Eglot did with Flymake, Eldoc, Imenu and Xref. If you don't do that, you start to make to our own ecosystem and soon you'll have a bloated lsp-mode.

But sometimes your take is correct. Which-func-mode, for example, is a horrible choice for implementing "breadcrumbs". And there is no refactor.el yet, and sometimes it's worth simplifying the legacy code or generalizing it. And sometimes it's just not worth it as you claim. But it's definitely not "almost always".

Does Hide-ifdef allow shading -- not only hiding? Does it have good Isearch support for hidden text, like hs-mode does? Is it quick and unobtrusive? Those are the questions to ask. If Hide-ifdef mode supports "backends" somehow, then Eglot-x could arrange for the Eglot-x specific backends (and only those -- that's the Eglot philosophy) to become active during Eglot's tenure of the user's buffers. It may even turn on Hide-ifdef automatically, just like Eglot does with Flymake, for example.

nemethf commented 6 months ago

I CC @yxlee, maybe the maintainer of hideif.el has something to add. Thanks.

Re hide-ifdef-mode, I didn't know about it (of course there is a mode!) My personal view is that it's better not to integrate. I looked at the code and it does a lot of stuff; given modern language servers it feels like it's nearly all legacy.

A LSP server can more reliably tell what regions need to be shadowed/hidden, but hideif provides a convenient way to define what should be hidden. For example, the user can easily switch to read the windows part of a code on a linux box without modifying the code.

Eglot does respect the settings of tab-width, require-final-newline, delete-trailing-lines. In a similar vein, eglot-x could respect hide-ifdef-lines and hide-ifdef-shadow.

Integrations like these always confuse me as a user. "So eglot-x supports inactive code regions, but do I need to enable hide-ifdef-mode? How do they interact exactly? Etc." In my view whenever the modern approach takes such a radical departure, it's almost always less confusing to the user to keep legacy modes completely separate.

That's what Eglot did with Flymake, Eldoc, Imenu and Xref. If you don't do that, you start to make to our own ecosystem and soon you'll have a bloated lsp-mode.

But sometimes your take is correct. Which-func-mode, for example, is a horrible choice for implementing "breadcrumbs". And there is no refactor.el yet, and sometimes it's worth simplifying the legacy code or generalizing it. And sometimes it's just not worth it as you claim. But it's definitely not "almost always".

Does Hide-ifdef allow shading -- not only hiding?

It does. See variable hide-ifdef-shadow.

Does it have good Isearch support for hidden text, like hs-mode does?

I don't know, but it does support reveal.el.

Is it quick and unobtrusive?

It is.

Those are the questions to ask. If Hide-ifdef mode supports "backends" somehow, then Eglot-x could arrange for the Eglot-x specific backends (and only those -- that's the Eglot philosophy) to become active during Eglot's tenure of the user's buffers. It may even turn on Hide-ifdef automatically, just like Eglot does with Flymake, for example.

There are no backends for hide-if.el, but I agree with this general approach.

coffeemug commented 6 months ago

I just pushed a version of the PR that addresses all the inline comments.

Re hide-ifdef-mode I think the feedback is reasonable. TBH I only glanced at the mode and went with my first reaction-- that it's ~2600+ lines of very specialized code to implement C preprocessor expression parsing. I can see the rationale to reuse the interface, though I still personally disagree with it. There are variables in the mode that aren't practical to respect as the functionality is now handled by the language servers. Examples of these are hide-ifdef-expand-reinclusion-guard or hide-ifdef-header-regexp. If there is an integration, as a user I'm left to wonder which parts of hide-ifdef-mode are supported, and which are not. IMO that's a very confusing experience, and given the simplicity of the patch is far more trouble than it's worth.

That said, I do see the value in refactoring hide-ifdef-mode to have a clear delineation between an interface and backend implementations. I'm not sure I want to commit time to doing that, I'd have to think about it.

joaotavora commented 6 months ago

2600 lines? Yikes, that's a lot indeed. And it's obsolete anyway, not only by LSP but by Emacs tree sitter too.

So I would personally be on the fence too. The only useful things to reuse are the hiding and shadowing logic, and maybe that itself should be in some kind of "folding" library.

So I'm sorry to bat for the other side now :)

nemethf commented 6 months ago

I just pushed a version of the PR that addresses all the inline comments.

Thanks. You can start the copyright assignment process by reading the relevant part of the Emacs manual.

There is one issue to fix before merging: the name of helper function should be prefixed with eglot-x--.

(Later, I'd like to extend this feature to support hiding besides shadowing even if it is not integrated with hideif.el in any ways.)

coffeemug commented 6 months ago

Done and done.

FYI re hiding— clangd labels the code between #ifdef … #end as inactive, but ccls labels code including #ifdef … #end as inactive. This is something I didn't expect.

On Sun, Dec 31, 2023 at 1:41 AM, Felicián Németh < @.*** > wrote:

I just pushed a version of the PR that addresses all the inline comments.

Thanks. You can start the copyright assignment process by reading the relevant part of the Emacs manual ( https://www.gnu.org/software/emacs/manual/html_node/emacs/Copyright-Assignment.html ).

There is one issue to fix before merging: the name of helper function should be prefixed with eglot-x--.

(Later, I'd like to extend this feature to support hiding besides shadowing even if it is not integrated with hideif.el in any ways.)

— Reply to this email directly, view it on GitHub ( https://github.com/nemethf/eglot-x/pull/7#issuecomment-1872905910 ) , or unsubscribe ( https://github.com/notifications/unsubscribe-auth/AAAL2NDHTR26FIOBY6LFD53YMEXNPAVCNFSM6AAAAABBHGDPB6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZSHEYDKOJRGA ). You are receiving this because you authored the thread. Message ID: <nemethf/eglot-x/pull/7/c1872905910 @ github. com>

nemethf commented 4 months ago

@coffeemug, is there any update on the copyright assignment?

coffeemug commented 4 months ago

Sorry I got back to my day job after xmas break and dropped the ball on this. Just signed the doc and sent it to fsf.

On Wed, Feb 21, 2024 at 1:56 AM, Felicián Németh < @.*** > wrote:

@ coffeemug ( https://github.com/coffeemug ) , is there any update on the copyright assignment?

— Reply to this email directly, view it on GitHub ( https://github.com/nemethf/eglot-x/pull/7#issuecomment-1956281044 ) , or unsubscribe ( https://github.com/notifications/unsubscribe-auth/AAAL2NCPU7KDJYOZM7FYFXTYUXAFHAVCNFSM6AAAAABBHGDPB6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJWGI4DCMBUGQ ). You are receiving this because you were mentioned. Message ID: <nemethf/eglot-x/pull/7/c1956281044 @ github. com>