LSP icon indicating copy to clipboard operation
LSP copied to clipboard

When a server returns SymbolInformation[] in textDocument/documentSymbol, show the containerName

Open genericptr opened this issue 4 years ago • 35 comments

Document symbols can and should support the containerName in the UI.

https://microsoft.github.io/language-server-protocol/specifications/specification-3-15/#textDocument_documentSymbol

/** The name of the symbol containing this symbol. This information is for user interface purposes (e.g. to render a qualifier in the user interface if necessary). It can't be used to re-infer a hierarchy for the document symbols. */ containerName?: string;

For example something like this would be helpful to give context to the symbol without affecting the filtering behavior.

Screen_Shot_2020-06-30_at_12 16 15_PM

genericptr avatar Jun 30 '20 12:06 genericptr

We actually have the entire parent hierarchy available to us in symbols.py. So we could display the hierarchy flattened, separated by a token.

My problem here is that different languages use different separator tokens.

For C++ (maybe Rust too?) the separator token is :: For JSON the separator token is / (to make it look like a JSON pointer). For JS the separator is . I'm assuming.

A neutral separator could be

  • / to make the item look like a "file hierarchy"
  • (a space) to make the items visually separated by a space

I'm currently leaning towards / as it feels familiar with CTRL+P.

rwols avatar Jul 17 '20 08:07 rwols

We really need minihtml to make this world properly.

can we use tabs to separate them? what about | ? -> comes to mind also. I have not string opinion I guess but we should lobby for minihtml as a better solution.

genericptr avatar Jul 17 '20 08:07 genericptr

If we join parent symbol with current symbol then that will affect filtering also. Would that be something we want or should we not join?

Unless we want to show the joined path in subfield in which case that will probably not affect filtering

rchl avatar Jul 17 '20 08:07 rchl

I'm not sure what minihtml can bring to the table here.

I'm talking about the following problem:

Given that I have a list of "names":

["MyNamespace", "CMyClass", "EMyNestedEnum"]

I'm assuming for Pascal it'd be best to render it in the quick panel as

MyNamespace.CMyClass.EMyNestedEnum

If this were JSON, it'd be most natural to render it as

MyNamespace/CMyClass/EMyNestedEnum

And for C++ it'd be best to render it as

MyNamespace::CMyClass::EMyNestedEnum

rwols avatar Jul 17 '20 08:07 rwols

If we want to join the we should find some generic symbol (some utf8 character for example) that would represent descendants well. Nothing that matches any particular language as that would get confusing for other languages.

rchl avatar Jul 17 '20 08:07 rchl

You're talking about showing the entire hierarchy. Honestly that may present its own sets of problems given how long the string could be become (maybe expose this as an option?) I don't think this is really syntax question but rather a visual design question.

What about emojis? :)

genericptr avatar Jul 17 '20 08:07 genericptr

If we want to join the we should find some generic symbol (some utf8 character for example) that would represent descendants well. Nothing that matches any particular language as that would get confusing for other languages.

I agree. Something like • would be good enough for me. Or emojis like ▶️ perhaps, but I'm not sure if those are allowed. I mentioned minihtml because I would prefer some smaller but different style text, kind of like a sub caption with a background color maybe even.

genericptr avatar Jul 17 '20 08:07 genericptr

We could be absolute nerds and use ("is superset of") (or possibly a unicode arrow )

rwols avatar Jul 17 '20 11:07 rwols

After playing around with this a bit I think it’s better if the flattened hierarchy is in the first row (so it influences the filtering), and if the separator is always /.

rwols avatar Jul 17 '20 18:07 rwols

but we don't want to filter the container name do we? That could interfere with searching potentially. Also the first rows text is larger so this long strings will cause truncating/clipping and maybe make the actual symbol name hard to see. Maybe consider making this an option.

Screen Shot 2020-07-18 at 9 03 31 AM

genericptr avatar Jul 18 '20 02:07 genericptr

Exactly, it would potentially make filtering harder as when looking for ParentB in your example, you would also get results for all symbols that have it in inheritance path. So the initially selected one might not be the one you want, requiring more effort from the user to select the correct one.

It might be better to show the full path in the second row so that it doesn't affect the filtering. Disadvantage is that that would make results take more space.

rchl avatar Jul 18 '20 08:07 rchl

OK, the majority has decided ;)

vscode-json:

Screenshot from 2020-07-18 11-37-29

clangd:

Screenshot from 2020-07-18 11-37-47

rwols avatar Jul 18 '20 09:07 rwols

I would suggest > surrounded by spaces for the separator symbol, i.e. ParentA > ParentB > ParentC. That's often used in e.g. website navigation to display a hierarchy.

jwortmann avatar Jul 18 '20 14:07 jwortmann

Looks good but I'm not so thrilled with the 3 lines now since we can see fewer items now. I assume they are going to add icons the quick panel eventually but for now that's just lots of wasted space. Could it be an option to add the symbol kind as a prefix to the container name with a space or some other separator symbol? Since ST4 they really trimmed down the list height and on my laptop I can only see a 3-4 symbols sometimes.

genericptr avatar Jul 19 '20 03:07 genericptr

The quick panel could be replaced with a ListInputHandler, which is more compact. Only the labels for the items are in the list in that case, and additional information can be shown at the bottom of the widget (supporting minihtml).

jwortmann avatar Jul 19 '20 10:07 jwortmann

But that would require to put the command in a .sublime-commands file, in which case it'll show up in the command palette, which is not ideal.

Highly off topic because you're not on Discord, but I made a basic LSP-Julia helper package. Though I don't program in Julia myself, and there's probably a bunch of things wrong with the setup. Are you interested in taking over maintenance @jwortmann ? Note that it's ST4-only.

rwols avatar Jul 19 '20 11:07 rwols

Your LSP-julia package is most likely outdated, because I already made one with lots of features at https://github.com/jwortmann/LSP-julia 😉 Currently it uses the old LSP API. I'm planning to update it to the new API, but only when ST 4 comes publicly available because I would like to test it first and I'm... well, kind of still on my evaluation period for Sublime Text. I think I would put in on Package Control then for ST 4 exclusive, when the API calls have been updated.

jwortmann avatar Jul 19 '20 11:07 jwortmann

Excellent, do you want to move that repo to the sublimelsp organization? I'll send an invite regardless and see what happens.

rwols avatar Jul 19 '20 11:07 rwols

The quick panel could be replaced with a ListInputHandler, which is more compact. Only the labels for the items are in the list in that case, and additional information can be shown at the bottom of the widget (supporting minihtml).

That sounds perfect but I don't understand the implications of what rwols said. Making the list more compact would be great since some users (like me) like to browse the list to some extent and on laptops this is going to be very limited now.

genericptr avatar Jul 19 '20 12:07 genericptr

The HTML content can only be shown for the currently selected item though, I believe. Not sure it's a deal-breaker or not but it's probably not ideal.

rchl avatar Jul 19 '20 12:07 rchl

The HTML content can only be shown for the currently selected item though

That's correct, but I would see this as an advantage due to the saved space, rather than a drawback. Here is an example of a ListInputHandler in ST 3, that I made based on the guide at https://docs.sublimetext.io/guide/extensibility/plugins/input_handlers.html#providing-a-list-of-options-with-listinputhandler:

ListInputHandler

This is an example with only a single line at the bottom of the widget, but it supports minihtml including linebreaks and text styling.

I'm not sure whether it's really not possible to implement as @rwols said, because the required command already exists; it's just "LSP: Document Symbols", and the ListInputHandler could be used via adding an input method there. The request to the language server would then be made inside its list_items method. But I might be wrong because I don't have a good overview about the currently used logic for "Document Symbols" in this package.

jwortmann avatar Jul 19 '20 13:07 jwortmann

The HTML content can only be shown for the currently selected item though

That's correct, but I would see this as an advantage due to the saved space, rather than a drawback.

Depends on the use case. In this case, I'm not sure since when you are, for example, looking at three similarly named symbols and want to decide which one to navigate to, you might want to see additional information without having to select each of them separately first.

rchl avatar Jul 19 '20 13:07 rchl

Depends on the use case. In this case, I'm not sure since when you are, for example, looking at three similarly named symbols and want to decide which one to navigate to, you might want to see additional information without having to select each of them separately first.

That could be very well be true. The extra 3rd line is really pushing it for me in terms of space. The symbol name is very short so it's wasteful to have it occupy an entire line. It's also the information I'm least interested.

genericptr avatar Jul 19 '20 13:07 genericptr

I just noticed something. We were talking about separator characters but SymbolInformation only has a "containerName" field. which is a single string and doesn't actually show a hierarchy. It's going to be the servers responsibility to format this string I believe.

genericptr avatar Jul 19 '20 13:07 genericptr

I just noticed something. We were talking about separator characters but SymbolInformation only has a "containerName" field. which is a single string and doesn't actually show a hierarchy. It's going to be the servers responsibility to format this string I believe.

No, that containerName from SymbolInformation should be the direct parent. But you shouldn't use SymbolInformation because it's deprecated. Instead return a list of DocumentSymbol structures.

rwols avatar Jul 19 '20 14:07 rwols

Where are you seeing it's deprecated?

Not correct, it's just a string. It's intentionally for non-hierarchical symbols.

SymbolInformation[] which is a flat list of all symbols found in a given text document. Then neither the symbol’s location range nor the symbol’s container name should be used to infer a hierarchy.

genericptr avatar Jul 19 '20 14:07 genericptr

You're right, looking at the spec it seems it's not deprecated. In my opinion, editors can make better use of DocumentSymbol, because the definition of that struct is more "strict", so we can rely upon a more coherent structure. My changes will only affect DocumentSymbol because I know what to expect from the protocol.

rwols avatar Jul 19 '20 14:07 rwols

The entire point of my request was to support containerName is SymbolInformation. :) It sounds like you have a better way to use DocumentSymbols but SymbolInformation is still useful and will be used by servers so it should be properly implemented.

genericptr avatar Jul 19 '20 14:07 genericptr

Another issue has been started about how to improve the API for us (https://github.com/sublimehq/sublime_text/issues/3489). If this gets implemented it will be great for LSP but I still stand that SymbolInformation should be properly implemented to include all relevant information.

genericptr avatar Jul 21 '20 01:07 genericptr

See #1260

rwols avatar Aug 19 '20 19:08 rwols

This seems to be fixed.

rwols avatar Dec 30 '20 16:12 rwols

What version was this fixed in?

genericptr avatar Dec 30 '20 16:12 genericptr

I thought this was about improving textDocument/documentSymbol, but upon closer inspection this issue asks for showing the containerName for when the server returns SymbolInformation[].

rwols avatar Dec 30 '20 18:12 rwols

Actually, that also already happens: https://github.com/sublimelsp/LSP/blob/b399ee2380ecb17d8ff43949f4a2b5322c869f14/plugin/symbols.py#L194

@genericptr can you describe what it is that you're missing?

rwols avatar Dec 30 '20 18:12 rwols

It's been a long time but my last comment on #1260 I think is what this was about. Basically the container name should be included where the type name is shown now. Did this get implemented yet?

genericptr avatar Dec 31 '20 00:12 genericptr