neodim icon indicating copy to clipboard operation
neodim copied to clipboard

Use the latest highlight

Open Avimitin opened this issue 1 year ago • 7 comments

nvim-treesitter introduce a new breaking change for nvim 0.8.0. They remove all the TS* highlight and map them to @*. So we can just combing "@" character with the match group.

Ref: https://github.com/nvim-treesitter/nvim-treesitter/commit/42ab95d5e11f247c6f0c8f5181b02e816caa4a4f

Avimitin avatar Oct 20 '22 03:10 Avimitin

Highlighting the group works but the code here doesn't for me.

zbirenbaum avatar Oct 21 '22 04:10 zbirenbaum

image

It's weird cuz the code works for me. I am also using the latest compiled neovim and the latest nvim-treesitter.

Avimitin avatar Oct 21 '22 04:10 Avimitin

image

It's weird cuz the code works for me. I am also using the latest compiled neovim and the latest nvim-treesitter.

I can try rebuilding my neovim. If this only works properly on nightly I'm not sure I want to merge it in the current state though. Some groups like parameters properly dim, but others like variables are not.

zbirenbaum avatar Oct 21 '22 04:10 zbirenbaum

image

In my environment all the highlight works as expect. But I did confirm that a has("nvim-0.8.0") can not guarantee it will work on every machines. I will try to figure out a proper way to make the code compatible with nightly and stable neovim users.

Avimitin avatar Oct 21 '22 04:10 Avimitin

I think I tracked down my issue and found that it's likely a colorscheme problem. But yeah, I severely doubt most colorschemes are up to date with this. I use my own, so its not a problem, but this will be a huge breaking change for most users.

It would be an option to check and see if a highlight is defined for the '@'..group with nvim_get_hl_by_name each time a new field is discovered, and then fall back to to_hl_group if not but I'm not sure what the overhead on that would be.

Another option, I think is more viable and less work to implement, would just be making it an opt in config option. Once it becomes more standard, it could be made the default with an opt out option. Typically deprecations last a full version cycle in neovim, so the legacy option could be removed then.

zbirenbaum avatar Oct 21 '22 04:10 zbirenbaum

Here is my trick, I found that neovim will always return a table for@ prefix highlight. But I expecting panic to distinguish the version. So I use the opposite way to fixed this problem: as treesitter remove all the TS* highlight in new version, try to get if TS prefix highlight exist will be a more better way to do this stuff.

And here is the patch, if you are satisfied with this, I will push the commit.

diff --git a/lua/neodim/util.lua b/lua/neodim/util.lua
index b8b4a28..58f6dc6 100644
--- a/lua/neodim/util.lua
+++ b/lua/neodim/util.lua
@@ -13,6 +13,14 @@ local to_hl_group = function (inputstr, sep)
   return 'TS' .. table.concat(t)
 end
 
+local to_ts_hl_group = function (str)
+  local still_old, _ = pcall(vim.api.nvim_get_hl_by_name, "TSWarning", true);
+  if still_old then
+    return nil
+  end
+  return "@"..str
+end
+
 M.get_treesitter_nodes = function(bufnr, row, col)
   local capture_fn = vim.treesitter.get_captures_at_pos or vim.treesitter.get_captures_at_position
   if capture_fn ~= nil then
@@ -25,7 +33,7 @@ M.get_treesitter_nodes = function(bufnr, row, col)
       return
     end
 
-    local hl_group = vim.fn.has("nvim-0.8.0") and "@"..matches[#matches] or to_hl_group(matches[#matches])
+    local hl_group = to_ts_hl_group(matches[#matches]) or to_hl_group(matches[#matches])
 
     return hl_group
   else

Avimitin avatar Oct 21 '22 05:10 Avimitin

In my opinion, giving an option to user, for the API-compatible issue, will generate a new maintenance problem. The best approach for users is that the working mechanism of the plugin should be a black box. They don't need to know if they have some "@" prefix highlight or "TS" prefix highlight, we do the job for them. And when the time the treesitter team stable the highlight variable has come, we silently remove the old code, and users don't even know.

Avimitin avatar Oct 21 '22 05:10 Avimitin

Sorry, I got a bit busy and this fell through the cracks. It looks good, thanks for implementing this, I'll merge now.

My one concern would be that some people may have legacy highlight groups defined except for TSWarning, but I'm not sure there's a much better method to do what you want to here.

zbirenbaum avatar Nov 02 '22 23:11 zbirenbaum