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(indicators): add cited #763

Merged
merged 2 commits into from
Mar 24, 2023
Merged

feat(indicators): add cited #763

merged 2 commits into from
Mar 24, 2023

Conversation

bdarcus
Copy link
Contributor

@bdarcus bdarcus commented Mar 24, 2023

@andersjohansson - can you give this a try?

It's just a few lines (no cache or hash table), but works for me, and simple and really clear IMHO, so easy to maintain, and provides clear demonstration value for people wanting to write new indicators.

(defun citar-is-cited ()
  "Return function to check if reference is cited in buffer."
  (let ((iscited
         (citar--major-mode-function 'list-keys #'ignore)))
    (lambda (citekey)
      (member citekey iscited))))

image

With icons:

image

My test file was small though.

If this is good to go, I can merge it.

We can then later think of other enhancements as and if needed (like possibly extending 'list-keys beyond the immediate buffer, or optimizing performance).

PS - 'list-keys was originally added to allow exporting of local bib files.

Also, I gave you co-author credit on the commit :-)

@bdarcus bdarcus added the enhancement New feature or request label Mar 24, 2023
Co-authored-by: Anders Johansson <[email protected]>
Signed-off-by: Bruce D'Arcus <[email protected]>
@bdarcus
Copy link
Contributor Author

bdarcus commented Mar 24, 2023

Update: I think I'm going to merge this, as it's the necessary first step regardless of whether it needs further optimization.

My only question was whether to set it by default.

I have, mostly for discovery, and because it's easy for people to now choose what they want.

Just trying to get the CI to pass; seems there's currently some internet-related problems not related to this code.

elpa.gnu.org/443 Temporary failure in name resolution

`citar--make-indicator-symbols` is the key function for indicator
formatting and alignment; this adds an explanation in the code for
future reference.

See: #764
@bdarcus bdarcus merged commit cd86ac2 into main Mar 24, 2023
@bdarcus bdarcus deleted the cited-ind branch March 24, 2023 15:52
@bdarcus
Copy link
Contributor Author

bdarcus commented Mar 24, 2023

Merged!

Let me know how you find it @andersjohansson.

I think the biggest performance constraint will be the number of citations. If it's a huge book, with hundreds, maybe there's a noticable delay?

But the look up is happening via the cached hash table, and that is already really fast.

So I expect performance to be snappy for most documents.

@andersjohansson
Copy link
Contributor

This works well! And the buffer parsing is only run once per selection session just as it should be. My other attempts was based on a misunderstanding that this would not be the case (I didn't realize that this would be taken care of only once and stored in the compiled function when running citar--indicator-processors).

And totally reasonable to just check against a list, I suppose people have few documents with several hundred (thousands?) different citations where member versus gethash would make any reasonable difference.

I tried in a small document and in one with 40 000 words and 330 references. In the large document there is a bit of a more noticeable delay (half a second) when launching selection (mainly, not unsurprisingly org-element-parse-buffer).

But this is totally ok for me! Trying to introduce good caching would be quite complicated. Too bad the org-element--cache can’t be re-used with org-element-cache-map (as I understand it, it doesn’t store objects, like citations).

@bdarcus
Copy link
Contributor Author

bdarcus commented Mar 25, 2023

I tried in a small document and in one with 40 000 words and 330 references. In the large document there is a bit of a more noticeable delay (half a second)... But this is totally ok for me!

But not ideal.

Ooh, you just reminded of the long #397 thread.

What if we used org-element-cache-map instead? It's new, but wouldn't that be perfect here?

@andersjohansson
Copy link
Contributor

andersjohansson commented Mar 26, 2023 via email

@bdarcus
Copy link
Contributor Author

bdarcus commented Mar 26, 2023

Yes, I missed what you were saying on that earlier!

We can always add it when that functionality exists.

I did look at reftex yesterday to consider ideas for org. I don't remember details, but it's pretty low-level code that uses re-search-forward, that I assume is pretty performant.

But not the code I want to include when we can wait on Ihor for something easier to code and maintain. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants