Add new command: consult-lsp-file-symbols

[northgaard]

Hello, thanks for the very useful package!

I have implemented an additional command which allows quickly searching through the symbols in the current file in a manner very similar to consult-line. Some general comments:

• I have used it daily in my work on a large C++ codebase, and have not encountered any issues so far. That said, I have only really tested with clangd, so it might be nice if people who are working with other languages/servers could try it out.
• Unlike the other commands provided by the package, the request is sent to the language server synchronously. I generally experience slight delays (on the order of 0.1-0.2 seconds) the first few times that I use the command at the beginning of the day, and then from there it is pretty much instantaneous (I suspect clangd needs to load the correct part of its index into memory initially). So I have found performance to be quite acceptable, but mileage may vary with other language servers/older hardware.

Let me know what you think!

[gagbo]

Thanks for the contribution, I'll take some time to check those!

• I think that using a synchronous command is ok.
  As you said, cold starts take some time with clangd, and I don't know if all LSP servers have good caching for this request. But it doesn't really matter, because if a server has bad performance, they can use imenu as well (and I assume lsp has some sort of imenu backend that does what this patch does)
• Maybe the marginalia stuff should go elsewhere ?
  No idea on this one, I thought that marginalia had most of its categories in their own repo, it feels weird to have the annotations there. At least I think it's better if the defcustom and annotation function are moved to their own file, like consult-lsp-marginalia.el. It separates the dependencies this way, and cleans up the main file imo. (And allows to quickly remove that part, should the marginalia annotation be upstreamed)
• General code review
  I don't know much about the markers API and the various consult helpers so it'll take me some time to get into that

[gagbo]

This check shouldn't be necessary, because a user who wants to use marginalia is going to activate the minor-mode anyway, thus automatically requiring 'marginalia

[gagbo]

I guess that line should have the condition (and consult-lsp-use-marginalia (bound-and-true-p 'marginalia-mode))

[gagbo]

(this is more of task for myself that I note here to remember) I will probably extract that snippet into its own consult-lsp--file-symbols--transformer function, as I used the same naming convention for the functions that transform LSP data to candidates.

[gagbo]

I changed it a little bit, you can follow progress on the wip/file-symbols branch (whenever I feel it's ready to be merged I'll make you co-author of the squashed commit for sure, just so we're clear).

As you told me that you didn't test it with other servers, I tried with Rust, and I found out that there were a few things I missed, my main pain point was that you didn't use lsp:symbol-information-name and lsp:document-symbol-detail? to build candidates in your "transformer", so the completion interface was lacking a bit of extra info that the server was nonetheless giving us.

Before

With marginalia enabled

Without marginalia

See how it's hard to distinguish all the Rational entries)

In between

With marginalia enabled

I use the name given by LSP instead of the buffer substring)

Without marginalia

All details are still appended to candidates if present)

After

Instead of fontifying and adding the type back, the types are grouped just as with global-workspaces

About making dictatorial UI/UX decisions

I know all of those are opinionated choices, I plan on exposing all the --transformers and --annotate functions as defcustom so that anyone can build their own UI (that's the reason I wanted to extract all the logic in a separate function)

[gagbo]

I think I should do better work around fontification, not sure how/if it's possible by using the LSP provided name though. I'd still rather lose the fontification and have the full impl Trait for Rational in my completion candidates, than keep fontification but only seeing 4/5 times the word Rational in the candidates

[northgaard]

I was not expecting such a detailed response in such a short time, interesting work!

• Having thought about the marginalia thing, you are definitely right that it should probably be integrated "properly" in a separate file. Here I'm basically just using a helper function from it to right align the annotation, rather than adding them to the completion category and let marginalia mode handle the actual annotation, which would as far as I understand be the way to do it.
• I personally kind of like that the candidates appear in the order in which they are defined in the text file. It feels like it's a more intuitive "local" view of symbols whereas the type grouping makes sense for the existing "global" search. But if one can provide a custom transformer as you say, I am totally fine with it not being the default.
• Regarding fontification, it's a tricky issue. I definitely like that the fontification is there, but on the other hand it does mean that some entries become far less "searchable" (your Rational example is a good one, C++ destructors are another, they just appear as "~" in the candidate list). I will think a bit more on this.

I will try to review your branch and think about how I can contribute as soon as I have a bit of time. Cheers!

[gagbo]

Hi,

Actually I pushed work directly on this branch (forgot GH allows to do that), so you can ignore the wip/* one from now on.

• I'll handle marginalia later (in the same PR where I'll add more custom variables and stuff Feat/customize look and feel #17 ), I'll create a minor mode in a separate file to deal with it. It's easier to handle separately for now

• I'm not sure how to actually customize the "grouping or no grouping ?" for file-symbols, so I'll remove the grouping for file-symbols as you did, it makes sense.

• For fontification, it's hard. One solution might be to bundle both information in the candidate (something like

(concat
 (buffer-substr ... 'fontify)
 (format " (%s)" (details?))

I might try to check later.

[gagbo]

Thanks for the idea, I'll merge it as is to work on #17 now, but if anything seems weird feel free to open a new PR !