emacs-citar / citar

Emacs package to quickly find and act on bibliographic references, and edit org, markdown, and latex academic documents.
GNU General Public License v3.0
502 stars 54 forks source link

Indicator for cited references, re-using major-mode `list-keys` function #761

Closed andersjohansson closed 1 year ago

andersjohansson commented 1 year ago

This is a simpler variant of #744.

I am not sure about whether it is smart to transform a list to a hash table in this way, compared to generating a hash-table from the beginning. A hash table should be most suitable for lookup (citar-is-cited) though.

andersjohansson commented 1 year ago

This seems to work ok at least for smaller org, markdown and latex files. Haven’t tested extensively for lager files.

bdarcus commented 1 year ago

I am not sure about whether it is smart to transform a list to a hash table in this way, compared to generating a hash-table from the beginning.

My question as well, particularly since we now rely on hash tables so pervasively.

I haven't looked; do you have a handle on how easy that change would be?

If easy and straightforward, perhaps we should just do it?

Or on the flip side, for lists of keys, maybe a list is just fine?

Particularly if package make that convenient?

Do you need a hash table here?

bdarcus commented 1 year ago

This is weird. It says I closed this, but I didn't; at least not intentionally!

Maybe because it is set to merge to a branch I deleted?

Can you switch it to main @andersjohansson?

Hmm ... since I can't change it, you probably can't either.

I'm guessing the solution is to open a new PR from the same branch, but against main.

andersjohansson commented 1 year ago

Why do you it here? My intention was all the indicators could be decoupled from this sort of code.

That would be good. But wouldn’t the other alternative be to do it in the indicator-function, which is run once for every (visible ?) key that is up for completion (if my understanding of affixation functions is correct). We only want to parse the buffer once, and then do lookups in the hash-table (is my idea). Or am I missing something?

bdarcus commented 1 year ago

Why do you it here? My intention was all the indicators could be decoupled from this sort of code. That would be good. But wouldn’t the other alternative be to do it in the indicator-function, which is run once for every (visible ?) key that is up for completion (if my understanding of affixation functions is correct).

Yes. That's how the indicators work, to ensure they're always up-to-date.

We only want to parse the buffer once, and then do lookups in the hash-table (is my idea). Or am I missing something?

The buffer changes, and every time you add or remove a citation you'd want the UI to update?

Same with bib files, notes, files, etc.

Do you find the performance problematic without the (cached) hash table?

Edit: if in the end this or other indicators needs a hash table, we could maybe store them in the indicator struct, just as we do with the bib stuff?

bdarcus commented 1 year ago

Maybe try with this as the function and see what happens?

(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))))

It's the simplest approach, without creating a cache, or even a hash table.

But I think it may work because the list shouldn't be long, and the values are citekeys, so the same as the keys in the larger hash tables.