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

[WIP] Add embark-bibtex frontend. #355

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

mtreca
Copy link

@mtreca mtreca commented Feb 1, 2021

Following issue #353, I worked on a prototype frontend for bibtex-completion leveraging the embark and marginalia packages.

What changed since last iteration:

  • Remove consult dependency.
  • Add rudimentary marginalia annotations.

What it looks like:

2021-02-01-211516_1920x1080_scrot

What is missing (a lot):

  • The front-facing functions do not support the optional args from ivy-bibtex (will add later once the prototype becomes more usable).
  • No simple mechanism to display the notes and pdf icons so far (I did not look very hard at this issue yet, though).
  • The way marginalia annotations are built is very hacky so far. It does not rely on any of the tools or variables provided by bibtex-completion (such as bibtex-completion-format-entry). From what I gather, this is because the default completion mechanism cannot rely on fancier display functions like ivy-set-display-transformer. This means that I have to parse the completion options to only fetch the entry keys, pass them to completing-read, and re-create annotations on top of that. It would be much nicer to reuse the bibtex-completion built-in tools.
  • Linked to the point above, annotation alignment is kind of tricky since I cannot rely on bibtex-completion-format-entry.

This is of course far from being PR-ready, but I would like to make this front-end a more collaborative effort, especially from people knowing both marginalia and bibtex-completion more than me.

@bdarcus
Copy link

bdarcus commented Feb 1, 2021

Seems this will be a great test for embark and marginalia!

FWIW, my biggest needs are elegant support for citation key insertion in org and markdown for document authoring, and ability to integrate with org-roam-bibtex (which takes over note-taking for bib notes for use with org-roam).

@minad
Copy link

minad commented Feb 1, 2021

Note that there is also no reason to depend on Marginalia if you provide your own annotation function. You can provide your own annotation-function in completing-read as part of the metadata. Marginalia is really meant to enhance existing commands (switch-to-buffer, find-file, etc.), not to be used in scenarios like here. I think I've mentioned that before. The only dependency you really need is Embark for action support. That's one of the main points of this package suite, everything is decoupled. Mostly standard builtins/APIs are used.

@minad
Copy link

minad commented Feb 1, 2021

If you want to see prefix annotations (icons for example), you should use affixation-function instead of annotation-function. But this is only supported by Selectrum and Emacs 28 default completion. Ideally you would provide both for backward compatibility, the affixation function will take precedence if supported.

@bdarcus
Copy link

bdarcus commented Feb 1, 2021

Out of my depth, but wouldn't bibtex-completion-display-formats cover this?

Edit: sorry, I think I mean bibtex-completion-format-entry, which uses the above to configure; in any case, whatever appropriate function already in bibtex-completion. Though I guess @mtreca already looked into that.

@minad
Copy link

minad commented Feb 1, 2021

@bdarcus Yes, by providing your own annotation function you can do your own formatting in the way you want it. This is better than using the marginalia specific internals.

@minad
Copy link

minad commented Feb 1, 2021

One more suggestion - you can also remove the Embark dependency and just provide the keymap and the actions as commands. Then this keymap should be bound via Embark to the completion category in the init.el use-package definition. But this way you already get commands out of all the actions if you only have bibtex-completion+this-new-package. And then my point was that the stuff should just be moved into the bibtex-completion package altogether :)

But maybe I should stay out of your way now... Please ping me if you want to discuss this further.

@tmalsburg
Copy link
Owner

tmalsburg commented Feb 2, 2021

Looks promising!

No simple mechanism to display the notes and pdf icons so far (I did not look very hard at this issue yet, though).

Have a look at bibtex-completion-format-entry. (But perhaps I missing something.)

embark-bibtex.el Outdated
(bibtex-completion-candidates)))

;;;###autoload
(defun embark-bibtex (bib-entry)

This comment was marked as outdated.

@mtreca
Copy link
Author

mtreca commented Feb 4, 2021

Thanks a lot for the feedback everyone!

@minad I did not know you could provide your own annotation function in completing-read. That is great news as I can indeed remove the marginalia dependency for now. As for removing the embark dependency, I am not sure I understand what you mean yet, but thanks for the advice. I will likely ping you later when I will tackle this, if you don't mind.

@bdarcus @tmalsburg Thanks for the remarks.

I will take all of your feedback into account for the next iteration.

@mtreca
Copy link
Author

mtreca commented Feb 4, 2021

One limitation of marginalia and providing completion annotations is that the annotations are not matchable when selecting a candidate. For instance, if one of the entries is displayed as

⌘ 2006 Yu, X-H and Recker, Wilfred W Stochastic adaptive control model for traffic signal systems article yu2006stochastic

ivy-bibtex (and I guess helm-bibtex) allow for matching parts of the title or authors. Providing annotations for standard completing-read (through annotation functions or marginalia) only allow to match the article key (in this case yu2006stochastic).

I see this as a pretty big limitation of the embark-bibtex frontend since fuzzy searching article titles or authors is extremely helpful IMO.

An option would be to wrap completion candidates using cons cells, as described by John Kitchin in this answer.

@bdarcus
Copy link

bdarcus commented Feb 4, 2021

Providing annotations for standard completing-read (through annotation functions or marginalia) only allow to match the article key (in this case yu2006stochastic).

I see this as a pretty big limitation of the embark-bibtex frontend since fuzzy searching article titles or authors is extremely helpful IMO.

Indeed; if this is the case, I would find it unusable. The whole point is, if I have 1000 items in my db, I don't know the key. So I type in parts of authors and titles to narrow it.

But I thought bibtex-complete presented an alist?

@minad
Copy link

minad commented Feb 4, 2021

I see this as a pretty big limitation of the embark-bibtex frontend since fuzzy searching article titles or authors is extremely helpful IMO.

There is no problem actually. Just make everything you want to match on part of the candidate string. Annotations are only auxiliary information which is per definition by Emacs not supposed to be matched on. Maybe annotations are not even necessary since you want to be able to match on everything?

@bdarcus
Copy link

bdarcus commented Feb 4, 2021

Just make everything you want to match on part of the candidate string.

So @mtreca - can you not then just use bibtex-completion-format-entry to match on, which then allows the user also to configure the details of that via bibtex-completion-display-formats?

@mtreca
Copy link
Author

mtreca commented Feb 4, 2021

@minad Yes this is what I am going for. it is actually easier than what I thought it was going to be. This snippet does proper completion matching, leverages built-in formatting functions from bibtex-completion, and does not use marginalia!

(let ((candidates
       (mapcar
        (lambda (cand)
          (cons (bibtex-completion-format-entry cand 188)
                (cdr (assoc "=key=" cand))))
        (bibtex-completion-candidates))))
  (cdr (assoc (completing-read "bib: " candidates) candidates)))

@minad
Copy link

minad commented Feb 4, 2021

Next step: No Embark :)

@mtreca
Copy link
Author

mtreca commented Feb 4, 2021

@minad Agreed. Could you explain a bit further how I can shortcut the embark dependency by "providing the keymap and the actions as commands. " ?

@minad
Copy link

minad commented Feb 4, 2021

Okay, I can take another look soon. This PR is not up-to-date yet, right? Can you please push these Marginalia changes?

@mtreca
Copy link
Author

mtreca commented Feb 4, 2021

No I did not push the changes yet, I am struggling a little incorporating these changes with the interactive lambda function using complete-with-action (my elisp skills are not great). I will push as-is for now.

embark-bibtex.el Outdated Show resolved Hide resolved
embark-bibtex.el Outdated Show resolved Hide resolved
embark-bibtex.el Outdated Show resolved Hide resolved
embark-bibtex.el Outdated
("s" embark-bibtex-show-entry)
("l" embark-bibtex-add-pdf-to-library))

(add-to-list 'embark-keymap-alist '(bib . embark-bibtex-map))
Copy link

@minad minad Feb 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. At this point, this line is the only dependency with Embark. Remove it and the Embark dependency. This line can be written in the user config instead in order to make the keymap available to embark.

Copy link

@bdarcus bdarcus Feb 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the end, the idea for a user who has selectrum setup as their completion framework, is they just install this package (or, ideally at some point, just bibtex-completion), and install and configure embark to use the keybindings, and so associate the bibtex-completion commands with the selected candidates?

@mtreca
Copy link
Author

mtreca commented Feb 5, 2021

@minad I added your remarks to a new version. My issue is that embark actions do not seem to be recognized anymore.

@bdarcus
Copy link

bdarcus commented Feb 5, 2021

I just tested this; just initially loading the bib and narrowing.

One little thing: the matching is ordered? E.g. author, then title, etc.

And if there's no author, I actually can't match on the rest (like title).

Where should this best be addressed, and how? In the users selectrum setup? In this package?

I was just testing this with emacs-sandbox, and the most minimal configuration.

@mtreca
Copy link
Author

mtreca commented Feb 5, 2021

@bdarcus I am pretty sure candidate ordering is outside of the scope of this project. I personally use prescient but the orderless package is also popular.

Did you try a fuzzy completion-style?

@bdarcus
Copy link

bdarcus commented Feb 5, 2021

@bdarcus I am pretty sure candidate ordering is outside of the scope of this project. I personally use prescient but the orderless package is also popular.

Yes, you are right; if I load selectrum-prescient, it works as I expect!

Edit: when the key-map is working, how does one invoke it from the selected candidate?

@bdarcus

This comment has been minimized.

@bdarcus
Copy link

bdarcus commented Feb 22, 2021

@tmalsburg - on the naming question, you suggested this:

By the way, I'd call the new package completing-read-bibtex in line with helm-bibtex and ivy-bibtex. That’s largely self-explanatory for anyone who’s made contact with helm- and ivy-bibtex.

Wouldn't that result in/require unnecessarily and awkwardly verbose command names?

I actually think there's a good argument to keep bibtex-actions, given the different design (flat, interactive, commands). The package and commands names would be concise, and very clear what they do.

Or, a reasonable compromise: cr-bibtex?

@bdarcus

This comment has been minimized.

@oantolin
Copy link

Let me start with the caveat that I have only skimmed the conversation here, and haven't read it the whole thing carefully, nor have I looked at the code yet.

This suggests to me that I can access any action from the snapshot, which is what we want.

But while I can access the default action, I can't figure out how to access others actions (say open the PDF) from that view, which would be ideal for this use case. The embark-act keybinding, for example, doesn't work.

Seems somehow that documentation could use amending to clarify. And if one can only access the default action, is this restriction necessary?

You can definitely use all the actions you have from an Embark collect buffer (indeed, the buffers wouldn't be very useful if you couldn't!). By default in the Embark collect buffers embark-act is bound to a, but if you have a global keybinding for embark-act you should be able to use that too. And of course, there is always M-x embark-act too.

I don't know what is keeping this from working for you, but we'll figure it out. So what exactly happens when you try to run embark-act in the Embark collect buffer? Also, does this inability to use actions in the Embark collect buffer only happen with your bibtex actions or is it a general problem with your installation? Have you tried embark-collect-snapshot from, say find-file and then tried to run some actions from the collect buffer?

The other issue I noted above also. I can't get the prefix map submenu to display. Is that possibly related to this bug your reported?

I think this does sounds like exactly the issue that @minad reported. I know how to fix it but haven't done it yet since it is a fairly large change. (I hadn't even noticed the problem because I don't use which-key myself.) Knowing that more people are using prefix keymaps in Embark, I'll make more of an effort to find the time.

However, I don't understand why you are using a prefix keymap here. Am I understanding correctly that you are placing every single bibtex action behind the prefix b? So no matter what action a user of your package wants to use, they press whatever keybinding they have for embark-act, then b, then finally select the action? If that's the case I highly recommend skipping the b, it sounds unnecessary and inconvenient.

It looks like you already have all the actions in a keymap called bibtex-actions-map. Then to use with Embark, all a user of your package needs to do to access the actions without a prefix key is:

