micangl / cmp-vimtex

Vimtex source for nvim-cmp.
MIT License
78 stars 5 forks source link

Use VimTeXs lua parser? #3

Closed lervag closed 1 month ago

lervag commented 10 months ago

I'm curious if it may make more sense to utilize the new VimTeX lua parser for bib files directly. It has now become quite fast. And after parsing a large file it can be kept in memory.

Thus, even if there would be a delay while parsing for the completion, the delay would be 1) very small, and 2) should only appear once for each session.

I believe this would simplify the cmp-vimtex plugin. Simpler is often better. :)

micangl commented 10 months ago

Thus, even if there would be a delay while parsing for the completion, the delay would be 1) very small, and 2) should only appear once for each session.

Whilst this is true, I'd like to avoid the slightest hint of freezing. The benchmark result was about half a second which, while an excellent time for the parsed-file size, is, I feel, already too much.

The parser is not excessively big and, apart from applying your recent enhancements, I think it's safe to say that it won't have to be altered in the foreseeable future (since it's practically complete). Whilst it would be simpler to use Vimtex's parser, it's a compromise I'd like to avoid.

lervag commented 10 months ago

Thus, even if there would be a delay while parsing for the completion, the delay would be 1) very small, and 2) should only appear once for each session.

Whilst this is true, I'd like to avoid the slightest hint of freezing. The benchmark result was about half a second which, while an excellent time for the parsed-file size, is, I feel, already too much.

That's fair; cmp-vimtex is your plugin and you are perfectly free to develop it as you see fit. In that case, I would strongly suggest that you rewrite your parser similar to the updated VimTeX parser. There are two reasons:

  1. I found and fixed several bugs(!).
  2. My update has a performance improvement of about 1.5-2.
micangl commented 9 months ago

@lervag Sorry for coming back to this issue after more than a month. I still haven't gotten around to making all the changes to the parser, as I haven't found the time.

I found and fixed several bugs(!).

By chance, do you remember what problems this bugs caused? I'm not noticing much, on my part.

On a separate note, I may have found the reason why you weren't able to display correctly the bibliographic information in the menu. Check out this issue and, especially, the pull request I've linked at the bottom.

lervag commented 9 months ago

@lervag Sorry for coming back to this issue after more than a month. I still haven't gotten around to making all the changes to the parser, as I haven't found the time.

No problem!

I found and fixed several bugs(!).

By chance, do you remember what problems this bugs caused? I'm not noticing much, on my part.

No, sorry. I don't think it was any major bug, e.g. nothing that was catched by the existing test cases. And since I catched them while implementing the Lua version I forgot to create new test cases.

I still think it would not be too much work for you to update your parser based on my updates, and I think it should be both faster and slightly less buggy.

On a separate note, I may have found the reason why you weren't able to display correctly the bibliographic information in the menu. Check out this issue and, especially, the pull request I've linked at the bottom.

I don't use lspkind.nvim, but perhaps I use something else that does something similar? Not sure. Things work well enough for me right now, though.

micangl commented 9 months ago

I still think it would not be too much work for you to update your parser based on my updates, and I think it should be both faster and slightly less buggy.

Then I'll start to work on it.

I don't use lspkind.nvim, but perhaps I use something else that does something similar? Not sure. Things work well enough for me right now, though.

Good to hear!

I've implemented the bibliographic databases search functionality in the database_search branch. When a cite completion item is selected, by calling the search_menu function, a menu will appear to select a search engine.

    vim.keymap.set("i", "<C-s>", function() require('cmp_vimtex.search').search_menu() end)

If you have time and would like to try it, let me know if you have any suggestions.

As soon as I find the time, I'll also write up the tutorial.

micangl commented 9 months ago

@lervag I've also written the tutorial.

Since @ejmastnak said he would wait for a tutorial in an old thread, I'll point out that the search functionality has been implemented on the database_search branch (it will be merged into masters in the future).

@benbrastmckie might be interested too.

All the plugin functionality has been thoroughly explained in the tutorial linked above. So, you have the time to check it out, I'd be glad to accept suggestions; both on new features, and the state of the currently available ones.

benbrastmckie commented 9 months ago

Definitely interested, just swamped. But will integrate this soon. Thanks for the amazing resources!

lervag commented 8 months ago

The search feature is quite nice, thanks! The tutorial looks good as well. Well done!

I understand it is a nontrivial amount of work to update the parser code as per the original comment in this thread. However, I've noticed that your parser has a bug that I've fixed that you can find quite easily reproduced. I'm opening an issue for that now (#15) - but I think the best fix for that would be to update the parser as requested here.