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(#2415): highlight overhaul (#2454) #2455

Merged
merged 38 commits into from
Jan 20, 2024
Merged

Conversation

alex-courtis
Copy link
Member

@alex-courtis alex-courtis commented Oct 8, 2023

fixes #2415

@gegoune @Akmadan23 apologies for this very large change; breaking it up was not feasible.

I'd be most grateful if you could test this for a week or so...

@Akmadan23
Copy link
Collaborator

Akmadan23 commented Nov 16, 2023

I noticed that renderer.highlight_git was missing from the accepted strings, so I simply added it.

By the way, I am working on adding type annotations to almost all functions, and I was wondering if would be worth it to do it now and deal with the conflicts with this branch or wait for this to be merged... What do you think?

@alex-courtis
Copy link
Member Author

I noticed that renderer.highlight_git was missing from the accepted strings, so I simply added it.

By the way, I am working on adding type annotations to almost all functions, and I was wondering if would be worth it to do it now and deal with the conflicts with this branch or wait for this to be merged... What do you think?

That's fantastic! It's been bothering me but I never seem to get a chance.

The big one would be a Node class... lot's of surface area last time I experimented...

You go first, this branch seems pretty localised and should merge without dramas.

@Akmadan23
Copy link
Collaborator

The big one would be a Node class...

That's the only one I haven't added yet, I'll try my best.

You go first, this branch seems pretty localised and should merge without dramas.

Yeah, maybe I can skip the renderer module since you are rewriting it. That was my only concern.

@alex-courtis
Copy link
Member Author

That's the only one I haven't added yet, I'll try my best.

Oh wow! Sounds like you're making great progress...

@Akmadan23
Copy link
Collaborator

Akmadan23 commented Dec 17, 2023

After some testing I noticed that the "partial" line highlight isn't consistent with all icon types, namely the default file icon is not included in the line highlight whereas (probably?) all other icons are. I took a quick look at the code but I didn't find the issue, but you surely know better where to look.

Edit: turns out it's related to the fact that NvimTreeIndentMarker is linked to Normal, which in my case has transparent background. The weird part is that the transparent highlight takes precedence over the line highlight...

Edit 2: turns out this "incomplete" line highlight is due to my on_attach function which overrides the Normal hl group. It works correctly when I open nvim-tree normally, but it doesn't on nvim .. This is weird because on master it works fine (kind of), however there is no need to fix it now since it's very much an edge case. I can eventually dive into it to and see what's the issue.

@alex-courtis
Copy link
Member Author

Edit 2: turns out this "incomplete" line highlight is due to my on_attach function which overrides the Normal hl group. It works correctly when I open nvim-tree normally, but it doesn't on nvim .. This is weird because on master it works fine (kind of), however there is no need to fix it now since it's very much an edge case. I can eventually dive into it to and see what's the issue.

Many thanks for all the detailed testing!

That should probably be fixed before release - I'm sure you're not the only one. Can you please link your on_attach so that I can attempt a fix?

I have little available time right now however I will over the new year so I'll get onto it then.

@Akmadan23
Copy link
Collaborator

Akmadan23 commented Dec 19, 2023

This is my minimal on_attach:

on_attach = function()
  vim.schedule(function()
    local winnr = require("nvim-tree.view").get_winnr()
    if winnr then
      local ns = vim.api.nvim_create_namespace("NvimTree")
      vim.api.nvim_win_set_hl_ns(winnr, ns)
      vim.api.nvim_set_hl(ns, "Normal", { fg = "#F8F8F2", bg = "#272822" })
    end
  end)
end

I use this namespace trick to have a different background color in nvim-tree windows.

This is weird because on master it works fine (kind of)

What I meant here is that on master the current line highlight isn't interrupted, but the background color is not the one I specified in my on_attach, instead uses the default Normal background color without transparency, so that's still pretty weird...

Edit: I tested the master branch with NvimTreeIndentMarker linked to Normal and the interrupted line highlight comes up, so nothing in this PR created the issue, it's probably something going on with conflicting highlights in different namespaces, and it has always been around. This is why I think this doesn't need to be addressed here.

@alex-courtis
Copy link
Member Author

Edit: I tested the master branch with NvimTreeIndentMarker linked to Normal and the interrupted line highlight comes up, so nothing in this PR created the issue, it's probably something going on with conflicting highlights in different namespaces, and it has always been around. This is why I think this doesn't need to be addressed here.

I see, thank you. So long as I'm not causing a regression...

@alex-courtis
Copy link
Member Author

alex-courtis commented Dec 25, 2023

There is a problem with additive highlights, which "limit 2" does not address properly, as it removes highlighting that should not be, such as icon colour.

:hi NvimTreeBookmarkHL guibg=blue
:hi NvimTreeCopiedHL guibg=magenta
:hi NvimTreeCutHL guibg=red
  vim.api.nvim_buf_add_highlight(0, -1, "DevIconC", 1, 2, 5)
  vim.api.nvim_buf_add_highlight(0, -1, "NvimTreeBookmarkHL", 1, 2, 5)
  vim.api.nvim_buf_add_highlight(0, -1, "NvimTreeCopiedHL", 1, 2, 5)
  vim.api.nvim_buf_add_highlight(0, -1, "NvimTreeCutHL", 1, 2, 5)

Icon is drawn in C blue, with blue bookmark HL.

Move the DevIconC to the end and it's drawn in magenta copied HL.

It seems that only one attribute may be overridden.

Possible solution: build and apply dynamic highlight groups based on combining highlight groups. These could be cached.

#2601

@alex-courtis alex-courtis marked this pull request as draft January 1, 2024 01:47
* feat(#2415): create combined highlight groups

* feat(#2415): create combined highlight groups

* feat(#2415): create combined highlight groups

* ci: allow workflow_dispatch (#2620)

* one and only one hl namespace, required winhl removal
collapse Builder
@alex-courtis alex-courtis marked this pull request as ready for review January 20, 2024 05:07
@alex-courtis
Copy link
Member Author

Pulling the trigger. It's been stably dogfooded for 2 weeks.

I have 3 days to deal with any regressions or questions.

@alex-courtis alex-courtis merged commit e9c5abe into master Jan 20, 2024
4 checks passed
@alex-courtis alex-courtis deleted the 2415-highlight-overhaul branch January 20, 2024 05:12
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.

Highlight Overhaul
3 participants