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(highlight): ns=0 to set :highlight namespace #17187

Merged
merged 1 commit into from
Feb 1, 2022

Conversation

lewis6991
Copy link
Member

Passing ns=0 to nvim_set_hl will alter the :highlight namespace.

@lewis6991 lewis6991 changed the title feat(highlight): ns=0 to set :highlight namespace [WIP] feat(highlight): ns=0 to set :highlight namespace Jan 24, 2022
@clason clason requested a review from bfredl January 24, 2022 09:39
@zeertzjq zeertzjq added the api libnvim, Nvim RPC API label Jan 24, 2022
@clason

This comment has been minimized.

src/nvim/syntax.c Outdated Show resolved Hide resolved
@lewis6991
Copy link
Member Author

Hmm, that seems to have trouble with the Normal highlight group being applied? (I get transparent background and pure white foreground.) This works when using a different namespace and nvim__set_hl_ns(ns).

Other groups seem to work, though.

Ok good to know.

FWIW I've done very little testing with this. Most of the work was just trying to understand how vim hl groups generally behave.

I'll pass this through a full colorscheme and and work out more of the edge cases; and of course add tests.

@clason

This comment has been minimized.

src/nvim/api/vim.c Outdated Show resolved Hide resolved
@lewis6991 lewis6991 marked this pull request as draft January 24, 2022 11:37
@lewis6991
Copy link
Member Author

Yeah, I just saw "non-draft" and jumped in :)

Ok. I did add a "WIP" to the title, but I've gone ahead and made this a draft as well.

I used this, if you want a pre-made one (just make the obvious change at the bottom): gist.github.com/clason/c164d718dcefbc27f08d3e0272cf93ae

Thanks. I also have my personal colorscheme to test for this: https://github.com/lewis6991/github_dark.nvim/blob/a0f6f75fc86f1a4f5e1760b0cda0c0258d51d2bc/lua/github_dark.lua#L30-L71, which was partially the motivation for working on this change.

@clason

This comment has been minimized.

@lewis6991
Copy link
Member Author

We don't do that anymore ;)

But I was mostly curious and wanted to give early feedback; feel free to ignore.

ACK.

Thanks, I really appreciate the feedback.

@lewis6991 lewis6991 changed the title [WIP] feat(highlight): ns=0 to set :highlight namespace feat(highlight): ns=0 to set :highlight namespace Jan 24, 2022
@lewis6991 lewis6991 closed this Jan 27, 2022
@lewis6991 lewis6991 reopened this Jan 27, 2022
@lewis6991 lewis6991 force-pushed the master branch 3 times, most recently from 00db039 to 5ab589d Compare January 31, 2022 21:06
@lewis6991
Copy link
Member Author

Hmm, that seems to have trouble with the Normal highlight group being applied? (I get transparent background and pure white foreground.) This works when using a different namespace and nvim__set_hl_ns(ns).

Other groups seem to work, though.

Finally, got round to debugging this, turns out Normal is special:

EXTERN int cterm_normal_fg_color INIT(= 0);
EXTERN int cterm_normal_bg_color INIT(= 0);
EXTERN RgbValue normal_fg INIT(= -1);
EXTERN RgbValue normal_bg INIT(= -1);
EXTERN RgbValue normal_sp INIT(= -1);

@clason

This comment has been minimized.

@lewis6991
Copy link
Member Author

lewis6991 commented Jan 31, 2022

(You're probably aware, but current code segfaults if you set highlights for a non-0 namespace, btw.)

I wasn't...

Thanks

EDIT: Fixed

@clason
Copy link
Member

clason commented Jan 31, 2022

Yep, now it works -- no crash, and Normal gets set correctly!

@lewis6991
Copy link
Member Author

Don't know why CI is failing. Let's un-draft to see what the other jobs do.

@lewis6991 lewis6991 marked this pull request as ready for review January 31, 2022 22:37
@clason
Copy link
Member

clason commented Jan 31, 2022

The CI Is a Harsh Mistress...

@lewis6991 lewis6991 marked this pull request as draft January 31, 2022 23:09
Copy link
Member

@clason clason left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll have to defer to @bfredl on the code itself.

@clason clason added this to the 0.7 milestone Feb 1, 2022
src/nvim/syntax.c Outdated Show resolved Hide resolved
Passing ns=0 to nvim_set_hl will alter the `:highlight` namespace.
@lewis6991
Copy link
Member Author

BTW, is there any mechanism we can use to detect this change in plugins? Lots of users have some dev build of v0.7 so it would be beneficial to provide some way of checking this.

My only idea would be to add something like vim._has_hl_api_ns0.

This applies to the signs PR too.

@clason
Copy link
Member

clason commented Feb 1, 2022

No, we don't do feature flags -- we only support:

  1. the latest stable version (0.6.1 atm)
  2. the latest commit on master (which already reports as 0.7.0)

Plugins are strongly recommended to follow the same policy. So in this (and the sign) case, plugins should just check has('nvim-0.7'). People on an older master build should be told to update. ("If you grab the nightly tiger by the tail...")

@lewis6991
Copy link
Member Author

No, we don't do feature flags -- we only support:

Ok I guess...

People on an older master build should be told to update.

This hasn't been so smooth in the past, but let's see how disruptive lewis6991/gitsigns.nvim#463 is.

@clason
Copy link
Member

clason commented Feb 1, 2022

Yeah, there's a segment of the audience who can't resist the temptation of running the latest and shiniest, but can resist the temptation of doing so in an informed manner.

The only thing that can be done is:

  1. be extra clear about the requirements and support guarantees (and stick to it when the complaints start) and
  2. wait a day or two for the official nightlies to include the feature (and maybe a fixup or two) before shipping a corresponding change in a plugin.

(Step 1 is exactly what I'm doing here, and anywhere else I can shoehorn it in.)

alexaandru added a commit to alexaandru/froggy that referenced this pull request Feb 2, 2022
alexaandru added a commit to alexaandru/froggy that referenced this pull request Feb 2, 2022
alexaandru added a commit to alexaandru/froggy that referenced this pull request Feb 2, 2022
alexaandru added a commit to alexaandru/froggy that referenced this pull request Feb 3, 2022
fitrh added a commit to fitrh/init.nvim that referenced this pull request Feb 3, 2022
alexaandru added a commit to alexaandru/froggy that referenced this pull request Feb 6, 2022
@zeertzjq zeertzjq modified the milestones: 0.8, 0.7 Mar 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api libnvim, Nvim RPC API highlight
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants