Skip to content

bug: doesn't render telescope previews of markdown files? #98

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

Closed
1 task done
mrjones2014 opened this issue Jul 26, 2024 · 16 comments
Closed
1 task done

bug: doesn't render telescope previews of markdown files? #98

mrjones2014 opened this issue Jul 26, 2024 · 16 comments
Labels
not a bug Not a bug in this plugin

Comments

@mrjones2014
Copy link

Neovim version (nvim -v)

0.10.0

Operating system

MacOS

Terminal emulator / GUI

Wezterm

Describe the bug

I would like it to render markdown in telescope previews, currently it doesn't, even if I use a telescope callback to call require('render-markdown').enable() on the preview buffer

Expected behavior

It renders markdown in telescope previews

Healthcheck output

render-markdown: require("render-markdown.health").check()

markdown.nvim [neovim version] ~
- OK Version >= 0.10

markdown.nvim [configuration] ~
- OK valid

markdown.nvim [nvim-treesitter] ~
- OK installed
- OK markdown: parser installed
- OK markdown: highlight enabled
- OK markdown_inline: parser installed
- OK markdown_inline: highlight enabled
- WARNING latex: parser not installed
  - ADVICE:
    - Disable LaTeX support to avoid this warning by setting { latex = { enabled = false } }

markdown.nvim [executables] ~
- WARNING latex2text: not installed
  - ADVICE:
    - Disable LaTeX support to avoid this warning by setting { latex = { enabled = false } }

markdown.nvim [conflicts] ~
- OK headlines: not installed
- OK obsidian: not installed

Plugin configuration

return {
  {
    'MeanderingProgrammer/markdown.nvim',
    main = 'render-markdown',
    dependencies = { 'nvim-treesitter/nvim-treesitter', 'nvim-tree/nvim-web-devicons' },
    opts = {},
    ft = 'markdown',
  },
}

Confirmations

  • I have provided markdown text for any screenshots used in my description & understand that my issue will be closed if I have not

Additional information

No response

@mrjones2014 mrjones2014 added the bug Something isn't working label Jul 26, 2024
@MeanderingProgrammer
Copy link
Owner

This is a limitation of how Telescope previews buffers. More details on that here: https://github.com/nvim-telescope/telescope.nvim?tab=readme-ov-file#previewers.

The relevant bit is here:

We don't do bufload and instead read the file asynchronously with vim.loop.fs_ and attach only a highlighter; otherwise the speed of the previewer would slow down considerably.

As a result of this the filetype is never set. In fact using lazy with ft = 'markdown' means the plugin won't be loaded.

Will add this as a known limitation.

@MeanderingProgrammer MeanderingProgrammer closed this as not planned Won't fix, can't repro, duplicate, stale Jul 26, 2024
@ibhagwan
Copy link

ibhagwan commented Dec 3, 2024

Hi @MeanderingProgrammer,

Would it be possible to manually attach to a buffer? I tried calling the below on an fzf-lua preview window but it doesn't seem to do anything, are there any other conditions that need to be met for manual attachment to work?

  require("render-markdown.manager").attach(bufnr)

@MeanderingProgrammer
Copy link
Owner

Hmm, that would be one way to do it. The attach function checks several conditions on the buffer in should_attach: https://github.com/MeanderingProgrammer/render-markdown.nvim/blob/main/lua/render-markdown/manager.lua#L54.

However I don't think you'll need to manually call the attach function. Most likely the problem is the filetype not matching.
As long as the fzf-lua preview has a filetype you can update the plugin config to listen to those buffers as well:

require('render-markdown').setup({
    file_types = { 'markdown', 'FzfPreview' },
})

If you do this it should also attach automatically since this plugin listens to the FileType event and calls the attach function. I assume what was happening before is the same filetype condition was not matching.

There can be some issues with preview windows that programmatically re-use the same buffer / window, since those don't trigger autocommands in the same way as user interactions. So there might be some gaps there. More than happy to help get this working!

@ibhagwan
Copy link

ibhagwan commented Dec 3, 2024

However I don't think you'll need to manually call the attach function. Most likely the problem is the filetype not matching.

Fzf-lua preview works in the same manner as Telescope and thus doesn't trigger loading events, basically same issues as #98.

There can be some issues with preview windows that programmatically re-use the same buffer / window, since those don't trigger autocommands in the same way as user interactions. So there might be some gaps there. More than happy to help get this working!

Fzf-lua will create a new preview buffer for different files, it will then be cached and reattached to the preview window as needed.

Hmm, that would be one way to do it. The attach function checks several conditions on the buffer in should_attach

