Skip to content

Fixed wrong completion icons#5999

Closed
DoctorKrolic wants to merge 2 commits into
dotnet:mainfrom
DoctorKrolic:fix-keywords-icons
Closed

Fixed wrong completion icons#5999
DoctorKrolic wants to merge 2 commits into
dotnet:mainfrom
DoctorKrolic:fix-keywords-icons

Conversation

@DoctorKrolic
Copy link
Copy Markdown

Fixes: #5793

Before:
devenv_8PTDbcJsDR
"for" and "foreach" completions has weird icons

After:
devenv_BV6Bf12wXg
All icons are ok

Yet another bug, occured because of completions post processing. In general having such post processing is the opposit of an idea of a language server, because every client needs to implement this post processing on his own. I'm not a fan of this approach at all! 😠 All functionality should be processed on the server side!

@NTaylorMullen
Copy link
Copy Markdown

Yet another bug, occured because of completions post processing. In general having such post processing is the opposit of an idea of a language server, because every client needs to implement this post processing on his own. I'm not a fan of this approach at all! 😠 All functionality should be processed on the server side!

100% Agree @DoctorKrolic! We actually have a backlog item to migrate our language server to be purely language server driven where through custom messages the server itself can delegate to C# etc. and then post-process on the language server if needed. It's a bit longer leaded but will ultimately get us into a position to make experiences better.

Love that you're digging through all this code and helping us out here ❤️

Copy link
Copy Markdown

@NTaylorMullen NTaylorMullen left a comment

Choose a reason for hiding this comment

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

Once again you're awesome! Thank you for the contribution and help!

@DoctorKrolic
Copy link
Copy Markdown
Author

This is no longer a valid PR as #6012 was merged, closing it

@DoctorKrolic DoctorKrolic deleted the fix-keywords-icons branch January 28, 2022 19:55
@NTaylorMullen
Copy link
Copy Markdown

This is no longer a valid PR as #6012 was merged, closing it

Oh geez, I should have merged this earlier for the contribution though. My apologies @DoctorKrolic !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Icons change for the @for intellisense and more

2 participants