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

Support for "LineNr" highlight #8

Closed
DanCardin opened this issue Nov 13, 2020 · 13 comments
Closed

Support for "LineNr" highlight #8

DanCardin opened this issue Nov 13, 2020 · 13 comments
Labels
enhancement New feature or request

Comments

@DanCardin
Copy link

I'm not sure if that's the correct name, but I've recently been keeping my sign column off because gitgutter is able to add highlights to the number column through GitGutterAddLineNr, GitGutterChangeLineNr, GitGutterDeleteLineNr, and GitGutterChangeDeleteLineNr.

@lewis6991 lewis6991 added the enhancement New feature or request label Nov 13, 2020
@lewis6991
Copy link
Owner

Thanks for the suggestion. I'm sure that it's something that can be easily implemented.

@romgrk
Copy link

romgrk commented Nov 20, 2020

Plus one here, otherwise it's not possible to have separate highlighting of text & signs.

It would be nice also to support existing GitGutterXxxLineNr groups, as a few colorscheme add support for that.

@lewis6991
Copy link
Owner

I've got this working locally (was pretty simple infact), the only thing I'm struggling with is how to configure this. My current plan is to add numhl: boolean to the config which just simply enables the use of GitSignsNum* highlight groups. The only reason for the enable switch is to stop the highlight groups from polluting the highlight namespace though I'm not sure how much that matters.

My other idea was to just take the highlight group used for signs and reverse it for the numhl but not sure if this is too restricted.

Opinions? Ideally I'd like enabling this feature to be as simple as possible (hence the second idea), but would also like to allow flexibility.

lewis6991 added a commit that referenced this issue Nov 22, 2020
@lewis6991
Copy link
Owner

Implemented this in #20. Can merge if people are ok with this.

@DanCardin
Copy link
Author

I found that I had to add

hi link GitSignsAddNr DiffAdd
hi link GitSignsChangeNr DiffModified
hi link GitSignsDeleteNr DiffDelete

in order to see any colors (which is fine if intended!). Works like a charm though, thanks!

@lewis6991
Copy link
Owner

The intention is that if GitSigns*Nr aren't defined, then they are defined automatically using the foreground of signs.*.hl as the background.

Can you check if something gets automatically defined by removing the hi links and running :hi GitSignsAddNr?

@DanCardin
Copy link
Author

I get GitSignsAddNr xxx cleared.

@lewis6991
Copy link
Owner

Interesting, that is unintended. Not sure why it doesn't work, the code is pretty simple:

  for t, sign_name in pairs(sign_map) do
    local cs = config.signs[t]

    if config.numhl and not hl_exists(cs.numhl) then
      -- If highlight 'numhl' is not defined, create it to be the reverse of
      -- texthl highlight fg->bg
      local ctermfg = api.nvim_get_hl_by_name(cs.hl, false).foreground
      local ctermbg = ctermfg and ('ctermbg=%d'):format(ctermfg) or ''
      local guifg   = api.nvim_get_hl_by_name(cs.hl, true).foreground
      local guibg   = guifg and ('guibg=#%x'):format(guifg) or ''
      if ctermbg ~= '' or guibg ~= '' then
        vim.cmd(('highlight %s %s %s'):format(cs.numhl, ctermbg, guibg))
      end
    end

    sign_define(sign_name, {
      texthl = cs.hl,
      text   = cs.text,
      numhl  = config.numhl and cs.numhl
    })

  end

With numhl disabled, can you verify the result of

:lua print(vim.api.nvim_get_hl_by_name('GitSignsAddNr', false))

is E5108: Error executing lua [string ":lua"]:1: Invalid highlight name: GitSignsAddNr

And if it's not much to ask, could you add a few print statements in the patch and see exactly why the highlights aren't being defined?

Otherwise we can just submit the PR as is and figure this issue at some other point in the future.

@DanCardin
Copy link
Author

Can confirm the above lua snippet errors in the expected way.