I'll take a look at should_attach when I get a chance to see what prevents render-markdown from attaching.

More than happy to help get this working!

Ty very much :-)

@MeanderingProgrammer
Copy link
Owner

MeanderingProgrammer commented Dec 3, 2024

Fzf-lua preview works in the same manner as Telescope and thus doesn't trigger loading events, basically same issues as #98.

Ah, I see. I found the issue with manually calling attach. The filetype is the empty string so updating the config to:

require('render-markdown').setup({
    file_types = { 'markdown', '' },
})

Gets us passed the attach stage.

Unfortunately beyond that the backbone of this plugin relies on autocommands on attached buffers to trigger updates. From the manager attach function:

local events = { 'BufWinEnter', 'BufLeave', 'CmdlineChanged', 'CursorHold', 'CursorMoved', 'WinScrolled' }
local change_events = { 'DiffUpdated', 'ModeChanged', 'TextChanged' }
if config:render('i') then
    vim.list_extend(events, { 'CursorHoldI', 'CursorMovedI' })
    vim.list_extend(change_events, { 'TextChangedI' })
end
vim.api.nvim_create_autocmd(vim.list_extend(events, change_events), {
    group = M.group,
    buffer = buf,
    callback = function(args)
        if not state.enabled then
            return
        end
        local win, windows = util.current('win'), util.windows(buf)
        win = vim.tbl_contains(windows, win) and win or windows[1]
        local event = args.event
        ui.update(buf, win, event, vim.tbl_contains(change_events, event))
    end,
})

The call to ui.update in the callback is what does all of the extmark calculations and unfortunately that only runs based on the events. I'm willing to change the mechanism to trigger updates but do you have any suggestions on what I could use instead? How would this plugin be aware when a preview window is re-opened / scrolled?

Alternatively you can bypass the lifecycle management part of this plugin and call the ui directly to force it to render extmarks on the preview window whenever a change event occurs:

require("render-markdown.core.ui").update(preview_buf, preview_win, "Fzf", true)

If I know this is being used externally I'll avoid any breaking changes on this function.

@ibhagwan
Copy link

ibhagwan commented Dec 3, 2024

Unfortunately beyond that the backbone of this plugin relies on autocommands on attached buffers to trigger updates. From the manager attach function:

I see, the issue with autocmds is that they are somewhat invasive when it comes to something you'd consider a scratch preview buffer, for example, not attaching an LSP client to the buffer.

The call to ui.update in the callback is what does all of the extmark calculations and unfortunately that only runs based on the events. I'm willing to change the mechanism to trigger updates but do you have any suggestions on what I could use instead? How would this plugin be aware when a preview window is re-opened / scrolled?

These are good quesions, I'm not sure if I have a better answer than to offload the updates to the calling plugin upon scroll, etc.

require("render-markdown.core.ui").update(preview_buf, preview_win, "Fzf", true)

Tried the above, still didn't render.

I'm going to leave this for now until we can perhaps come up with a different strategy that won't be as prone to error as reintroducing autocmds or relying on the caller to update (even though there probably aren't too many locations to call render, scrolling, resizing, etc).

Btw, there's a similar issue/wish-list with displaying nvim-treesitter-context in the preview buffer.

@MeanderingProgrammer
Copy link
Owner

I see, the issue with autocmds is that they are somewhat invasive when it comes to something you'd consider a scratch preview buffer, for example, not attaching an LSP client to the buffer.

I definitely understand why you want to avoid all the default things that happen when you open a buffer for a preview buffer, especially to have any kind of decent performance. We do end up in this unfortunate state where the preview buffers kind of exist on their own and each feature you want to add needs to be a hand rolled integration.

I'm going to leave this for now until we can perhaps come up with a different strategy that won't be as prone to error as reintroducing autocmds or relying on the caller to update (even though there probably aren't too many locations to call render, scrolling, resizing, etc).

Holding off make sense. Calling the ui update should work now, I pushed a change recently to render in terminal mode by default: da70762, you can also set this in your configuration but having it on by default will make things easier if we ever come back to this and shouldn't cause any problems.

As a thought for what we can actually do that's less hacky what do you think about the concept of preview buffer extensions? It would be something similar to telescope extensions or nvim-cmp sources. They would have a few methods to implement like open(buf), scroll(buf), close(buf) which you would integrate once in fzf-lua then plugin authors would write the implementation so that we can run on preview buffers. Prevents breaking changes from needing to be fixed in fzf-lua, instead the extension would just need to be updated. 3rd party ones could also be written outside of any plugins, like one for nvim-treesitter-context. Happy to talk about it more if that sounds interesting!

@ibhagwan
Copy link

ibhagwan commented Dec 4, 2024

Holding off make sense. Calling the ui update should work now, I pushed a change recently to render in terminal mode by default: da70762, you can also set this in your configuration but having it on by default will make things easier if we ever come back to this and shouldn't cause any problems.

It works and seems quite nice, I only needed to add a call in the initial buffer populate method and the scolling:
Peek 2024-12-03 19-26

As a thought for what we can actually do that's less hacky what do you think about the concept of preview buffer extensions? It would be something similar to telescope extensions or nvim-cmp sources. They would have a few methods to implement like open(buf), scroll(buf), close(buf) which you would integrate once in fzf-lua then plugin authors would write the implementation so that we can run on preview buffers. Prevents breaking changes from needing to be fixed in fzf-lua, instead the extension would just need to be updated. 3rd party ones could also be written outside of any plugins, like one for nvim-treesitter-context. Happy to talk about it more if that sounds interesting!

This is a great idea, shouldn't be a big deal to add a few callbacks to the previewer class, I'm debating with myself in the case such as this which party is the responsible party for the integration, fzf-lua for "integration with redner-markdown" or the opposite? :-)

In any event the simple integration I just added is a mere 5 lines of code.

@ibhagwan
Copy link

ibhagwan commented Dec 4, 2024

Even works beautifully with live_grep match highlighting:
image

@ibhagwan
Copy link

ibhagwan commented Dec 4, 2024

@MeanderingProgrammer, if you wanna try it out yourself, try the latest fzf-lua commit ibhagwan/fzf-lua@e0c16c1, don't need to configure anything special, if render-nvim is loaded fzf-lua will attempt to call require("render-markdown.core.ui").update on init and scroll - lmk what you think?

@MeanderingProgrammer
Copy link
Owner

MeanderingProgrammer commented Dec 4, 2024

It works and seems quite nice, I only needed to add a call in the initial buffer populate method and the scolling:

Oh that's awesome! Super glad it works and looks great!

Does it cause a noticeable delay in opening the file, or is the performance pretty good?

This is a great idea, shouldn't be a big deal to add a few callbacks to the previewer class, I'm debating with myself in the case such as this which party is the responsible party for the integration, fzf-lua for "integration with redner-markdown" or the opposite? :-)

I think following the existing patterns from nvim-cmp and telescope would work perfectly here. So it would be up to fzf-lua to add the ability to add extensions and call registered extensions' methods when appropriate and it would be up to plugins or interested 3rd parties to write the integrations and keep up with any changes.

As a very rough sketch of what I had in mind:

Types

---@class fzf.preview.Context
---@field buf integer
---@field win integer

---@class fzf.preview.Extension
---@field name fun(self: fzf.preview.Extension): string
---@field update fun(self: fzf.preview.Extension, context: fzf.preview.Context)

render-markdown Implementation

---@class render.md.fzf.Extension: fzf.preview.Extension
local Extension = {}
Extension.__index = Extension

---@return fzf.preview.Extension
function Extension.new()
    return setmetatable({}, Extension)
end

---@return string
function Extension:name()
    return 'render-markdown'
end

---@param context fzf.preview.Context
function Extension:update(context)
    require("render-markdown.core.ui").update(context.buf, context.win, "FzfPreview", true)
end

local M = {}

function M.setup()
    local ok, fzf = pcall(require, 'fzf-lua')
    if ok then
        fzf.register(Extension.new())
    end
end

return M

Somewhere in the initialization of render-markdown I'll call setup on this module, likely gated behind a config flag in case users prefer to not run this plugin with fzf-lua. Can also make this something users do in their plugin configurations more akin to telescope's load_extension.

Changes in fzf-lua

Add register method that takes extensions, stores them in something like table<string, fzf.preview.Extension> based on extension:name().

In probably the same spots as the render-markdown integration:

local context = { buf = preview_buf, win = preview_win }
for _, extension in pairs(config.extensions) do
    extension:update(context)
end

@MeanderingProgrammer
Copy link
Owner

if render-nvim is loaded fzf-lua will attempt to call require("render-markdown.core.ui").update on init and scroll - lmk what you think?

Tried it out and works exactly how I would want, thank you!

Was a little surprised to see this vim.bo[bufnr].ft = ft, is there a reason to avoid setting for other buffers? Or since it's only needed for markdown buffers there's just no point in adding it for the others.

I'll add a note to myself that the update function is being called by fzf-lua, just to avoid renaming it or something.

@ibhagwan
Copy link

ibhagwan commented Dec 4, 2024

Was a little surprised to see this vim.bo[bufnr].ft = ft, is there a reason to avoid setting for other buffers? Or since it's only needed for markdown buffers there's just no point in adding it for the others.

This is just out of caution :-)

I learned the hard way that every meaningless change has someone somwhere doing something I didn't anticipate causing an unexpected error lol, for example ibhagwan/fzf-lua@a3f66ca#commitcomment-149465672 which ended up with a commit revert.

I'm still trying to think of a downside for setting ft for all preview buffers, one such downside would be triggering of the FileType event (which I can ignore with eventigore ofc).

@ibhagwan
Copy link

ibhagwan commented Dec 4, 2024

@MeanderingProgrammer, ty for the API suggested implementation, I’d have to think about it a bit more as not every window/previewer supports everything, for example some users use fzf_native which uses fzf’s native previewer in the shell (and bat utility as a file previewer), some use fzf-tmux which offload the entire UI to the shell/tmux so not everything is applicable, this can only work with the “builtin” neovim previewer (which is a neovim window/buffer).

I think it will also be wise on my end to first refactor the window class code and separate each window object (native, tmux, neovim float, neovim split) to their corresponding classes, then the proper API will “reveal” itself to me :)

However, I’m in no rush as the integration demand is currently fulfilled via the very simple fzf_exec API, most of which doesn’t require special triggers and if someone wants their own previewer they inherit from the builtin previewer: https://github.com/ibhagwan/fzf-lua/wiki/Advanced#preview-nvim-builtin.

Does it cause a noticeable delay in opening the file, or is the performance pretty good?

Not at all, it seems you’ve implement the rendering asynchronously which is great, first the preview buffer content is displayed, only then the rendering is triggered on its own free time.

@MeanderingProgrammer
Copy link
Owner

MeanderingProgrammer commented Dec 4, 2024

I learned the hard way that every meaningless change has someone somwhere doing something I didn't anticipate causing an unexpected error lol, for example ibhagwan/fzf-lua@a3f66ca#commitcomment-149465672 which ended up with a commit revert.

I would definitely not expect checking window is valid to break a filter function, that's a fun side effect.

I'm still trying to think of a downside for setting ft for all preview buffers, one such downside would be triggering of the FileType event (which I can ignore with eventigore ofc).

I hadn't noticed that the FileType event is being triggered, that makes sense, I assumed events were suppressed at a buffer level. As a result this plugin is attaching to the buffers, but still not enough to work on its own since it's missing the change events. I definitely get where you're coming from with trying to limit any potential impact.

I was curious why LSPs weren't being attached to the buffers as well, turns out it's because they have the nofile buftype: https://github.com/neovim/nvim-lspconfig/blob/master/lua/lspconfig/manager.lua#L183C20-L183C27.

I’d have to think about it a bit more as not every window/previewer supports everything, for example some users use fzf_native which uses fzf’s native previewer in the shell (and bat utility as a file previewer), some use fzf-tmux which offload the entire UI to the shell/tmux so not everything is applicable, this can only work with the “builtin” neovim previewer (which is a neovim window/buffer).

No problem! Definitely no rush and I get if you'd prefer to not expose something like this, was just a thought. The hard part is definitely all those edge cases with what should be called when. You'd probably want extensions to somehow specify which buffers they should run on and more of those finer details.

Not at all, it seems you’ve implement the rendering asynchronously which is great, first the preview buffer content is displayed, only then the rendering is triggered on its own free time.

I wish :( Trying to make it async using something from vim.uv proved much to difficult since all of the computations rely on accessing the vim API which can't be done across threads. There's also not much that could be done by using jobs / coroutines for much the same reason.

Instead the main rendering is wrapped in a vim.schedule so hopefully the delay comes after all the important things. It is fairly fast since it only processes what's currently visible, so even large files are quick, but it is sometimes noticeable. Problems could occur if fzf-lua used vim.schedule after calling the render function since that would now run after the render logic.

Glad it's working well! I've switched my config over from telescope to fzf-lua, really like how it looks and feels :) Will look into the more advanced configurations to see what's possible.

Thanks for the thoughtful responses I've enjoyed this chat, if there's any follow up definitely feel free to reach out!

@ibhagwan
Copy link

ibhagwan commented Dec 4, 2024

Glad it's working well! I've switched my config over from telescope to fzf-lua, really like how it looks and feels :) Will look into the more advanced configurations to see what's possible.

I’m complimented, ty @MeanderingProgrammer, likewise.

@MeanderingProgrammer MeanderingProgrammer added not a bug Not a bug in this plugin and removed bug Something isn't working labels Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not a bug Not a bug in this plugin
Projects
None yet
Development

No branches or pull requests

3 participants