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

Plugin development #1

Closed
4 tasks done
abeldekat opened this issue Dec 31, 2024 · 14 comments
Closed
4 tasks done

Plugin development #1

abeldekat opened this issue Dec 31, 2024 · 14 comments
Assignees

Comments

@abeldekat
Copy link
Owner

abeldekat commented Dec 31, 2024

Test setup:

  • clone LazyVim
  • copy the contents of file mini_snippets_with_sources in this PR to file lua/plugins/extras/coding/mini_snippets_with_sources.lua in your local config
  • open LazyVim
  • open extras
  • select nvim-cmp and mini_snippets_with_sources(a "user" extra)
  • restart LazyVim

Checklist

  • Investigate the most common sources, especially cmp-luasnip
  • cmp-luasnip has caching, including documents. Not needed for mini.snippets?
  • Test with LazyVim extra "mini_snippets_with_sources"
  • Add the source to the wiki of nvim-cmp
@abeldekat abeldekat pinned this issue Dec 31, 2024
@abeldekat abeldekat self-assigned this Dec 31, 2024
@abeldekat
Copy link
Owner Author

abeldekat commented Jan 1, 2025

Hello @echasnovski,

The plugin is working.

Would it be possible to have MiniSnippets.expand() accept a snippet parameter? In the execute method of the source, I already have the snippet, selected by nvim-cmp.

The documentation popup is markdown with a filetype indicator. I am unsure how the highlighting will respond for global, non-filetype snippets.

In cmp-luasnip, a cache is maintained by ft, both for snippets and docs. That might very well be done for performance reasons. Otherwise, perhaps because LuaSnips supports snippets to be dynamically added/changed?

On each completion, all snippets do need to be transformed into results for nvim-cmp.... I am still unsure if caching should be added.

@echasnovski
Copy link

  • cmp-luasnip has caching, including documents. Not needed for mini.snippets?

Yes, I'd say no extra caching should be needed, but there are nuances. 'mini.snippets' already does proper amount of caching with built-in loaders plus there is an example for even more caching. The only nuance is that it is designed for manual expand which is relatively rare, but 'nvim-cmp' source will be computed way more frequently (gut feeling is at least 10x more).

The single prepare step (after the initial reading data from disk) in my Lua setup with about 100 snippets to resolve takes:

  • 0.75 ms currently with prepare step resolving snippets on every expand.
  • 0.125 ms with caching prepare step by context.buf_id .. context.lang.

Another issue here might be to correctly decide what to use as caching identifier. 'mini.snippets' implements the idea of "context" which (in theory) can be any table.

@echasnovski
Copy link

echasnovski commented Jan 1, 2025

Would it be possible to have MiniSnippets.expand() accept a snippet parameter? In the execute method of the source, I already have the snippet, selected by nvim-cmp.

Then there is no need to expand(), only insert (which acts at cursor, just to be clear).

On each completion, all snippets do need to be transformed into results for nvim-cmp.... I am still unsure if caching should be added.

The only way to make an educated decision here is by profiling, I am afraid.

@abeldekat
Copy link
Owner Author

Thanks!

Then there is no need to expand(), only insert (which acts at cursor, just to be clear)

Of course. I intend to read the help again...). I thought there wouldn't be a session, my mistake.

The only way to make an educated decision here is by profiling, I am afraid.

Indeed. I have never done that for Neovim. Do you know of a good resource explaining the steps?
I created a separate issue #2

The documentation popup is markdown with a filetype indicator. I am unsure how the highlighting will respond for global, non-filetype snippets.

Do you have an opinion regarding my question concerning the popup window?

@echasnovski
Copy link

Indeed. I have never done that for Neovim. Do you know of a good resource explaining the steps?
I created a separate issue #2

Answered in #2.

The documentation popup is markdown with a filetype indicator. I am unsure how the highlighting will respond for global, non-filetype snippets.

Do you have an opinion regarding my question concerning the popup window?

This line might indeed be time consuming so might be worth caching. Not sure about the best cache id, though.

One extra problem here is that vim.bo.filetype isn't always the same as markdown language used for code blocks. LaTeX can be one example, but there are more. In 0.11 there is vim.treesitter.language.get_lang(), but its "good" behavior is fairly recent (0.10 is slightly different, 0.8 doesn't have it).

Other than that, using fenced code block after convert_input_to_markdown_lines probably is ok.

@abeldekat
Copy link
Owner Author

Then there is no need to expand(), only insert (which acts at cursor, just to be clear).

That cursor is a problem unfortunately.
Expanding fore yields foreachfor i, x in pairs(table) do

Perhaps I can keep using MiniSnippets.expand()? The data is cached, and it inserts correctly.

@echasnovski
Copy link

echasnovski commented Jan 1, 2025

That cursor is a problem unfortunately.
Expanding fore yields foreachfor i, x in pairs(table) do

This doesn't look like the reason is inserting at cursor. The reason is that insert is meant to only insert snippet at cursor, not remove the text region used to match it. This is assumed to be a completion plugin or expand() job.

The default_insert() however can remove the matched text region if there is appropriate data in snippet table. See here and here.

See also this part about general config.insert.

Perhaps I can keep using MiniSnippets.expand()? The data is cached, and it inserts correctly.

This might result in showing vim.ui.select as it will make the whole prepare-match-select-insert dance.

@abeldekat
Copy link
Owner Author

Yes, I will try to pass the region based on nvim-cmp.

@echasnovski
Copy link

Yes, I will try to pass the region based on nvim-cmp.

I'd suggest removing it manually before calling insert. This way it will allow custom user overrides to also work.
Removing a text region is basically a vim.api.nvim_buf_set_text(0, from_line, from_col, to_line, to_col, {}). Also place the cursor afterwards at from_line, from_col; there could be problems if mode is not Insert, but I think it is safe to assume that it will always be Insert in completion source.

@abeldekat
Copy link
Owner Author

I added a new commit. The code is much shorter than I thought it would be.

Do you think the plugin is ready to be announced?

I plan to first work on finishing the LazyVim PR. Then, I will do the profiling for the cache.

@echasnovski
Copy link

echasnovski commented Jan 1, 2025

If it works, then looks good :)
By "announce" do you mean the Reddit post? Sure, if you feel this is ready.

I'll wait a bit for all the issues to iron out (week, two?), then plan to add this to 'mini.snippets' README.

@abeldekat
Copy link
Owner Author

abeldekat commented Jan 1, 2025

I'll wait a bit for all the issues to iron out (week, two?), then plan to add this to 'mini.snippets' README.

That's great!
I wanted to add the plugin to the the list of nvim-cmp sources.
However, yesterday someone else registered a source under the same name: https://github.com/xzbdmw/cmp-mini-snippets.
That's a somewhat unpleasant surprise...)

@echasnovski
Copy link

That's great!
I wanted to add the plugin to the the list of nvim-cmp sources.
However, yesterday someone else registered a source under the same name: https://github.com/xzbdmw/cmp-mini-snippets.

As far as I can tell, your plugin has earlier first commit with at least some version of the actual code (by about 4 hours, curiously). Plus, you're the long time 'mini.nvim' user.

So if that's up to me, I'll use this one if/when I need to recommend an 'nvim-cmp' source for 'mini.snippets'. I trust you can iron out any possible issues (but I don't expect many). Plus LazyVim PR should also get you covered.

That's a somewhat unpleasant surprise...)

Not a total surprise, though, as there was public info about it. This is all open source, people can work on the same things at the same time.

@abeldekat
Copy link
Owner Author

Not a total surprise, though, as echasnovski/mini.nvim#1428 (comment). This is all open source, people can work on the same things at the same time.

Yes you're right. Thanks for the support and for answering my questions!

@abeldekat abeldekat unpinned this issue Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants