Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more context to the symbol picker #2452

Closed

Conversation

mdelaney
Copy link

The symbol picker functionality is really useful; however, it can be annoying when you want to do something like only see the function symbols, or maybe only struct or enum kinds. It'd also be convenient to be able to filter by the containing symbol name.

This adds the symbol kind along with its direct parent symbol. So:

mod foo {
    fn bar() {}
}

will show symbols like

module    : foo
function  : foo → bar

This lets you quickly filter by the symbol kind, containing symbol or
symbol name.

Here is how this looks before (top) and after (bottom):
compare

The nice thing about this is that you can search by the symbol type (like function) to more quickly narrow in on what you want.
compare-search

By showing the containing symbol (if it exists) it becomes easy to search only symbols in the test module as well.


This also addresses a TODO and a bit of minor formatting cleanup.

This adds the symbol kind along with its direct parent symbol. So:
```rust
mod foo {
    fn bar() {}
}
```
will show symbols like
```
module    : foo
function  : foo → bar
```

This lets you quickly filter by the symbol kind, containing symbol or
symbol name.
@CptPotato
Copy link
Contributor

Some personal feedback, take it with a grain of salt:

The display of namespaces/modules with an arrow looked a little odd at first glance. I like that the module is shown, though.
Does it include all modules for deeply nested projects or only the last one?

Would it be possible to have the item type (function/enum/etc.) aligned to the right or does this break searching for fun xyz?
I'm seeing parallels to the completion popup which looks different from this PR's current formatting (right aligned, no dividing char like :). I'm not sure if it's worth it, but having more consistency in the UI might pay off. (there's also #1905 which adds colors to the completion popup).

@mdelaney
Copy link
Author

Hi @CptPotato, thanks for the feedback!

The display of namespaces/modules with an arrow looked a little odd at first glance

Yea I played around with some other possibilities like => or simply :. Is there any convention already established elsewhere for this? Very much willing to change this, biggest goal was to minimize the number of characters to keep as much room as possible for the symbols.

Does it include all modules for deeply nested projects or only the last one?

Unfortunately the symbol only contains the direct parent and didn't find a good way to get the rest of the hierarchy. I played around with building up a hash map but ran into some problems when there are symbols with the same name. It feels like this is something that should be solvable with tree-sitter (though admittedly I haven't played with that). If you have any specific ideas on how to get this I'm happy to play with this further (otherwise can very much put a TODO here).

Would it be possible to have the item type (function/enum/etc.) aligned to the right or does this break searching for fun xyz?

Could be. I'll play around with this in the next day or so. I agree it'd be kinda nice to have it right aligned if it doesn't make filtering more difficult.

@CptPotato
Copy link
Contributor

Thanks for elaborating!

I personally don't have a strong preference for how to separate the namespace from the symbol name. Different languages have different conventions here (like :: in Rust and C++ vs . in C#). I also thought about using / like it's used in paths, but users might mistake it for an actual file system path which isn't necessarily the case depending on the language.
Anyway, I agree that shorter is probably better to save on line width.

I also don't think it's a big issue that only the parent namespace is shown in the list for now. If a project has a very deep structure, it might clutter up the window and make it harder to see the actual symbol name on smaller screen sizes.

@mdelaney
Copy link
Author

The problem with only showing the immediately containing symbol is pretty well shown in helix-core/src/movement.rs in the test module. Consider the following enum Axis definitions.

this would show up something like:

enum      : vertical_moves_jumping_column → Axis
enum mem  : Axis → H
enum mem  : Axis → V
enum      : multibyte_character_wide_column_jumps → Axis
enum mem  : Axis → H
enum mem  : Axis → V

Definitely an improvement over the current output but it'd be much nicer to have the full nested structure (albeit truncating long names to make the length reasonable). (FWIW this is the exact example that broke my initial implementation 😛 )

All that to say, this still feels like a problem that should be solved (though maybe outside the scope of this PR). I suspect that this would be more problematic in some languages than others.

@mdelaney
Copy link
Author

@CptPotato - Ok, so after digging in a bit more....

Would it be possible to have the item type (function/enum/etc.) aligned to the right or does this break searching for fun xyz?

This isn't possible without significantly reworking how the picker is implemented. In the PR you linked the right alignment is done by using two Cells in the table widget.

That said, right aligning also makes filtering by symbol kind pretty awkward as the filtering is from left to right, so you'd have to do something like foo function but that eliminates the ability to first filter by the symbol kind and then by name.

I think there'd be a lot of value to reworking the picker to support style information to use color to differentiate pieces of information in the picker. Even before this PR, the workspace symbol picker has the symbol and file path, it'd be really great to have them styled differently to make it easy for the eye to parse the different pieces of information. (if there is general agreement on that direction I'd be happy to spend some time on it at some point)


I added a TODO for adding ancestor symbols.

For the separating symbol, my view is that a simple is probably the simplest least ambiguous option given that the symbol to the left literally contains the symbol to the right. This stays away from language specific symbols and is only a single char wide.

Any thoughts?

@CptPotato
Copy link
Contributor

I think reworking the picker to make alignment, coloring or searching work better is out of scope for this PR. It might be worth revisiting in the future, but for now I think having the item types added to the symbol picker is already a nice addition. 👍

Comment on lines 81 to 112
let kind = match symbol.kind {
lsp::SymbolKind::FILE => "file ",
lsp::SymbolKind::MODULE => "module ",
lsp::SymbolKind::NAMESPACE => "namespace ",
lsp::SymbolKind::PACKAGE => "package ",
lsp::SymbolKind::CLASS => "class ",
lsp::SymbolKind::METHOD => "method ",
lsp::SymbolKind::PROPERTY => "property ",
lsp::SymbolKind::FIELD => "field ",
lsp::SymbolKind::CONSTRUCTOR => "construct ",
lsp::SymbolKind::ENUM => "enum ",
lsp::SymbolKind::INTERFACE => "interface ",
lsp::SymbolKind::FUNCTION => "function ",
lsp::SymbolKind::VARIABLE => "variable ",
lsp::SymbolKind::CONSTANT => "constant ",
lsp::SymbolKind::STRING => "string ",
lsp::SymbolKind::NUMBER => "number ",
lsp::SymbolKind::BOOLEAN => "boolean ",
lsp::SymbolKind::ARRAY => "array ",
lsp::SymbolKind::OBJECT => "object ",
lsp::SymbolKind::KEY => "key ",
lsp::SymbolKind::NULL => "null ",
lsp::SymbolKind::ENUM_MEMBER => "enum mem ",
lsp::SymbolKind::STRUCT => "struct ",
lsp::SymbolKind::EVENT => "event ",
lsp::SymbolKind::OPERATOR => "operator ",
lsp::SymbolKind::TYPE_PARAMETER => "type param",
_ => "",
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see much benefits of aligning this, since they have a different group.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benefit is that it makes it easier for eyes to spot the symbol kind and the symbol name. The screenshot below shows the difference. The top (with alignment) makes it much easier to see at a glance. Is there any specific disadvantage that you are thinking of?

Screen Shot 2022-05-14 at 7 06 42 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that be solved if we just reorder the appearance? Or maybe the order is somehow useful?

/// the result will be: `function : foo → bar`
fn get_symbol_string(symbol: &lsp::SymbolInformation) -> String {
let kind = match symbol.kind {
lsp::SymbolKind::FILE => "file ",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some idea but not sure if it's good or not. Why not do [file] ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside of [symbol kind] is that it would take more horizontal space. It also feels a bit more cluttery (at least in my view). I think the ideal would be to eventually differentiate with color (which would also remove the need for the separator altogether).

lsp::SymbolKind::EVENT => "event ",
lsp::SymbolKind::OPERATOR => "operator ",
lsp::SymbolKind::TYPE_PARAMETER => "type param",
_ => "",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better if we log this out in case we missed it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great callout! Will add this.

@mdelaney mdelaney force-pushed the symbol-picker-improvements branch from 2611c36 to 1f29d08 Compare May 15, 2022 03:58
@the-mikedavis the-mikedavis added the S-waiting-on-review Status: Awaiting review from a maintainer. label May 18, 2022
Comment on lines +82 to +107
lsp::SymbolKind::FILE => "file ",
lsp::SymbolKind::MODULE => "module ",
lsp::SymbolKind::NAMESPACE => "namespace ",
lsp::SymbolKind::PACKAGE => "package ",
lsp::SymbolKind::CLASS => "class ",
lsp::SymbolKind::METHOD => "method ",
lsp::SymbolKind::PROPERTY => "property ",
lsp::SymbolKind::FIELD => "field ",
lsp::SymbolKind::CONSTRUCTOR => "construct ",
lsp::SymbolKind::ENUM => "enum ",
lsp::SymbolKind::INTERFACE => "interface ",
lsp::SymbolKind::FUNCTION => "function ",
lsp::SymbolKind::VARIABLE => "variable ",
lsp::SymbolKind::CONSTANT => "constant ",
lsp::SymbolKind::STRING => "string ",
lsp::SymbolKind::NUMBER => "number ",
lsp::SymbolKind::BOOLEAN => "boolean ",
lsp::SymbolKind::ARRAY => "array ",
lsp::SymbolKind::OBJECT => "object ",
lsp::SymbolKind::KEY => "key ",
lsp::SymbolKind::NULL => "null ",
lsp::SymbolKind::ENUM_MEMBER => "enum mem ",
lsp::SymbolKind::STRUCT => "struct ",
lsp::SymbolKind::EVENT => "event ",
lsp::SymbolKind::OPERATOR => "operator ",
lsp::SymbolKind::TYPE_PARAMETER => "type param",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think even if you want to align, it is better to remove the spaces, then use left align format specifier to do the spacing rather than having to do that manually. Doing it manually will cause changing the largest variant require changing all the lines, bad for git diff.

.clone()
.container_name
.map_or(String::new(), |name| format!("{} → ", name));
format!("{}: {}{}", kind, prefix, symbol.name.clone())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this look bad for the case in which we don't match? Given that now it will show : ......

@archseer
Copy link
Member

I think this is a good change, but rather than formatting the text by hand the picker UI needs to use the table component instead, so we can add a bunch of automatically aligned columns. #2013 is working towards that goal so let's park this PR for a bit

@kirawi kirawi added A-helix-term Area: Helix term improvements S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Sep 13, 2022
@the-mikedavis the-mikedavis mentioned this pull request Feb 17, 2024
@pascalkuthe
Copy link
Member

this will be covered by #9647

@pascalkuthe pascalkuthe closed this Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants