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

bug: html tag not removed in treesitter lsp markdown hover #424

Closed
3 tasks done
seblj opened this issue Apr 6, 2023 · 7 comments · Fixed by #431 or #639
Closed
3 tasks done

bug: html tag not removed in treesitter lsp markdown hover #424

seblj opened this issue Apr 6, 2023 · 7 comments · Fixed by #431 or #639
Labels
bug Something isn't working

Comments

@seblj
Copy link

seblj commented Apr 6, 2023

Did you check docs and existing issues?

  • I have read all the noice.nvim docs
  • I have searched the existing issues of noice.nvim
  • I have searched the existing issues of plugins related to this issue

Neovim version (nvim -v)

0.9.0-dev-1324+ge30cc8be1

Operating system/version

MacOs 13.1

Describe the bug

It doesn't properly remove all html tags for the markdown render of lsp docs

Neovim:

image

VsCode:
image

Note the <code>Option\<String\></code> in the neovim popup. I would expect something like in vscode where the tags are not included

Steps To Reproduce

Use this Rust file:

fn main() {
    let foo = Some("foo");
    foo.map(|_| "bar");
}

Try to hover the docs on map and notice the tags being there

Expected Behavior

I would expect something like this where the ` is gone:

image

Note I just set modifiable and removed the tags to make it look like it ideally should.

Repro

-- DO NOT change the paths and don't remove the colorscheme
local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs({ "config", "data", "state", "cache" }) do
  vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
  vim.fn.system({ "git", "clone", "--filter=blob:none", "https://github.com/folke/lazy.nvim.git", lazypath, })
end
vim.opt.runtimepath:prepend(lazypath)

-- install plugins
local plugins = {
  "folke/tokyonight.nvim",
  "folke/noice.nvim",
  -- add any other plugins here
}
require("lazy").setup(plugins, {
  root = root .. "/plugins",
})

vim.cmd.colorscheme("tokyonight")
-- add anything else here
@seblj seblj added the bug Something isn't working label Apr 6, 2023
@seblj seblj changed the title bug: markdown tag not removed in treesitter render of markdown bug: markdown tag not removed in treesitter render Apr 6, 2023
@seblj seblj changed the title bug: markdown tag not removed in treesitter render bug: markdown tag not removed in treesitter lsp hover Apr 6, 2023
@seblj seblj changed the title bug: markdown tag not removed in treesitter lsp hover bug: html tag not removed in treesitter lsp markdown hover Apr 6, 2023
@seblj
Copy link
Author

seblj commented Apr 7, 2023

It seems like something like this could work for the escape character:

diff --git a/lua/noice/text/markdown.lua b/lua/noice/text/markdown.lua
index cef6b13..bb84214 100644
--- a/lua/noice/text/markdown.lua
+++ b/lua/noice/text/markdown.lua
@@ -63,7 +63,7 @@ end
 function M.parse(text, opts)
   opts = opts or {}
   ---@type string
-  text = text:gsub("</?pre>", "```"):gsub("\r", "")
+  text = text:gsub("</?pre>", "```"):gsub("\r", ""):gsub("(\\)(%p)", "%2")
   text = M.html_entities(text)
 
   ---@type Markdown

However, I am not too sure about how we should deal with the <code> tag. Because if we change it for a backtick, then all the text inside will be interpreted as a literal string, and the conceal won't work for the url which makes it a bit ugly. So I think maybe just doing :gsub("</?code>", "") could work okay. If you have a better idea, let me know!

@seblj
Copy link
Author

seblj commented Apr 7, 2023

Another potential expected behaviour is to have it all be wrapped in backticks, but that would probably mean that the concealing of the url must be done in noice(?)

It seems like helix does it like this:

image

@seblj
Copy link
Author

seblj commented Apr 16, 2023

@folke that won't work correctly, because now the links will not be concealed properly:

image

@seblj
Copy link
Author

seblj commented Apr 16, 2023

Something like this seems to work good:

diff --git a/lua/noice/text/markdown.lua b/lua/noice/text/markdown.lua
index b14a2c0..1078679 100644
--- a/lua/noice/text/markdown.lua
+++ b/lua/noice/text/markdown.lua
@@ -64,7 +64,7 @@ end
 function M.parse(text, opts)
   opts = opts or {}
   ---@type string
-  text = text:gsub("</?pre>", "```"):gsub("\r", ""):gsub("</?code>", "`")
+  text = text:gsub("</?pre>", "```"):gsub("\r", ""):gsub("</?code>", ""):gsub("(\\)(%p)", "%2")
   text = M.html_entities(text)
 
   ---@type Markdown

@folke
Copy link
Owner

folke commented Apr 16, 2023

I've reverted my changes. This should be fixed in the treesitter markdown parser. \\ should probably be concealed when inside <code> tags

@folke
Copy link
Owner

folke commented Apr 16, 2023

Your suggested changes may work for rust, but will very likley break a lot of other markdown code

@seblj
Copy link
Author

seblj commented Apr 16, 2023

I don't think it will very likely break a lot of other markdown code. This fix has been suggested to upstream as well:

neovim/neovim#13803
neovim/neovim#14069

It looks like the reason they didn't go this route is because they wanted users to have more general control over syntax rules: neovim/neovim#14073

I agree with mjlbach on this: I'm betting the frequency of unintended rendered \ >> frequency of intended \

And by replacing <code> with a backtick will always break the concealing of url since then it isn't parsed as a url anymore. Fixing this would probably require a lot of change in the markdown_inline parser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants