completions: Add colors based on the LSP item kind #1905
Conversation
danyspin97
force-pushed
the
add_colors_to_completions
branch
2 times, most recently
from
17aee10
to
45e5938
Compare
|
Is there anything else I need to do for this PR? |
|
Other than the |
the-mikedavis
added
the
S-waiting-on-review
danyspin97
force-pushed
the
add_colors_to_completions
branch
from
45e5938
to
ceacb1c
Compare
|
My holdup here is that with the metadata colorized, it pulls away focus from the actual completions. From what I can tell it's not colorized in VSCode either, instead the icon is colorized. |
|
Considering the final result from #2190 where the completions have the matching highlighted, I don't think the colors pull away the focus too much. Plus, once I learn the matching color/category, I'd be easier for me to search something in the completion menu. But I'd say that the colored icon would make the same result as well. |
|
What if we put a cursor block (like the terminal cursor block) on the left of the metadata with color? That way only one character have color and it looks nice. We could even color the whole metadata by background, that looks nice too but feels like powerline to me and maybe a bit much. |
I think we could try that. |
|
Colored background: Colored blocks: I have tried both method and I think the original one looks cleaner. The one with the background color could look better, providing correct foreground color and better spacing, but I agree with @pickfire that it clutters the interface a bit too much. The other one with the colored blocks doesn't help much for me. |
danyspin97
force-pushed
the
add_colors_to_completions
branch
from
ceacb1c
to
27909d2
Compare
|
maybe it's more feasible to join forces with #2869 and use the standard solution? |
danyspin97
force-pushed
the
add_colors_to_completions
branch
from
27909d2
to
7979f6a
Compare
|
Rebased the PR :) |
There was a problem hiding this comment.
I'm not a fan of adding a field to the function signature of the trait just to pepper every single concrete call with an unused parameter, except in one place. I do like the idea of using the theme to decide the color though. Not really sure how else to do it either, since I think it does have to be in the rendering loop. Not really sure if there's a way around it.
| .map(|(theme, style)| theme.get(style)) | ||
| .unwrap_or_default() | ||
| .remove_modifier(Modifier::all()) | ||
| .add_modifier(Modifier::ITALIC); |
There was a problem hiding this comment.
Why are we intentionally removing the modifier from the theme and hardcoding italics? What's the point of pulling in user themes if we're just going to ignore them? I personally can't stand italics in code.
There was a problem hiding this comment.
We are pulling the theme to apply the colors. This is for the completions items, they should have a matching modifier, i.e. no bold but all in italic. Please have a look at the screenshots above.
There was a problem hiding this comment.
Yeah, in my opinion, we should just let the theme decide how to style the completion items here and not hard code style for the user.
| @@ -33,11 +33,57 @@ impl menu::Item for CompletionItem { | |||
| .into() | |||
| } | |||
|
|
|||
| fn format(&self, _data: &Self::Data) -> menu::Row { | |||
| // fn label(&self, _data: &Self::Data) -> Spans { | |||
There was a problem hiding this comment.
Did you mean to remove this commented code?
There was a problem hiding this comment.
Yea, this slipped through.
It was different before. The function was called Another important thing to note is that here the theme is only applied to the items that are shown to the user. To ensure the same, we will still need some way to pass this information from the caller of |
|
closing this one as its gone stale with many conflicts. I am also working on more completion changes that will conflict even more with this. I am not fundmentally opposed to it but I don't think it adds a lot and is fairly low priority and should probably be deffered until other larger changes are done. Thank you for contributing! |
This is the result:
I was searching for a way to avoiding passing the theme to the row() function, but I couldn't think of anything, I am still new to the codebase. Let me know if there is any better way to to do and I'll reimplement the feature.
I don't have much experience in theming, so I tried to match all the LSP items with a respective parameter of the theme. However, I am not sure that they are all correct, and I have to leave out some of them.