helm-bibtex icon indicating copy to clipboard operation
helm-bibtex copied to clipboard

[WIP] Add embark-bibtex frontend.

Open mtreca opened this issue 3 years ago • 78 comments

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.

mtreca avatar Feb 01 '21 20:02 mtreca

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

bdarcus avatar Feb 01 '21 20:02 bdarcus

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 avatar Feb 01 '21 21:02 minad

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.

minad avatar Feb 01 '21 21:02 minad

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.

bdarcus avatar Feb 01 '21 21:02 bdarcus

@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 avatar Feb 01 '21 21:02 minad

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.

minad avatar Feb 01 '21 21:02 minad

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

tmalsburg avatar Feb 02 '21 14:02 tmalsburg

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 avatar Feb 04 '21 16:02 mtreca

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.

mtreca avatar Feb 04 '21 16:02 mtreca

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?

bdarcus avatar Feb 04 '21 17:02 bdarcus

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?

minad avatar Feb 04 '21 17:02 minad

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?

bdarcus avatar Feb 04 '21 17:02 bdarcus

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

mtreca avatar Feb 04 '21 17:02 mtreca

Next step: No Embark :)

minad avatar Feb 04 '21 17:02 minad

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

mtreca avatar Feb 04 '21 17:02 mtreca

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

minad avatar Feb 04 '21 17:02 minad

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.

mtreca avatar Feb 04 '21 17:02 mtreca

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

mtreca avatar Feb 05 '21 14:02 mtreca

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.

bdarcus avatar Feb 05 '21 14:02 bdarcus

@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?

mtreca avatar Feb 05 '21 15:02 mtreca

@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 avatar Feb 05 '21 15:02 bdarcus

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

That's a very good question, I am not really sure. By using embark, you can define a shortcut key to trigger embark-act like so:

(use-package embark
  :ensure t
  :bind (:map selectrum-minibuffer-map
         ("M-o" . embark-act)))

You also have to manually add the map to the embark keymap as I removed the explicit dependency:

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

(Please note that the keymap is not picked-up by embark-actions yet, this probably has to do with the recent refactoring).

That being said, I don't think there is a built-in way to call different actions on candidates in Emacs. I guess the long-term plan is to have multiple action and completions available to users (one per action), in which case embark would not be required since one could simply call bibtex-completion-open-pdf and use standard completion there without needing to select an action.

mtreca avatar Feb 05 '21 15:02 mtreca

@mtreca Yes, this is right. You have to call the action via embark-act! There is no built-in way to call different actions on candidates without Embark, but the embarked actions are actually proper commands now, so you can call them directly via M-x as you already wrote with bibtex-completion-open-pdf. It is not as comfortable but it is the Emacs default way. I am not claiming that Emacs is comfortable by default, for me it is barely usable honestly.

minad avatar Feb 05 '21 15:02 minad

So, I changed a few things in between iterations.

  • Since embark is no entirely optional, I renamed the module and function prefixes to bibtex-actions
  • The macro now autonames functions based on their base names, and take docstrings (I still have to document those).
  • I fixed the action selection for those functions, which should now work properly.

I still don't see why embark actions do not show up anymore when calling bibtex-actions, but I will figure this out later.

mtreca avatar Feb 05 '21 19:02 mtreca

Great work! :+1:

Since embark is no entirely optional, I renamed the module and function prefixes to bibtex-actions

My suggestion would be bibtex-completion. :laughing: (But everything should be working first before making this proposal again...)

minad avatar Feb 05 '21 19:02 minad

My suggestion would be bibtex-completion

I was thinking the same thing!

I still don't see why embark actions do not show up anymore when calling bibtex-actions, but I will figure this out later.

@mtreca - let me know when you've sorted this out, and I can test.

When I do "M-o" (using you example config above) on an entry, nothing happens, though I do see "Act" in the left corner, and the default action works if I hit enter.

But I don't see any of the other action options, and if I use the keys specified in your code, it says "Not an action."

I guess that's what you're referring to above?

image

bdarcus avatar Feb 05 '21 19:02 bdarcus

Well, did you use bibtex here?

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

In the comment above by @mtreca he used bib, but the completion category specified in the completing-read call is bibtex. The completion category/type is how Embark identifies the appropriate actions.

minad avatar Feb 05 '21 23:02 minad

Thanks @minad.

This minimal code, with emacs-sandbox, does now give me access to the commands.

(load "/tmp/bib/bibtex-actions.el")

(setq bibtex-completion-bibliography
      '("~/org/bib/academic.bib"
        "~/org/bib/data.bib"))

(use-package embark
  :ensure t
  :bind
  ("M-o" . embark-act))

(add-to-list 'embark-keymap-alist '(bibtex . bibtex-actions-map))

But embark doesn't present me with a menu of those actions, which is what I was expecting, a la ivy-bibtex.

Am I missing some config detail on embark, is something missing from this package, or just not supported?

bdarcus avatar Feb 06 '21 00:02 bdarcus

Did you setup Embark properly with which-key? Embark uses which-key as menu. But you can also use a completing-read prompter. Please take a look at the Embark README.

minad avatar Feb 06 '21 00:02 minad

Just FYI for the full setup I am using Selectrum(Icomplete-vertical) + Marginalia + Embark + Orderless(Prescient) + Which-key + Consult. In parens the alternatives. I am not sure if I can point you to a unified resource for how to setup all these since the packages are still changing. But if you copy the basic configurations from the READMEs of these projects it should work.

minad avatar Feb 06 '21 00:02 minad

For quick experiments regarding this PR just use emacs -Q + Embark(for actions) + Which-Key(for visual menu). And maybe set the completion styles to substring.

minad avatar Feb 06 '21 00:02 minad

Did you setup Embark properly with which-key?

No, but I just did, and that now works. Thank you.

It would probably be worth adding some of these details to the comments/docs, once this gets farther along.

Might also be nice to be able to group the which-key commands as well.

bdarcus avatar Feb 06 '21 01:02 bdarcus

@minad Good catch regarding the use of 'bib' and 'bibtex', this is why my embark actions where not available.

I tested the new iteration of this PR and everything seems to work on my side. So there are now two entry points to this front-end:

  • bibtex-actions (the name should be changed), which functions similarly to ivy-bibtex and helm-bibtex provided embark is enabled for bibtex actions.
  • Individual bibtex actions, which call completing-read on a specific bibtex-completion function.

I guess the second group of actions could indeed easily be added to the bibtex-completion core.

One corner case that I still need to figure out is to make sure that (bibtex-completion-init) is called before any bibtex-actions funciton is called. Should be fixed today.

mtreca avatar Feb 06 '21 09:02 mtreca

@mtreca

bibtex-actions (the name should be changed), which functions similarly to ivy-bibtex and helm-bibtex provided embark is enabled for bibtex actions. Individual bibtex actions, which call completing-read on a specific bibtex-completion function. I guess the second group of actions could indeed easily be added to the bibtex-completion core.

I wonder a bit about this design, what makes the default-action now different from the individual actions. For me this is a flat list of commands. There is bibtex-completion-insert, bibtex-completion-open-pdf, ... This means the user just binds the one or two actions they use the most to some keys and the other actions are always available. There is no real advantage in going through the single action entry point. Actually it is rather the advantage of this design that you have the option to bind multiple commands directly for faster access.

One corner case that I still need to figure out is to make sure that (bibtex-completion-init) is called before any bibtex-actions funciton is called. Should be fixed today.

Call it in bibtex-action-read? Is this similar to bookmark-maybe-load, which loads the bookmark list?

minad avatar Feb 06 '21 13:02 minad

There is no real advantage in going through the single action entry point. Actually it is rather the advantage of this design that you have the option to bind multiple commands directly for faster access.

In the end, I guess it's just a UX question; what's most intuitive and convenient for the user?

The single entry point is basically the user saying "I want to search my bib collection."

The specific commands are them saying "I want to insert a citation key/open a PDF/etc."

The specific commands wouldn't be changing with the removal of the single entry point; right?

I'm agnostic/not sure.

Edit: I guess the other related issue is if the commands can be in bibtex-completion, without the single entry point, there'd be no need for this package.

bdarcus avatar Feb 06 '21 14:02 bdarcus

The specific commands wouldn't be changing with the removal of the single entry point; right?

They would not change. All I am saying is the following: I have a list of commands operating on bibtex entries. I will bind the most commonly used command to a specific key (the default action by my own choosing, maybe insert) and then I have another often used action (maybe open pdf), which I bind to another key. All the other actions can be accessed via the Embark keymap or via M-x. I just don't see why there is a special default action on the level of bibtex-completion and in this PR. This makes sense if you have the action/command distinction - in Helm and Ivy it makes sense, in this PR not so much I think.

For this reason I would propose to remove the bibtex-actions-default-action default action. The user can just bind the action they want to some global keymap, e.g., C-c b. There is no reason for an additional defcustom. Furthermore bibtex-actions can be removed. All I am proposing here is a simplification without losing any functionality.

minad avatar Feb 06 '21 16:02 minad

The next logical steps could be the following:

  1. Add the interactive spec directly to all of the bibtex-completion actions.
(interactive (list (cdr (assoc (bibtex-actions--read) (bibtex-actions--get-candidates)))))

I would maybe clean this up a bit, such that it is just (interactive (list (bibtex--completions-read))). The actions don't have an interactive spec as of now, so this is a pure addition.

  1. The keymap could also go to bibtex-completions. You can bind this keymap as a prefix map to C-c b. Then C-c b p would open pdf and so on. This would also make sense even if you don't use Embark, just plain Emacs.

In the end one would not even need a separate package. This was my original proposal :)

minad avatar Feb 06 '21 16:02 minad

I'm convinced!

So then making this available for selectrum users wouldn't require any code; just a short section of the bibtex-completion docs, say with a suggested config for embark.

bdarcus avatar Feb 06 '21 17:02 bdarcus

Let's see what @mtreca and @tmalsburg think about it.

Maybe there is one thing which would be required to make this fair to helm and ivy users. The bibtex--completions-read function which goes into the interactive spec, should probably be written such that you can configure the completion backend. The default implementation based on completing-read from this PR should go the bibtex-completion package and the helm and ivy packages could provide their respective versions. This way you would have all bibtex commands available via M-x in all completion systems and all bibtex commands could also be triggered as actions in each completion system. You would get the same power with each of the completion systems and the change is also non-intrusive. The only design point which is lost is that there is no backend/frontend distinction anymore. But I would argue that this distinction has been an artificial one.

minad avatar Feb 06 '21 17:02 minad

I do agree with @minad regarding the removal of the defcustom which does not seem to serve a purpose anymore.

However, I think that @bdarcus makes a valid point regarding the design decision of removing a single entry point. Suppose I have this configuration, related the workflow mentioned by @minad, which I think a lot of users would be following:

(use-package bibtex-completion
  :bind (("C-c b c" . bibtex-actions-insert-key)
         ("C-c b p" . bibtex-actions-open-pdf)))

In the rare occasions where I would need to call a bibtex-actions command that is not bound, do you think it makes more sense to call it from one of the bound functions and toggle the embark actions, or to use a specific command? I do agree that this distinction is artificial since the embark keymap is accessible from any bibtex-actions command, but it might be counter-intuitive to some to call, for instance, bibtex-actions-open-pdf to actually call another command through embark that has nothing to do with PDFs. This could be for instance confusing for users switching from ivy or helm (such as myself), who are expecting a generic ivy-bibtex function to trigger actions on.

That being said, this functions would really be a placeholder (no default actions, just a completion view with embark support), and removing it would simplify the code further. I would also like to hear what @tmalsburg thinks of this.

mtreca avatar Feb 06 '21 19:02 mtreca

In the rare occasions where I would need to call a bibtex-actions command that is not bound, do you think it makes more sense to call it from one of the bound functions and toggle the embark actions, or to use a specific command?

In this case I would probably call M-x bibtex-the-command-I-want or I would know that I can call any bibtex command and just take a look at the Embark actions.

This could be for instance confusing for users switching from ivy or helm (such as myself), who are expecting a generic ivy-bibtex function to trigger actions on.

I don't think it would be confusing as soon as you get accustomed to the Embark work flow, which is basically just the standard Emacs workflow. Everything is just a command. Embark only offers additional quick access commands on top via the action menu. You can always execute exactly the command you want via M-x.

That being said, this functions would really be a placeholder (no default actions, just a completion view with embark support), and removing it would simplify the code further.

Yes, it is only a no-op placeholder or it would execute a "default action". But then it makes more sense to directly invoke the default action command.