(setf (alist-get 'bibtex embark-keymap-alist) 'bibtex-actions-map)

@bdarcus
Copy link

bdarcus commented Feb 23, 2021

Let me start with the caveat that I have only skimmed the conversation here, and haven't read it the whole thing carefully, nor have I looked at the code yet.

No problem; you shouldn't need to read the conversation beyond that one message. Thanks!

So what exactly happens when you try to run embark-act in the Embark collect buffer? Also, does this inability to use actions in the Embark collect buffer only happen with your bibtex actions or is it a general problem with your installation? Have you tried embark-collect-snapshot from, say find-file and then tried to run some actions from the collect buffer?

Context: using Doom evil, and the WIP selectrum et al module.

doomemacs/doomemacs#4664

If I hit "a" within the collect buffer on find-file, the cursor just changes to insert mode.

If I do the embark-act keybinding instead, it opens a previously opened buffer.

That's what I see when using the bibtex commands also.

The other issue I noted above also. I can't get the prefix map submenu to display. Is that possibly related to this bug your reported?

I think this does sounds like exactly the issue that @minad reported. I know how to fix it but haven't done it yet since it is a fairly large change. (I hadn't even noticed the problem because I don't use which-key myself.) Knowing that more people are using prefix keymaps in Embark, I'll make more of an effort to find the time.

No problem at all. I'm just glad we know the problem, and it wasn't me!

However, I don't understand why you are using a prefix keymap here. Am I understanding correctly that you are placing every single bibtex action behind the prefix b? So no matter what action a user of your package wants to use, they press whatever keybinding they have for embark-act, then b, then finally select the action? If that's the case I highly recommend skipping the b, it sounds unnecessary and inconvenient.

OK, we'll do that.

The issue I was trying to work around I note above in the "sorting/grouping" section of this post. Basically, too-long command names (which is related to an orthogonal question about package name), and wanting to sort or group the keybindings so easier to quickly grok.

It looks like you already have all the actions in a keymap called bibtex-actions-map. Then to use with Embark, all a user of your package needs to do to access the actions without a prefix key is:

(setf (alist-get 'bibtex embark-keymap-alist) 'bibtex-actions-map)

@oantolin
Copy link

So basically the Embark collect buffers don't let you use other actions at all under your Doom Emacs configuration, @bdarcus? Since I don't use Doom, maybe you can help me debug this issue. First, just to check the obvious possible problem: you are sure you are running embark-act, right? There isn't some other Doom keybinding shadowing your keybinding for embark-act? Did you try running it by name instead of by keybinding? (I don't know if M-x works in Doom, but if it doesn't, maybe : runs arbitrary Emacs commands too?)

@bdarcus
Copy link

bdarcus commented Feb 23, 2021

Did you try running it by name instead of by keybinding?

Ugh; I should have thought of that!

If I run the command directly, it works fine. I'll report it to that doom PR.

Thanks again @oantolin!

@oantolin
Copy link

That's a relief, @bdarcus! It's sounds like it's just a keybinding or Evil state issue, then, which should probably just be fixed locally in your personal configuration. You should bind some key to run embark-act in the Evil normal state in buffers that are in embark-collect-mode, or alternatively you could configure things so that embark-collect-mode buffers start in Evil's Emacs state (but you'd loose your nice Evil navigation, so probably the first choice is better).

@bdarcus
Copy link

bdarcus commented Feb 23, 2021

Doom has extensive keybinding support, including in that module for embark-act, so it should really be fixed there. I'll link your comment over there.

@oantolin
Copy link

oantolin commented Feb 23, 2021

Oh, I went to doomemacs/doomemacs#4664. It also loads Embark! So then the Evil normal state for embark-act in Embark collect buffers should be added there, not in your personal configuration.

EDIT: Didn't see your above comment while writing this one.

@bdarcus
Copy link

bdarcus commented Feb 23, 2021

So for the broader questions I was asking, the simplest approach is to keep things as are with just completing-read, and encourage people to use embark and embark-collect-snapshot if they need more flexibility to act on multiple candidates quickly and easily. That's where the real power will be, it seems to me.

The remaining issues I see are (mostly) minor:

  1. I think this and Make bibtex-completion (more) interactive #361 are far enough long to make decisions on separate package vs mods to bibtex-completion, and if the former, whether hosted here.
  2. if a separate package hosted here, the package name. I prefer the current bibtex-actions because it and resulting command names are clear and concise, over the overly verbose completing-read-bibtex, which yield insane commands like completing-read-bibtex-insert-key. But a reasonable compromise might be cr-bibtex?
  3. need to adapt the --get-candidates function to add extra-search-fields metadata as hidden propertized text. This will allow the bibtex-completion-additional-search-fields to work as expected. Solution is here; just need to accept the suggestion.
  4. need documentation, with a good example config or two. Below @oantolin suggests configuring which-key to hide the embark-general-map keybindings from the display, which I think is a good idea, particularly if we can confine it to these commands (in other cases, we'd want the general keybindings, I think). Or maybe it would be good to keep a binding for embark-collect-snapshot?

@minad
Copy link

minad commented Feb 23, 2021

It seems @oantolin already answered most questions satisfactorily? Thank you, @oantolin!

@bdarcus Regarding completing-read-multiple - I believe there is no special support as of now. Therefore, as you said, everything should be kept as is.

Maybe Embark could get support for completing-read-multiple such that one could act on the already selected candidates at once. Independent of completing-read-multiple one could add an Embark feature which allows selecting candidates and then act on all the selected candidates. This could work with plain completing-read. I am unsure if this has already been considered - it would be quite a deviation from the current approach.

@oantolin
Copy link

@bdarcus About using the b prefix for all actions you wrote:

The issue I was trying to work around I note above in the "sorting/grouping" section of this post. Basically, too-long command names (which is related to an orthogonal question about package name), and wanting to sort or group the keybindings so easier to quickly grok.

I think probably a better fix would be to hide all the general actions (that is, actions bound in embark-general-map) from the which-key display. There are so many now! And each of them is useful, I believe, so I like having them there, always available, but since they are always there, maybe users don't need to be constantly reminded of them.

@bdarcus
Copy link

bdarcus commented Feb 23, 2021

I think probably a better fix would be to hide all the general actions (that is, actions bound in embark-general-map) from the which-key display.

At first glance, I like that idea @oantolin.

Would we suggest that here, say in an example configuration in the docs?

@@ -0,0 +1,116 @@
;;; bibtex-actions.el --- Bibliography manager action

Copy link

@bdarcus bdarcus Feb 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
;;; Commentary:
;;;
;;; Provides wrappers around bibtex-completion action-functions, to make
;;; them available as standard emacs commands, and within completing-read
;;; compatible completion systems such as selectrum and icomplete-vertical.
:::
::: For additional power and flexibility, install marginalia and embark, and set up
;;; the action keybindings like so:
;;;
;;; (setf (alist-get 'bibtex embark-keymap-alist) 'bibtex-actions-map)
;;;

@@ -0,0 +1,116 @@
;;; bibtex-actions.el --- Bibliography manager action
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
;;; bibtex-actions.el --- Bibliography manager action
;;; bibtex-actions.el --- Bibliography manager actions

@bdarcus
Copy link

bdarcus commented Feb 23, 2021

Maybe Embark could get support for completing-read-multiple such that one could act on the already selected candidates at once. Independent of completing-read-multiple one could add an Embark feature which allows selecting candidates and then act on all the selected candidates.

For this example, I also think the selectrum UI for multiple selection would need work. It would seem to be better, at least optionally, to be able to mark the selected candidates in the selectrum minibuffer, as in helm.

But with embark-collect, maybe that's not needed. Maybe that could be extended to also add ability to mark and act on multiple candidates?

@bdarcus
Copy link

bdarcus commented Feb 23, 2021

Maybe another PR could be opened regarding this issue, in which we could maybe convince you further! In any case I would be interested in participating in this other PR as well.

At @mtreca's request, and adapting his code, I've opened (the start of) just such a PR at #361!

@tmalsburg
Copy link
Owner

Hi all, I'm currently super busy at work and couldn't follow the recent discussion. I will soon catch up and look at the PR but it may take a couple of days.

@mtreca
Copy link
Author

mtreca commented Feb 24, 2021

Same as @tmalsburg, I am really busy this month. Will take time to review these discussions and apply suggested changes.

@bdarcus
Copy link

bdarcus commented Feb 24, 2021

Sounds good @mtreca @tmalsburg.

To pare down the content here, I've hidden some of the recent longer posts of mine. I think this post summarizes my view of the current status.

#355 (comment)

"Return all keys from bibtex-completion-candidates."
(mapcar
(lambda (cand)
(cons (bibtex-completion-format-entry cand (1- (frame-width)))

This comment was marked as outdated.

This comment was marked as outdated.

Comment on lines +21 to +27
(defun bibtex-actions--get-candidates ()
"Return all keys from bibtex-completion-candidates."
(mapcar
(lambda (cand)
(cons (bibtex-completion-format-entry cand (1- (frame-width)))
(cdr (assoc "=key=" cand))))
(bibtex-completion-candidates)))
Copy link

@bdarcus bdarcus Feb 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion uses cl-loop, as elsewhere in the codebase and for better performance, and also uses propertize to allow us to support bibtex-completion-additional-search-fields.

Suggested change
(defun bibtex-actions--get-candidates ()
"Return all keys from bibtex-completion-candidates."
(mapcar
(lambda (cand)
(cons (bibtex-completion-format-entry cand (1- (frame-width)))
(cdr (assoc "=key=" cand))))
(bibtex-completion-candidates)))
(defun bibtex-completion--get-candidates ()
"Return all keys from bibtex-completion-candidates."
(cl-loop
for candidate in (bibtex-completion-candidates)
collect
(cons
;; Here use one string for display, and the other for search.
(propertize
(car candidate) 'display (bibtex-completion-format-entry candidate (1- (frame-width))))
(cdr (assoc "=key=" candidate)))))

@bdarcus
Copy link

bdarcus commented Feb 28, 2021

I think probably a better fix would be to hide all the general actions (that is, actions bound in embark-general-map) from the which-key display.

@oantolin - maybe fodder for your wiki: how do I do this?

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.

5 participants