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

feat: add blink.cmp hlgroups (proceeds #263) #271

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

WieeRd
Copy link
Contributor

@WieeRd WieeRd commented Jan 3, 2025

Adds highlight groups for blink.cmp.

Fixed version of #263, which was using completely wrong highlight items.
Added support for ghost texts and label details as well.

jonsch318 and others added 3 commits November 22, 2024 10:50
Changes suggested in rebelot#263 (review)

- Prefer using links to Pmenu groups.
- Added missing `CmpGhostText` group.
- Removed some misplaced whitespaces.
- Did NOT change ghost texts and deprecated items to `NonText`
  since it might be just my personal preference.
@jonsch318
Copy link

jonsch318 commented Jan 3, 2025

Thanks yes i would have fixed it today :). I opened it in November, since then Blink has changed drastically, thus my highlight groups are wrong now.
However this is problematic because blink is still in beta and we should expect changes. So either:

  1. We/You update this pull-request until it is merged and then always keep it updated with changes from blink
  2. Or we wait until blink stabilizes (if ever) and then add it

@jonsch318
Copy link

But yes especially the BlinkCmpItemKind => BlinkCmpKind things i just missed. Although i still use it and it looks fine :/ Thanks for correcting it anyway

@WieeRd
Copy link
Contributor Author

WieeRd commented Jan 3, 2025

Oh, I should have been a bit more patient, just a few more hours.
I didn't know that highlights were named differently on earlier versions,
apologies on my rude review comment on your original PR.

Although i still use it and it looks fine :/

I assume you couldn't notice the breaking change due to the fallback to nvim-cmp highlights option. This option is planned to be removed in the future, so we do need explicit blink.cmp highlights.

However this is problematic because blink is still in beta and we should expect changes

Seeing how many Neovim related projects stay in 0ver for years, I doubt waiting for the stabilization is a good idea. And having outdated highlights in the config doesn't do much harm other than wasting a few nanoseconds of startup time. I'd say it's better to have this merged now and update it as blink.cmp evolves. Also I don't think there will be much reason to change highlight names yet again (hopefully).

I won't be having much time to spare this year. So if yet another breaking change happens to blink.cmp before this PR gets merged, please create a proceeding PR like I did. @jonsch318

@jonsch318
Copy link

I am largely finished with my thesis so i will have more time this year 👍

And yeah i will try to keep looking for breaking changes

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.

2 participants