Regarding the confusion - one point I would like to add - I suggest it is better to make the package somehow adapt to the Embark/Emacs way instead of trying to implement its own paradigm. This is what a more experienced user of the package suite would expect I guess. If you are looking at it from the perspective of the helm-bibtex package, it may be confusing, but I don't think it would be in the greater scheme of things. People transitioning from Ivy/Helm to Embark already have some adaption to make, and under such circumstances I would value consistency. I think the main thing which has to click is that Embark is really only normal commands, I mean it is super simple. Note that you can even call M-o M-x some-command, if M-o triggers Embark in your case - redirecting to any different action command you like. The keymaps are actually only a convenience feature :)

minad avatar Feb 06 '21 19:02 minad

  1. Add the interactive spec directly to all of the bibtex-completion actions...
    I would maybe clean this up a bit, such that it is just (interactive (list (bibtex--completions-read))).

So to boil this down, and to tie back to your earlier suggestion, and this more recent one, we want this, where bibtex--completions-read then uses ivy or helm in their respective packages, but defaults to completing-read in bibtex-completion?

(defun bibtex-completion-open-any (keys)
  (interactive (list (bibtex--completions-read))
  (bibtex-completion-open-pdf keys 'bibtex-completion-open-url-or-doi))

Edit: not sure, but maybe the function should be called bibtex-completion--read?

bdarcus avatar Feb 07 '21 14:02 bdarcus

One corner case that I still need to figure out is to make sure that (bibtex-completion-init) is called before any bibtex-actions funciton is called. Should be fixed today.

Call it in bibtex-action-read? Is this similar to bookmark-maybe-load, which loads the bookmark list?

I does look similar. This is what I ended up doing.

I would maybe clean this up a bit, such that it is just (interactive (list (bibtex--completions-read))). The actions don't have an interactive spec as of now, so this is a pure addition.

Just to make sure I understand what you are suggesting: We should add a (bibtex--completions-read) function that, depending on the chosen backend, will provide adequate candidates to the completion frontend (helm, ivy, or standard completing-read). That way, pretty much all of this PR can be directly integrated into bibtex-completions. Am I correct?

If so, I really like this idea, it would be a pretty elegant solution since:

  • There would be no need for an additional module file, and potentially another MELPA module as initially suggested.
  • No external dependencies are needed, not even embark, as all completion commands are now directly accessible without having to have a completion action selection mechanism enabled (ivy, helm, embark).
  • Potentially simpler code in to helm-bibtex and ivy-bibtex frontends.

mtreca avatar Feb 07 '21 17:02 mtreca

Just to make sure I understand what you are suggesting: We should add a (bibtex--completions-read) function that, depending on the chosen backend, will provide adequate candidates to the completion frontend (helm, ivy, or standard completing-read). That way, pretty much all of this PR can be directly integrated into bibtex-completions. Am I correct?

Yes.

There would be no need for an additional module file, and potentially another MELPA module as initially suggested.

Yes. No need for another package.

No external dependencies are needed, not even embark, as all completion commands are now directly accessible without having to have a completion action selection mechanism enabled (ivy, helm, embark).

Yes, it would work out of the box with Emacs default completion. But if you install the helm-bibtex package you get helm actions, if you install the ivy-bibtex package you get ivy actions and if you install embark you can use the keymap as embark action keymap.

Potentially simpler code in to helm-bibtex and ivy-bibtex frontends.

I am not sure about that, maybe. But very important - there should be no degradation in functionality for Helm or Ivy. Rather you can access all the bibtex commands directly instead, where all of them have all additional actions. The ui would be consistent over all the completion systems, which I find desirable.

minad avatar Feb 07 '21 17:02 minad

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

Bibtex-completion returns entries in the reverse order in which they appear in the .bib file. So when I add a new entry at the end of the file, it appears at the top of the list. Personally, I never felt we needed sorting capabilities and over the years there was only one FR regarding sorting (I think). One issue with sorting is that some people have really large bibliographies (e.g. crypto.bib) and sorting be too slow when done ad-hoc.

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

Sorting, to me, seems like a UI feature. So it should probably not be part of bibtex-completion.

tmalsburg avatar Feb 08 '21 11:02 tmalsburg

@bdarcus

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

I think that would work against the design of some completion styles. For example there is the completion style named orderless, which is not about the order of the candidates, but rather about the order of the space-separated matching terms. You can match in arbitrary order, first write some author name, then the a part of the title or the other way round. This allows for a lot of flexibility and faster narrowing in my opinion. I actually recommend Orderless in combination with Selectrum, Embark, Consult etc. This is better than the built-in completion-styles like substring, basic, flex etc. Helm, Ivy, Prescient allow matching similar to Orderless.

minad avatar Feb 08 '21 11:02 minad

Sorting, to me, seems like a UI feature. So it should probably not be part of bibtex-completion

Yeah, totally fine with that. I had just needed to setup selectrum to use prescient (or, per @minad, orderless).

bdarcus avatar Feb 08 '21 11:02 bdarcus

Interesting discussion and cool to see how this project is coming along. I'm trying to keep up with this thread, but unfortunately I don't have time to read everything carefully. If you need input from me specifically, please @ me.

Regarding the architecture: I'm not convinced that UI code should go into bibtex-completion. As I understand it, the idea is to make the action-functions, which are currently pure API, into proper Emacs commands. This seems elegant because it is minimally invasive, but I'm afraid that it will complicate maintenance of this code in the future. For instance, if we'd like to change the UI of the completing-read front end, it would affect the API as well and hence helm-bibtex and ivy-bibtex. Likewise, if we need to change the API, it will affect the UI of the completing-read front end. Not ideal. Also, users of helm/ivy-bibtex would effectively get two UIs, the helm/ivy front ends plus the action commands. Also keep in mind that there are other packages that depend on bibtex-completion as well and which might also be affected by these changes (org-ref, org-roam-bibtex).

As far as I’m concerned it’s best to keep things modular, the features orthogonal, and the code as easy to maintain as possible for everyone involved. If that means that we'll have a couple of extra lines of code and a new package, that's an okay price to pay.

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.

Once again, great work, and I'm looking forward to seeing the result.

tmalsburg avatar Feb 08 '21 12:02 tmalsburg

I am pretty sure it would not complicate maintenance, break compatibility and so on. And as I said the changes are minimally invasive. I am a bit disappointed that I or we seemed to have failed to convince you here. For sure it is okay to keep the current design. I believe it could be better, but it is your decision.

minad avatar Feb 08 '21 12:02 minad

Perhaps I'm misunderstanding the proposal, but let's say it was convenient to rename a user-facing function, e.g. because users perceive it to be misleading, how could we change that function without also changing the API for all other packages using bibtex-completion? When I created these functions they were not intended to be user-facing, and accordingly I didn't put much thought into their names. For instance, bibtex-completion-open-any is not great. I also don't see (could be my fault) the downside of placing the proposed front-end in a separate file. The cost is basically zero and for users it doesn't really make a difference.

tmalsburg avatar Feb 08 '21 13:02 tmalsburg

Regardless of what the final decision is (separate package or not), on this point I made earlier:

It would probably be worth adding some of these details to the comments/docs, once this gets farther along.

... the documentation should recommend prescient and/or orderless, and give an example configuration with (including setting up the keymap) and without embark.

Should also mention such a setup will work with selectrum.

bdarcus avatar Feb 08 '21 13:02 bdarcus

Thanks for your feedback @tmalsburg.

I think keeping things separated for now is OK. We could merge a new module, completing-read-bibtex, containing the current implementation, and document its possible integration with embark. That way, completing-read and selectrum users get a working front-end to bibtex-completion that re-uses a lot of builtin Emacs functionality without impacting ivy-bibtex and helm-bibtex users. This is a win-win situation IMO.

(This PR is not ready to merge yet, I still need to document the new module and "port" local bib functionality from ivy-bibtex).

On the longer run, I do agree with @minad's proposed implementation, but it might be outside of the scope of this PR (and of my elisp abilities). 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.

mtreca avatar Feb 08 '21 14:02 mtreca

Maybe another PR could be opened regarding this issue.

I'd guess this possible new PR would include the modifications (as demonstrated on this PR) to bibtex-completion, as well as helm/ivy-bibtex (just the one new function?).

Functionally, it would be the same as this package for the end user (since @mtreca already removed the single entry point from this PR), but may have other advantages, per @minad.

bdarcus avatar Feb 09 '21 14:02 bdarcus

Fyi in a recent Consult discussion I linked above (see in particular this comment https://github.com/minad/consult/issues/210#issuecomment-775923226), Embark got support for non-command actions, since this allows to retain text-properties and makes it easier to write custom actions in certain cases. This also applies to your use case, where everything is going through the single entry point as per the Ivy/Helm-UI. I still believe that the "everything is a command" paradigm is better since it gives the user more flexibility to bind the commands to keys and it works with just Emacs built-ins, without installing any package. But there is the alternative now.

For more details - Embark allows to bind these two types of actions now:

(defun function-action (target)
  ;; non interactive, target retains text properties
   ...)

(defun command-action (target)
   (interactive (list (completing-read ...))) ;; target is a plain string WITHOUT text properties
   ...)

See https://github.com/oantolin/embark/commit/b33ee3004da4b210da030f8d76194f81a94e51d6.

minad avatar Feb 10 '21 10:02 minad

Nice to see how quickly this ecosystem is evolving.

But on this ...

This also applies to your use case, where everything is going through the single entry point as per the Ivy/Helm-UI.

... @mtreca removed that single entry point. Not sure what he's thinking about its status as he wraps up final revisions, but doesn't seem he needs to add it back.

Edit: might be nice if the only difference between this separate package and the tweaks discussed above to bibtex-completion is simply a different use-package statement, so that this package is in part a test/demonstration of that idea, at least from a UI POV.

bdarcus avatar Feb 10 '21 16:02 bdarcus

I am quite busy with work this week, but I will definitely look at this discussion in detail when I have the chance. As @bdarcus said, nice to see the embark package evolving quickly!

mtreca avatar Feb 10 '21 16:02 mtreca

I was just testing the UX, to see how intuitive I find it. A few observations and suggestions:

Allowing multiple completion candidates (keys)

Edit: per discussion below, I think this is a non-issue for now. embark-collect-snapshot is a reasonable approximation, allowing quick actions on a candidate list, and also alternate actions.

Many of the bibtex-completion functions accept multiple keys, but the current code doesn't.

If I change completing-read to completing-read-multiple, selectrum will allow me to select multiple candidates, but I get an error when hitting enter with multiple, and nothing when only one.

There's also a UI oddity that happens when doing this, however, that we would want to fix. When selecting an item, you see this (the complete formatted string, rather than the key):

I think (but am not 100% sure) it'd be better to have the keys there, so one could see and much more easily edit the whole list of candidates.

In any case, this is a UX nuance that would be nice to figure out.

Command and package names

I actually prefer the bibtex-actions name. It's clear, and concise, and reflects the different approach of the ivy and helm alternatives.

A package name like completing-read-bibtex, yields awkward/verbose commands like completing-read-bibtex-insert-key. Note below the command names are so long which-key becomes less useful.

Maybe cr-bibtex would be a more reasonable compromise?

Sorting/Grouping of commands in Embark/which-key

Edit: this issue and the above could be addressed by my comment below on keybindings. It might be I just don't understand how to set this up correctly.

A minor thing, but it would be nice if the bibtex commands were grouped at the top of the lists of available commands in C-h and provided by which-key. Is that possible?

Seems like yes according to which-key docs, but I can't get it to work as I expect.

Note here also: the long command names get cut-off of the which-key menu.

Single command entry point?

I don't think it's needed, particularly if the individual commands are simple and straightforward. If we want to be consistent with ivy/helm-bibtex, we could simple add a [prefix]-search command?

embark-collect-snapshot opportunity

I'm still learning this ecosystem, but it strikes me embark-collect-snapshot could be really useful for this use case. It could, for example, be an alternative for my multiple candidates point above.

As in, if I'm writing a paper, or a section of a paper, I could run this command to collect the subset of items I want to cite, and then run actions on them from that buffer.

There might be some opportunity here in terms of configuration, though I need to think on it more.

bdarcus avatar Feb 16 '21 00:02 bdarcus

I'd guess this possible new PR would include the modifications (as demonstrated on this PR) to bibtex-completion, as well as helm/ivy-bibtex (just the one new function?).

I don't have the skill to do this, and so don't feel comfortable opening a new PR ATM.

But I did try to adapt the new bibtex-completion--read function @mtreca came up with, and then add the single line that @minad suggested to bibtex-completion-insert-key.

https://github.com/bdarcus/helm-bibtex/commit/81a76ac85fd3c8e401760485a19aa212680e6e1d

I think there's some minor problem somewhere, as I get a "wrong-type-argument sequencep.122" error.

If we did want to open a new PR on this, seems the next steps would be to fix these functions, and adapt it to helm/ivy-bibtex. Would obviously need to alleviate Titus' concerns for him to reconsider.

bdarcus avatar Feb 19 '21 14:02 bdarcus

Above, @minad suggested the following:

You can bind this keymap as a prefix map to C-c b. Then C-c b p would open pdf and so on.

I think this would be better from a UI POV, and address my points above about command names and sorting/grouping of the keybindings in which-key. The user would thus, upon selecting candidate(s):

  1. Hit return for the default action (which is available even without invoking embark-act).
  2. Hit b to bring up a secondary list of specific alternatives from the keymap.

But how to bind that prefix map, and get "+prefix" to render as "bibtex" or some such, where upon pressing "b" which-key pops up with the list of command from bibtex-actions-map?

Screenshot from 2021-02-19 15-50-04

It also occurs to me that if we did want to add what we've called a "single entry point", it should really just be a "search" command and binding, and so simply another flat command, equivalent to the others. So, to do the equivalent of helm-bibtex, one would simply do, for example C-c b s (though I guess there's no real bibtex-completion-search function ATM).

bdarcus avatar Feb 19 '21 19:02 bdarcus

@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 avatar Feb 22 '21 14:02 bdarcus

@minad - when you get a chance, a few questions:

Should we use completing-read-multiple? + related embark-collect-snapshot question

So I asked above about completing-read-multiple, but let me ask the question from a broader and more user-focused perspective.

I am writing a paper. I want to work with a a collection of references, and insert them as citations in different places within the text.

Sometimes those citations are single key, and other times multi key.

A single key citation [doe15].
A multi key citation [doe20, smith17, smith19].

What's the preferred way to handle this here for the multi key example?

  1. select multiple candidates using completing-read-multiple, run insert-key action on them
  2. use embark-collect-snapshot, run the (default only? see below) action on the ones I need
  3. allow both options?

If we don't use completing-read-multiple and a user isn't using embark, then they would have to run multiple cycles of command-complete-select action, which seems less-than-ideal.

Seems to me we want 3, ideally with embark allow different actions on the snapshot items, but I'd appreciate your thoughts.

embark-collection-snapshot non-default actions?

cc @oantolin

I'm confused on 2. The embark documentation says the following:

The embark-collect-snapshot command produces a buffer listing all the current candidates, for you to peruse and run actions on at your leisure.

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?

UI issue with completing-read-multiple

If we should use completing-read-multiple instead, how do we deal with the UI problem I note above (see the first screenshot)?

prefix map submenus in embark

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?

https://github.com/oantolin/embark/issues/139

How should we configure the prefix map so it works as intended?

bdarcus avatar Feb 23 '21 12:02 bdarcus

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)

oantolin avatar Feb 23 '21 13:02 oantolin

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.

https://github.com/hlissner/doom-emacs/pull/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)

bdarcus avatar Feb 23 '21 14:02 bdarcus

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

oantolin avatar Feb 23 '21 14:02 oantolin

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!

bdarcus avatar Feb 23 '21 14:02 bdarcus

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

oantolin avatar Feb 23 '21 14:02 oantolin

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.

bdarcus avatar Feb 23 '21 14:02 bdarcus

Oh, I went to hlissner/doom-emacs#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.

oantolin avatar Feb 23 '21 14:02 oantolin

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 #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?

bdarcus avatar Feb 23 '21 14:02 bdarcus

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.

minad avatar Feb 23 '21 14:02 minad

@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.

oantolin avatar Feb 23 '21 15:02 oantolin

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?

bdarcus avatar Feb 23 '21 15:02 bdarcus

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 avatar Feb 23 '21 18:02 bdarcus

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!

bdarcus avatar Feb 23 '21 22:02 bdarcus

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.

tmalsburg avatar Feb 24 '21 08:02 tmalsburg

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

mtreca avatar Feb 24 '21 08:02 mtreca

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.

https://github.com/tmalsburg/helm-bibtex/pull/355#issuecomment-784251233

bdarcus avatar Feb 24 '21 11:02 bdarcus

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?

bdarcus avatar Feb 28 '21 22:02 bdarcus