A few things I noticed:

  • not hl_exists(cs.numhl) is false, so this conditional is never entered

  • Even if i force it to enter that conditional, api.nvim_get_hl_by_name(cs.hl, false).foreground is nil, so it doesn't perform the highlight. .foreground doesn't seem to exist

    api.nvim_get_hl_by_name(cs.hl, false) seems to be yield {true = 6}.

    api.nvim_get_hl_by_name(cs.hl, true) seems to yield {background = 4346703}

    so if i do [true], and .background respectively, I finally get highlights (what seems to me the same colors i got from the link highlights i do above

  • Unsure if relevant but your PR also modifies https://github.com/lewis6991/gitsigns.nvim/blob/develop/lua/gitsigns.lua#L233, which seems to never be hit because cs.show_count doesn't exist (unless i either set it on the default sign config, or manually set my own config.

  • if i do that, i then get an error because it doesn't send the opts in as a table, like your above sign_define does.

@lewis6991
Copy link
Owner

Thanks for investigating.

not hl_exists(cs.numhl) is false, so this conditional is never entered

So that implies cs.numhl exists at that point? Or hl_exists isn't working properly.

api.nvim_get_hl_by_name(cs.hl, false) seems to be yield {true = 6}.
api.nvim_get_hl_by_name(cs.hl, true) seems to yield {background = 4346703}

Well that's strange. What's the definitions for the highlights you use in config.signs.*.hl? Maybe this code is making too many assumptions? Either that or my only other guess is that your neovim is too old. Are you running a recent build?

Unsure if relevant but your PR also modifies https://github.com/lewis6991/gitsigns.nvim/blob/develop/lua/gitsigns.lua#L233, which seems to never be hit because cs.show_count doesn't exist (unless i either set it on the default sign config, or manually set my own config.

This is for a different feature that I haven't documented yet (see #14) so I wouldn't expect that branch to execute.

Without being able to reproduce your env there's not much I can do. Would you be able to debug further? Otherwise we can just submit?

@DanCardin
Copy link
Author

So that implies cs.numhl exists at that point? Or hl_exists isn't working properly.

yes cs.numhl matches the default config values

What's the definitions for the highlights you use in config.signs.*.hl?

I'm not setting these, they are what the default values the plugin defines are.

your neovim is too old. Are you running a recent build?

I'm on a very recent (like friday) version of master of neovim. I can dig up the precise commit if you think that's important.
Or if you use nix (package manager) i can give you my precise overlay.

In any case, I can always just explicitly set the highlights myself that i mentioned above (which seem to exactly match the behavior that happens if i make the changes i mentioned in my previous comment to make it work), so I'm happy either way to keep debugging or leave it be

@lewis6991
Copy link
Owner

yes cs.numhl matches the default config values

The default numhl highlights aren't defined by default and get defined during setup via that conditional. You can find out if they are defined by running :highlight [hl group]. Assuming they aren't defined when they hit that conditional, can you try running

print(vim.inspect(vim.api.nvim_get_hl_by_name(GitSignsAddNr, _)))

before you run gitsigns.setup and report the output. This is essentially the implementation of hl_exists. My only guess right now is that function isn't working properly.

I'm not setting these, they are what the default values the plugin defines are.

Then you'll be using DiffAdd, DiffChange and DiffDelete. Can you find out what these are defined as in your colorscheme by running :highlight DiffAdd etc?

I'm on a very recent (like friday) version of master of neovim. I can dig up the precise commit if you think that's important.
Or if you use nix (package manager) i can give you my precise overlay.

Don't worry, I'm sure it's recent enough.

@DanCardin
Copy link
Author

DanCardin commented Dec 1, 2020

The default numhl highlights aren't defined by default and get defined during setup via that conditional.

Oh, I thought you meant literally cs.numhl which is 'GitSignsDeleteNr' etc. Where hl_exists(cs.numhl) yields true.

print(vim.inspect(vim.api.nvim_get_hl_by_name('GitSignsAddNr', _)))

Yields: GitSignsAddNr xxx cleared with gitsigns.setup {numhl = true} or
E5108: Error executing lua [string ":lua"]:1: Invalid highlight name: GitSignsAddNr without having called setup (or with setup where numhl = false).

Can you find out what these are defined as in your colorscheme by running :highlight DiffAdd etc?

DiffAdd        xxx guibg=#42534f
DiffChange     xxx guibg=#42534f
DiffDelete     xxx guibg=#534f56

lewis6991 added a commit that referenced this issue Dec 13, 2020
lewis6991 added a commit that referenced this issue Dec 13, 2020
lewis6991 added a commit that referenced this issue Dec 13, 2020
lewis6991 added a commit that referenced this issue Aug 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants