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

embark-act not presenting command/kb options #176

Closed
bdarcus opened this issue Mar 1, 2021 · 36 comments
Closed

embark-act not presenting command/kb options #176

bdarcus opened this issue Mar 1, 2021 · 36 comments

Comments

@bdarcus
Copy link

bdarcus commented Mar 1, 2021

I am working on this PR, that I am 99% sure should work, but the commands aren't available from embark-act. I don't even know where to begin to figure out why.

I do know the embark-keymap-alist variable is correctly set; here's what's reported by describe-variable.

Value:
((bibtex . bibtex-completion-map)

I'm using Emacs 28.0.50.

Any tips?

@bdarcus bdarcus changed the title embark-act not presenting command/kb options embark-act not presenting command/kb options Mar 1, 2021
@oantolin
Copy link
Owner

oantolin commented Mar 1, 2021

I'll look into it, but right now I am not able to run the code. Here are a few problems I found:

  • The variable bibtex-completion-read-backend is nil after I evaluate your code. I think this happens because it is set to the function value of bibtex-completion--completing-read before that function is defined.
  • Although bibtex-completion-read-backend is defined with set making it a variable it is called in bibtex-completion--read using the expression (bibtex-completion-read-backend) which only works on functions. In Emacs Lisp to call a function held in a variable you need funcall, (funcall bibtex-completion-read-backend).

Are you able to run your code? If so, can you give me some instructions on how to do that? Here's what I tried:

  • I cloned your repo.
  • Installed the packages in your list of requirements: parsebib, biblio, s, f, etc.
  • Evaluated (setq bibtex-completion-bibliography '("~/a/bib/file/I/had.bib")).
  • Tried M-x bibtex-completion-insert-key.

In the meantime I can try the obvious fixes for the problems I found, but would appreciate some help in getting the code to run so I can test.

@oantolin
Copy link
Owner

oantolin commented Mar 1, 2021

Oops, sorry for the noise, I realized you just have a tiny typo:

(set 'bibtex-completion-read-backend
      (symbol-function 'bibtex-completion--completing-read))

That should use fset in place of set. With that change I can run the code.

@bdarcus
Copy link
Author

bdarcus commented Mar 1, 2021 via email

@oantolin
Copy link
Owner

oantolin commented Mar 1, 2021

One piece of advice that I have independent of Embark is to not reload the bibtex file after every single keystroke!

In the bibtex-completion--completing-read function you should call (bibtex-completion--get-candidates) *once`, storing the result in a variable and use that variable inside your completion table function.

@bdarcus
Copy link
Author

bdarcus commented Mar 1, 2021 via email

@oantolin
Copy link
Owner

oantolin commented Mar 1, 2021

I can run it now, no worries.

@oantolin
Copy link
Owner

oantolin commented Mar 1, 2021

Ah, you have the syntax for defvar backwards! When you define bibtex-completion-map, you are actually assigning the docstring to the variable. The keymap should come first, and the docstring second. (In case you aren't using it, I highly recommend turning on eldoc-mode, it shows you the argument list for the current function call in the echo area, that way you don't have to remember the argument order).

@bdarcus
Copy link
Author

bdarcus commented Mar 1, 2021 via email

@oantolin
Copy link
Owner

oantolin commented Mar 1, 2021

After fixing the keymap definition I do get offered the actions in Embark! They don't quite seem to work since they receive the entire formatted reference string shown in the minibuffer and seem to expect receiving only the key. (I don't have PDFs for my references, so I only tried the insert-key, insert-bibtex and show-entry actions.)

@bdarcus
Copy link
Author

bdarcus commented Mar 1, 2021

They don't quite seem to work since they receive the entire formatted reference string shown in the minibuffer and seem to expect receiving only the key.

Weird indeed.

Particularly because earlier this worked fine.

@bdarcus
Copy link
Author

bdarcus commented Mar 1, 2021

I made the fixes you noted (except, for now, the suggestion to store the candidates in a variable; will figure that out later).

Still doesn't work for me.

Screenshot from 2021-03-01 14-57-34

And what do you make of this?

They don't quite seem to work since they receive the entire formatted reference string shown in the minibuffer ...

@bdarcus
Copy link
Author

bdarcus commented Mar 1, 2021 via email

@oantolin
Copy link
Owner

oantolin commented Mar 1, 2021

I believe your current version is still defining bibtex-completion-map to be the string "Keymap for bibtex-completion commands". In the defvar form you need to put the docstring at the end. As I said before, after making that correction, Embark does offer me the actions. As things are it cannot because bibtex-completion-map is a string, not a keymap!

@oantolin
Copy link
Owner

oantolin commented Mar 1, 2021

With this version of the function everything seem to work here:

(defun bibtex-completion--completing-read ()
  "Read bibtex-completion entries for completion using completing-read."
  (bibtex-completion-init)
  (when-let ((candidates (bibtex-completion--get-candidates))
             (chosen
              (completing-read
               "BibTeX entries: "
               (lambda (string predicate action)
                 (if (eq action 'metadata)
                     '(metadata
                       ;; (annotation-function . bibtex-completion--annotation)
                       (category . bibtex))
                   (complete-with-action action candidates string predicate))))))
    (cdr (assoc chosen candidates))))

As I said above the action commands were not getting called with the bibtex key, but rather with the string containing all the info that is used for matching. What happens is that completing-read when called with an alist, uses the keys as completion candidates and returns a key. If you want the associated value in the alist (in this case the bibtex key) you need to look it up yourself.

Hope this helps!

@bdarcus
Copy link
Author

bdarcus commented Mar 1, 2021 via email

@oantolin
Copy link
Owner

oantolin commented Mar 1, 2021

It is definitely possible to warn if a variable listed in embark-keymap-alist doesn't actually hold a keymap, but it seems like such an unusual case I'm not sure it would be worth it.

@bdarcus
Copy link
Author

bdarcus commented Mar 1, 2021

OK :-)

Just to confirm everything now works as expected. Thanks again!

@bdarcus bdarcus closed this as completed Mar 1, 2021
@bdarcus
Copy link
Author

bdarcus commented Mar 9, 2021

If you have a few moments at some point, @oantolin, conceptually, how should I adapt your modified function (pasted below for convenience) for completing-read-multiple, so instead of passing a key to the action, I'm passing a list of keys?

(defun bibtex-completion--completing-read ()
  "Read bibtex-completion entries for completion using completing-read."
  (bibtex-completion-init)
  (when-let ((candidates (bibtex-completion--get-candidates))
             (chosen
              (completing-read
               "BibTeX entries: "
               (lambda (string predicate action)
                 (if (eq action 'metadata)
                     '(metadata
                       ;; (annotation-function . bibtex-completion--annotation)
                       (category . bibtex))
                   (complete-with-action action candidates string predicate))))))
    (cdr (assoc chosen candidates))))

@oantolin
Copy link
Owner

oantolin commented Mar 9, 2021

Embark doesn't have support for passing lists to actions. The actions receive a single string, the top completion of the minibuffer contents. If you are using a completing-read-multiple UI where the minibuffer has a comma-separated list of candidates (such as the default completion UI, Icomplete, or current Selectrum before any changes in radian-software/selectrum#489 that might affect the minibuffer contents), that entire string with commas and candidates will be passed to the actions.

It might be worth changing the crm-separator, if the bibtex candidates can contain commas.

EDIT: I said the actions recieve a single string with candidates and commas. The situation is a little better than that. That string is what is injected at the minibuffer prompt of the action, but if the action reads its arguments with completing-read-multiple, then completing-read-multiple will reparse the comma-separated string into a list of candidates!

@oantolin
Copy link
Owner

oantolin commented Mar 9, 2021

As for that function that you pasted, I think the only change it needs is to replace completing-read with completing-read-multiple.

@bdarcus
Copy link
Author

bdarcus commented Mar 9, 2021

Thanks for the quick response.

First, I'm glad I was correct that the function should work with modifying anything else.

On this:

EDIT: I said the actions recieve a single string with candidates and commas. The situation is a little better than that. That string is what is injected at the minibuffer prompt of the action, but if the action reads its arguments with completing-read-multiple, then completing-read-multiple will reparse the comma-separated string into a list of candidates!

So what does this mean practically for me?

The backend bibtex-completion functions do take a list of key strings:

ELISP> (bibtex-completion-insert-key (list "zukin_new_2009" "couper_2014"))
nil
ELISP> (bibtex-completion-insert-key "zukin_new_2009,couper_2014")
*** Eval error ***  Wrong type argument: sequencep, 122

In the earlier version of the code, it was just always a list of one, I guess.

@oantolin
Copy link
Owner

oantolin commented Mar 9, 2021

So I think that as long as the completion UI you are using was the property that the minibuffer contents represents the entire multiple selection that the only changes required are to the function bibtex-completion--completing-read. I earlier missed that, besides changing completing-read to completing-read-multiple, the end of the function should change too!

(defun bibtex-completion--completing-read ()
  "Read bibtex-completion entries for completion using completing-read."
  (bibtex-completion-init)
  (when-let ((candidates (bibtex-completion--get-candidates))
             (chosen
              (completing-read-multiple
               "BibTeX entries: "
               (lambda (string predicate action)
                 (if (eq action 'metadata)
                     '(metadata
                       ;; (annotation-function . bibtex-completion--annotation)
                       (category . bibtex))
                   (complete-with-action action candidates string predicate))))))
    (mapcar (lambda (choice) (cdr (assoc choice candidates))) chosen)))

Then all these commands that recieve an argument that is a list of bibtex keys interactively using (interactive (list (bibtex-completion--read))) should be able to use each other as Embark actions.

If you want further detail, here's how it will work under the hood: if you run one of those commands and select various bibliography items, the minibuffer will contain a crm-separator-separated list of formatted bibtex entries, call this the "selection string", say. When you call embark-act with one of your actions, Embark will simulate you running the command, typing in the seletion string and pressing RET. Since the action you ran also uses (interactive (list (bibtex-completion--read))) to read its argument, the completing-read-multiple call in the above code will parse the selection string into its crm-separator-separated components, and then the final mapcar, will look each one up; and the action will recieve the correct list of bibtex keys as a single argument.

It might be nicer if these selection list weren't serialized to crm-separator-separated and then reparsed, but at least it all happens automatically.

@bdarcus
Copy link
Author

bdarcus commented Mar 9, 2021

So I think that as long as the completion UI you are using was the property that the minibuffer contents represents the entire multiple selection ...

It seems selectrum has that characteristic now, and is likely to retain that. Right?

I earlier missed that, besides changing completing-read to completing-read-multiple, the end of the function should change too!

Hah; I was wondering about that!

(defun bibtex-completion--completing-read ()
  "Read bibtex-completion entries for completion using completing-read."
  (bibtex-completion-init)
  (when-let ((candidates (bibtex-completion--get-candidates))
             (chosen
              (completing-read-multiple
               "BibTeX entries: "
               (lambda (string predicate action)
                 (if (eq action 'metadata)
                     '(metadata
                       ;; (annotation-function . bibtex-completion--annotation)
                       (category . bibtex))
                   (complete-with-action action candidates string predicate))))))
    (mapcar (lambda (choice) (cdr (assoc choice candidates))) chosen)))

Then all these commands that recieve an argument that is a list of bibtex keys interactively using (interactive (list (bibtex-completion--read))) should be able to use each other as Embark actions.

If you want further detail, here's how it will work under the hood: if you run one of those commands and select various bibliography items, the minibuffer will contain a crm-separator-separated list of formatted bibtex entries, call this the "selection string", say. When you call embark-act with one of your actions, Embark will simulate you running the command, typing in the seletion string and pressing RET. Since the action you ran also uses (interactive (list (bibtex-completion--read))) to read its argument, the completing-read-multiple call in the above code will parse the selection string into its crm-separator-separated components, and then the final mapcar, will look each one up; and the action will recieve the correct list of bibtex keys as a single argument.

OK, this is super helpful.

I'll see if I can get this working now; first just the basic commands, and then with embark.

I actually moved what I was working on to a separate repo because easier for me to manage (branching, issue tracker, etc.).

https://github.com/bdarcus/bibtex-actions

It might be nicer if these selection list weren't serialized to crm-separator-separated and then reparsed, but at least it all happens automatically.

This process you describe here is what embark is doing; correct?

@oantolin
Copy link
Owner

It seems selectrum has that characteristic now, and is likely to retain that. Right?

It does have that characteristic now. I didn't follow the full discussion, so I'm not completely sure about future plans. As far I did read, I think the plan was to keep the minibuffer contents the same.

I earlier missed that, besides changing completing-read to completing-read-multiple, the end of the function should change too!

Hah; I was wondering about that!

Yeah, sorry. I'm glad you caught it.

I'll see if I can get this working now; first just the basic commands, and then with embark.

If I understand the structure of these functions correctly, they will automatically work with Embark.

This process you describe here is what embark is doing; correct?

Correct.

@bdarcus
Copy link
Author

bdarcus commented Mar 10, 2021

OK, great.

I integrated the code here ...

emacs-citar/citar#18

... and I'm getting closer to it working.

But doesn't appear to find the key, as the output is comma-separated empty spaces.

I've been grading all day though, so brain is fried. I'll look again in the morning.

Thanks again Omar; really appreciate your patience.

bdarcus added a commit to emacs-citar/citar that referenced this issue Mar 11, 2021
The bibtex-completion functions all take multiple keys as input, and
some of them (notably the "input" ones) lose functionality without
support for this.

Hence, this changes the core read function to use
'completing-read-multiple'.

In order to do that, it also sets the 'crm-separator' to ampersand, 
since default comma won't work for these data.

Many thanks to @oantolin for help on the read function.

Addresses #17.

cc oantolin/embark#176 (comment)
@bdarcus
Copy link
Author

bdarcus commented Apr 9, 2021

One piece of advice that I have independent of Embark is to not reload the bibtex file after every single keystroke!

In the bibtex-completion--completing-read function you should call (bibtex-completion--get-candidates) *once`, storing the result in a variable and use that variable inside your completion table function.

Not urgent at all @oantolin, but can I ask a followup on this?

Your suggestion was this:

(defun bibtex-completion--completing-read ()
  "Read bibtex-completion entries for completion using completing-read."
  (bibtex-completion-init)
  (when-let ((candidates (bibtex-completion--get-candidates))

So the question is simple: how long is the candidates variable results stored?

Would it be refreshed every time the bibtex-completion--completing-read function is run?

Trying to sort out how to get file-notify to update the candidates when certain files or directories change.

@oantolin
Copy link
Owner

oantolin commented Apr 9, 2021

Would it be refreshed every time the bibtex-completion--completing-read function is run?

Yes. I would leave it at that and not bother with file-notify: if someone changes the files while the minibuffer is already open the changes won't be noticed until the next bibtex-actions command, and I think that's totally fine.

@bdarcus
Copy link
Author

bdarcus commented Apr 9, 2021 via email

@bdarcus
Copy link
Author

bdarcus commented Apr 10, 2021 via email

@oantolin
Copy link
Owner

I think I'd try caching with a simple manual refresh first to see if that's comfortable. A user can just go about their bibtex business and when they try to insert a reference that they are sure they have but bibtex-actions can't find, they refresh the cache. People have different habits, but I think that for me that would be sufficient. I tend to only add references to my bibtex files in batches, I'd just have to refresh the cache after every batch.

@bdarcus
Copy link
Author

bdarcus commented Apr 10, 2021

I think I'd try caching with a simple manual refresh first to see if that's comfortable.

Makes sense.

I actually learned today in subsequent back-and-forth that I already am indirectly caching, because bibtex-completion already has that functionality.

emacs-citar/citar#70

But there appears to be a slight delay when I call my commands, probably because the candidate function (which ultimately does call the cached data) is a bit slow.

I tend to only add references to my bibtex files in batches, I'd just have to refresh the cache after every batch.

Me too.

The use case I was thinking of is note-taking on sources, say with org-roam-bibtex. I sometimes go back-and-forth between the notes and the manuscript.

@bdarcus
Copy link
Author

bdarcus commented Apr 12, 2021

I think I'd try caching with a simple manual refresh first to see if that's comfortable.

Done: emacs-citar/citar@edb568a

Initially, I just added a section to the README with how to manually setup file-notify for this.

I may also add a couple of convenience commands that the user can also run manually.

Thanks!

@oantolin
Copy link
Owner

oantolin commented Apr 12, 2021

Very cool! One small suggestion I'd make is to have bibtex-actions-refresh take an optional argument which it ignores, so that you don't need to wrap it in a lambda for file-notify-add-watch.

@bdarcus
Copy link
Author

bdarcus commented Apr 12, 2021

One small suggestion I'd make is to have bibtex-actions-refresh take an optional argument which it ignores, so that you don't need to wrap it in a lambda for file-notify-add-watch.

If you have an additional moment, can you explain that? Why does that optional argument obviate the lambda.

FWIW, as I was working on this, I was wondering why the lambda!

@oantolin
Copy link
Owner

The file-notify-add-watch expects a callback function that can receive one parameter and calls it with the event. The lambda you wrote is such a function: it can receive one parameter (and just ignores it). You cannot use bibtex-actions-refresh directly as the callback because you'll get an error message saying "wrong number of arguments" since it takes 0 and file-notify-add-watch will call it with 1. But if you change the definition of bibtex-actions-refresh by adding an optional argument that it ignores, it can be used both ways: as an Emacs command which will call it without the optional argument, and as a callback for file-notify-add-watch which will provide the event as the optional argument.

@bdarcus
Copy link
Author

bdarcus commented Apr 12, 2021

Thank you; now I understand!

Here's the commit: emacs-citar/citar@d31bfa6.

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