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

Injections are not properly parsed with smoothscroll and spell enabled #29452

Closed
yuukibarns opened this issue Jun 22, 2024 · 7 comments · Fixed by #29460
Closed

Injections are not properly parsed with smoothscroll and spell enabled #29452

yuukibarns opened this issue Jun 22, 2024 · 7 comments · Fixed by #29460
Labels
bug issues reporting wrong behavior treesitter

Comments

@yuukibarns
Copy link

Problem

Recording.2024-06-22.233322.mp4

Steps to reproduce

Here is my minimal config.

-- nvim-treesitter
local path = vim.fn.stdpath("data") .. "/nvim-treesitter"
vim.opt.rtp:prepend(path)

require("nvim-treesitter.configs").setup({
	ensure_installed = {
		"markdown",
		"markdown_inline",
		"latex"
	},
	highlight = { enable = true },
})

-- Enable conceal and spell for markup langs
local augroup = vim.api.nvim_create_augroup
local autocmd = vim.api.nvim_create_autocmd

autocmd("FileType", {
	group = augroup("ConcealSpell", {}),
	pattern = { "markdown" },
	callback = function(ev)
		vim.opt_local.spell = true
	end,
	desc = "Special Files",
})

-- options
local opt = vim.opt
opt.smoothscroll = true
opt.clipboard = "unnamedplus"

I download nvim-treesitter via lazy.nvim and clone the plugin in into nvim-data/nvim-treesitter and remove lazy.nvim to ensure a minimal configuration.

This two settings are essential.
It is only when I enable the both that the problem arises.

vim.opt_local.spell = true
opt.smoothscroll = true

Here is the output of :checkhealth nvim-treesitter

==============================================================================
nvim-treesitter: require("nvim-treesitter.health").check()

Installation ~
- OK `tree-sitter` found 0.22.6 (b40f342067a89cd6331bf4c27407588320f3c263) (parser generator, only needed for :TSInstallFromGrammar)
- OK `node` found v20.13.1 (only needed for :TSInstallFromGrammar)
- OK `git` executable found.
- OK `cc` executable found. Selected from { vim.NIL, "cc", "gcc", "clang", "cl", "zig" }
  Version: cc (GCC) 14.1.0
- OK Neovim was compiled with tree-sitter runtime ABI version 14 (required >=13). Parsers must be compatible with runtime ABI.

OS Info:
{
  machine = "x86_64",
  release = "10.0.22631",
  sysname = "Windows_NT",
  version = "Windows 11 Home China"
} ~

Parser/Features         H L F I J
  - latex               ✓ . ✓ . ✓
  - markdown            ✓ . ✓ ✓ ✓
  - markdown_inline     ✓ . . . ✓

  Legend: H[ighlight], L[ocals], F[olds], I[ndents], In[j]ections
         +) multiple parsers found, only one will be used
         x) errors found in the query, try to run :TSUpdate {lang} ~

Here is my .md file used in the video.
j.md

Expected behavior

The blue color should not vanish when the top of the buffer passes through the long paragraph.

Neovim version (nvim -v)

NVIM v0.10.0

Vim (not Nvim) behaves the same?

I don't know, I think the nvim-treesitter plugin is for neovim only.

Operating system/version

Windows 11 Home China

Terminal name/version

PowerShell 7.4.3

$TERM environment variable

I think it is not important.

Installation

nvim-win64.zip, latest release.

@yuukibarns yuukibarns added the bug issues reporting wrong behavior label Jun 22, 2024
@clason clason changed the title Long paragraph leads to failure of markdown highlight. Injections are not properly parsed with smoothscroll and spell enabled Jun 22, 2024
@clason
Copy link
Member

clason commented Jun 22, 2024

@luukvbaal @lewis6991

@luukvbaal
Copy link
Contributor

I doubt the fix for this will be in the smoothscroll/draw code. I'd image the treesitter runtime code(which I'm not at all familiar with) fails somewhere due to an incorrect/unexpected start_col or something?

@clason
Copy link
Member

clason commented Jun 22, 2024

Yes, the "smoothscroll" does not notify the treesitter code about the shifted viewport (like a regular scroll does) so the newly visible injections are not parsed like they should.

My suspicion is that there's a decoration provider (on_win) callback missing in the smoothscroll codepath.

@luukvbaal
Copy link
Contributor

luukvbaal commented Jun 22, 2024

My suspicion is that there's a decoration provider (on_win) callback missing in the smoothscroll codepath.

The thing is that that is not possible, the smoothscroll code is quite far removed from the actual drawing. Any window that is drawn will first trigger on_win, any line that is drawn first triggers on_line.
Perhaps the on_win callback needs the current skipcol? We may then want to actually pass that from C, but it is already available through winsaveview() as well.

@luukvbaal
Copy link
Contributor

Yeah this regressed in #26614. Following diff restores the highlighting. I haven't looked in as to what would be a reasonable adjustment to this logic to make it work with a smoothscrolled topline.

diff --git a/runtime/lua/vim/treesitter/highlighter.lua b/runtime/lua/vim/treesitter/highlighter.lua
index 003f7e0169..424743f23a 100644
--- a/runtime/lua/vim/treesitter/highlighter.lua
+++ b/runtime/lua/vim/treesitter/highlighter.lua
@@ -181,9 +181,9 @@ function TSHighlighter:prepare_highlight_states(srow, erow)
     local root_start_row, _, root_end_row, _ = root_node:range()

     -- Only consider trees within the visible range
-    if root_start_row > erow or root_end_row < srow then
-      return
-    end
+    -- if root_start_row > erow or root_end_row < srow then
+    --   return
+    -- end

     local highlighter_query = self:get_query(tree:lang())

@luukvbaal
Copy link
Contributor

luukvbaal commented Jun 22, 2024

Uhm the actual issue here is that the _on_spell_nav callback is called in win_line() -> spell_move_to() where it is called only to fetch the length of the misspelled word, and the actual cursor position is restored. Seems like we shouldn't invoke the _on_spell_nav callback in that case?

In this repro that just clears the correctly on_win populated "highlight state". But I'm not sure what the _on_spell_nav callback even does and if it should be called in win_line(), despite it's name suggesting that its just for spell navigation.

@luukvbaal
Copy link
Contributor

luukvbaal commented Jun 23, 2024

OK after reading into this some more this seems like an appropriate fix:

diff --git a/runtime/lua/vim/treesitter/highlighter.lua b/runtime/lua/vim/treesitter/highlighter.lua
index 003f7e0169..1eba07c080 100644
--- a/runtime/lua/vim/treesitter/highlighter.lua
+++ b/runtime/lua/vim/treesitter/highlighter.lua
@@ -377,11 +377,13 @@ function TSHighlighter._on_spell_nav(_, _, buf, srow, _, erow, _)
     return
   end

+  local highlight_states = self._highlight_states
   self:prepare_highlight_states(srow, erow)

   for row = srow, erow do
     on_line_impl(self, buf, row, true)
   end
+  self._highlight_states = highlight_states
 end

 ---@private

From what I understand, _on_spell_nav doesn't actually want to to modify the highlight state. Temporarily clearing, populating, and restoring the highlight state still allows the C code to detect whether the handled region should be spell checked, without modifying the highlight state (which probably wasn't anticipated to be affected by the win_line() -> spell_move_to() code path).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issues reporting wrong behavior treesitter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants