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: allow gx to function for markdown links #28630

Merged
merged 1 commit into from
May 24, 2024

Conversation

dundargoc
Copy link
Member

@dundargoc dundargoc commented May 3, 2024

In other words, gx works regardless of where it was used in
[...](https://...). This only works on markdown buffers.

@dundargoc dundargoc marked this pull request as ready for review May 3, 2024 18:37
@dundargoc dundargoc requested review from justinmk and removed request for justinmk May 3, 2024 19:41
@ribru17
Copy link
Contributor

ribru17 commented May 3, 2024

Since the markdown parser is bundled could we use Tree-sitter for this? The downside is that maybe the grammar changes (affecting the node names) but it seems like this could be easier:

  local current_node = get_node { ignore_injections = false }
  while current_node do
    local type = current_node:type()
    if type == 'link_destination' then
      return do_open(vim.treesitter.get_node_text(current_node, 0))
    elseif type == 'inline_link' or type == 'image' then
      return do_open(vim.treesitter.get_node_text(current_node:named_child(1), 0))
    elseif type == 'link_text' or type == 'image_description' then
      return do_open(vim.treesitter.get_node_text(current_node:next_named_sibling(), 0))
    end
    current_node = current_node:parent()
  end

@dundargoc dundargoc marked this pull request as draft May 3, 2024 20:00
@dundargoc
Copy link
Member Author

Since the markdown parser is bundled could we use Tree-sitter for this?

I dunno if there's any problems with that, I'll need to ask the ts squad: @neovim/treesitter

@dundargoc dundargoc marked this pull request as ready for review May 3, 2024 20:03
@dundargoc

This comment was marked as resolved.

@ribru17

This comment was marked as resolved.

@clason

This comment was marked as resolved.

@dundargoc
Copy link
Member Author

Yes, if the language name is not equal to the filetype (or registered as an alias), you need to specify it explicitly.

Syntax aside, is relying on the markdown parser acceptable in this case?

@lewis6991
Copy link
Member

lewis6991 commented May 3, 2024

What is the benefit in using the Markdown parser? This seems easy enough to parse manually.

@ribru17
Copy link
Contributor

ribru17 commented May 3, 2024

@lewis6991 No real reason if it is easy enough to parse manually; that said I do think the code looks a bit simpler (and using the parser would handle cases with escaped \) or \] that I think needs to be handled specially and might not work with the current manual code (not 100% sure about this))

@ribru17
Copy link
Contributor

ribru17 commented May 3, 2024

Also the inline parser is pretty flat so traversing up nodes like that (only needed when there is e.g. an emphasis node inside the link text node) won't be too expensive

@dundargoc
Copy link
Member Author

I tried the treesitter suggestion (with some adjustments to get rid of type warnings). It works as expected. I suspect this might be more robust than my initial PR. Thanks for the suggestion.

I will leave this up for a bit to see if there are more input on this.

@dundargoc dundargoc force-pushed the feat/gx branch 3 times, most recently from 0766870 to 8cc1769 Compare May 3, 2024 21:19
@dundargoc dundargoc requested review from a team and justinmk May 3, 2024 21:47
@dundargoc dundargoc marked this pull request as draft May 6, 2024 10:35
@dundargoc dundargoc added this to the 0.11 milestone May 6, 2024
@dundargoc dundargoc force-pushed the feat/gx branch 2 times, most recently from 228a1bd to 331d55b Compare May 6, 2024 15:29
@dundargoc
Copy link
Member Author

Edit: Also should this code check first if the user is in a markdown buffer? I think if this is run on another filetype it will still try to parse it and then potentially return back some nonsense nodes (if there happens to be text that the inline parser can process). E.g. I tried running this on CMakeLists.txt and it sometimes thought I was in a shortcut link, if there was a string like [[ ... ]]

I see what you mean. The parser thinks the text inside [[ ]] is a link_text. This did cause a problem before when we explicitly matched for link_text, but if we only look for inline_link like we do now the problem seems to go away.

@ribru17
Copy link
Contributor

ribru17 commented May 8, 2024

Ah I see; but couldn't it be the case that there happens to be some language using some weird [...](...) syntax that will still affect the current version?

@dundargoc
Copy link
Member Author

dundargoc commented May 8, 2024

Theoretically, yes. But I still wonder if the benefit of always enabling this isn't greater than the potential clash with a language that may or may not exist. I certainly can't come up with any. I don't think this decision is irreversible, if we notice it causes any problems for some filetype we can further evaluate and iterate on this. I'm not dead set on either approach though, so input welcome. @justinmk any thoughts?

@dundargoc dundargoc marked this pull request as ready for review May 9, 2024 16:58
@justinmk
Copy link
Member

justinmk commented May 12, 2024

Tested locally. It works, but it seems that the parse tree gets "stale". If I use gx on [foo bar baz](https://example.com/?a=1&b=2) then make random edits and try gx again, then undo and try again, ... the results are just random.

Possibly #28715 is needed, or just need to re-parse. Or maybe we should use vim.treesitter.get_string_parser() instead?

@dundargoc dundargoc marked this pull request as draft May 12, 2024 13:39
@dundargoc dundargoc force-pushed the feat/gx branch 7 times, most recently from fc164f3 to 5bfef49 Compare May 12, 2024 19:20
@dundargoc dundargoc marked this pull request as ready for review May 12, 2024 19:20
@icholy
Copy link
Contributor

icholy commented May 18, 2024

Would it be possible to export this as a callable function? Or add a way to hook into it?

I maintain a plugin which adds support for LSP document links. It has a gx function which opens the link under the cursor or falls back to the default <cfile> behaviour if one doesn't exist.

If this change goes in as-is, I no longer have a simple way to fallback to the default.

@dundargoc dundargoc marked this pull request as draft May 18, 2024 09:30
@dundargoc

This comment has been minimized.

@icholy

This comment has been minimized.

@dundargoc
Copy link
Member Author

dundargoc commented May 18, 2024

I've created module vim.url and added the function _get_url(). Underscore to mark it private. I don't particularly care where this is added, I just chose something. If someone wants this somewhere else that's fine by me.

@dundargoc dundargoc marked this pull request as ready for review May 18, 2024 13:13
@dundargoc
Copy link
Member Author

dundargoc commented May 21, 2024

Future improvements:

  • Allow links without "https", e.g. using gx on "github.com" doesn't work.
  • (Optional) upstream https://github.com/icholy/lsplinks.nvim or a variation of it. Unsure if it's useful enough to upstream in core though, seemed kinda niche.

@icholy
Copy link
Contributor

icholy commented May 21, 2024

@dundargoc FYI vscode is using LSP document links to implement this functionality for markdown files https://github.com/microsoft/vscode/tree/main/extensions/markdown-language-features/server

runtime/lua/vim/url.lua Outdated Show resolved Hide resolved
In other words, `gx` works regardless of where it was used in
`[...](https://...)`. This only works on markdown buffers.

Co-authored-by: ribru17 <ribru17@gmail.com>
@dundargoc dundargoc merged commit f864b68 into neovim:master May 24, 2024
29 checks passed
@dundargoc dundargoc deleted the feat/gx branch May 24, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants