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

refactor: add candidates cache #71

Merged
merged 9 commits into from
Apr 11, 2021
Merged

refactor: add candidates cache #71

merged 9 commits into from
Apr 11, 2021

Conversation

bdarcus
Copy link
Contributor

@bdarcus bdarcus commented Apr 11, 2021

Refactor so that bibtex-actions-read pulls candidates from a
cache: bibtex-actions--candidates-cache.

Add:

  • bibtex-actions--candidates-cache variable
  • bibtex-actions-refresh interactive command, to update cache

Refactor:

  • bibtex-actions--get-candidates renamed to bibtex-actions--format-candidates
  • bibtex-actions--get-candidates now grabs the candidates from the cache

fixes #69


@wenjie2wang - I do think it would also be good to add the hook to bibtex-completion, but you want to give this a try and let me know?

Turns out it was easier than I thought to add this.

But I haven't fully tested it and seen if it has any unintended consequences.

It does cache the candidates, however, so should address #69.

So if you could test it further and let me know, that'd be great.

bibtex-actions.el Outdated Show resolved Hide resolved
@wenjie2wang
Copy link
Contributor

Thanks, @bdarcus ! I tried this PR and I found that it worked great. There is no more pause after the first run due to the added candidate cache.

@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 11, 2021

Thanks, @bdarcus ! I tried this PR and I found that it worked great. There is no more pause after the first run due to the added candidate cache.

Great!

Can you review the code for the basic logic and confirm that it makes sense to you; that it shouldn't cause any problem when I merge it? It's very simple, but I think should work.

@bdarcus bdarcus changed the title add candidates cache refactor: add candidates cache Apr 11, 2021
@wenjie2wang
Copy link
Contributor

How will the cache get invalidated? I removed a few BibTeX entries and I still got the cached results.

@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 11, 2021

How will the cache get invalidated? I removed a few BibTeX entries and I still got the cached results.

The idea is one would have to run bibtex-actions--reload-candidates-cache (hmm, maybe I should make that a public function and command?), either manually, or via a filenotify process as mentioned here:

https://github.com/bdarcus/bibtex-actions/wiki/Configuration#expand-proactive-loading-of-library-to-pdfs-and-notes

But that would need to include the bib files, since while bibtex-completion already handles this, this PR does not.

I omitted it because I'm thinking it's too complicated (see this recent still open thread), and this approach gives users more flexibility.

WDYT?

Also, maybe you can test the config I list on the wiki for your bib file to confirm it works for you?

@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 11, 2021

Updated the wiki section to address here:

https://github.com/bdarcus/bibtex-actions/wiki/Configuration#proactive-reloading-of-library

I will probably add that section to the README before merging.

I wonder if there's a way to do this asychronously, so we never experience a pause when it's reloading?

@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 11, 2021

Latest commit:

  • changes that reload function to the bibtex-actions-refresh interactive command
  • adds section to the README (just need to confirm those file-notify configs are right; may not be for the bib one if there is more than one bib file?)

Change the 'bibtex-actions--reload-candidates-cache' to
'bibtex-actions-refresh' and make it an interactive command.

Also, adds section on this to the README.
@wenjie2wang
Copy link
Contributor

Latest commit:

* changes that reload function to the `bibtex-actions-refresh` interactive command

* adds section to the README (just need to confirm those `file-notify` configs are right; may not be for the bib one if there is more than one bib file?)

I followed the README and added file-notify configs. The cache got invalidated correctly.

@bdarcus bdarcus merged commit edb568a into main Apr 11, 2021
@bdarcus bdarcus deleted the performance branch April 11, 2021 18:46
@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 11, 2021

Merged; thanks much!

Let me know of course if you have other ideas, etc.

@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 11, 2021

PS - I'm glad I did this. It's nice to have the instant loading!

@bdarcus bdarcus added this to the v1.0 milestone Apr 13, 2021
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

Successfully merging this pull request may close these issues.

add caching of bib candidates
2 participants