tmalsburg / helm-bibtex

Search and manage bibliographies in Emacs
GNU General Public License v2.0
463 stars 74 forks source link

[Feature/WIP] Add selectrum et al support #353

Open mtreca opened 3 years ago

mtreca commented 3 years ago

First of all thanks for this great package.

I recently moved to the selectrum completing framework, which plays very nicely with a certain number of other utilities such as marginalia for minibuffer annotations and embark for minibuffer actions.

Having a selectrum-bibtex frontend leveraging these tools would be great. I started working on a POC, I will open a PR when something usable comes out of it.

bdarcus commented 3 years ago

Yes please!

Happy to provide feedback.

mtreca commented 3 years ago

Awesome. I managed to port most of the features from ivy-bibtex quite easily. Working on marginalia support now!

minad commented 3 years ago

@mtreca I am not sure if you are using my Consult package, but I wonder if you could do the selectrum-bibtex port in the spirit of Consult by using completing-read instead of Selectrum-specific APIs? This way one would get compatibility with other completion systems like default completion, Icomplete-vertical and Selectrum.

mtreca commented 3 years ago

I am actually using your consult--read helper function in my script, which I guess is completing-read friendly. I will look into it!

minad commented 3 years ago

@mtreca Yes, consult--read is using completing-read. It is okay to use consult--read if

mtreca commented 3 years ago

While I think that most selectrum users will have consult, marginalia et al installed it might be better to use completing-read to limit dependencies. Embark will be necessary however since selectrum-bibtex will have to provide multiple actions during completion.

tmalsburg commented 3 years ago

Hi all, I'm not familiar with selectrum but in principle I'm happy to include support for frameworks other than helm and ivy. Note however that bibtex-completion (the backend) is available as a separate MELPA package, so you could host selectrum support also in a separate repository. I don't know enough about what you're planning to be able to say whether that makes more sense than including selectrum support here. The ivy and helm front-ends are sufficiently similar to benefit from being maintained by the same person in the same repo but the selectrum front-end may be different.

minad commented 3 years ago

@tmalsburg From what I see bibtex-completion uses completing-read and is already supported by Selectrum as is. Maybe @mtreca wants to provide additional actions and annotations via Embark and Marginalia?

mtreca commented 3 years ago

@tmalsburg Thanks for your comment, I did not know that bibtex-completion was available as a separate package. Most of my work so far is very similar to ivy-bibtex, and the finished product should be around the same amount of code (less than 200 LOC). @minad Yes, my main aim is to provide an "out-of-the-box" experience similar to ivy-bibtex, including actions on completions and possibly annotations for selectrum/icomplete users.

tmalsburg commented 3 years ago

@mtreca If selectum-bibtex closely follows ivy-bibex, it might make sense to host it here, so we can add new features there as well. But as you prefer.

I recommend having a look at helm-bibtex as well. Since helm-bibtex is older, has more users, since it's the framework that I'm using myself, and since helm is more powerful, it may have features that are absent in ivy.

bdarcus commented 3 years ago

Just a thought: maybe start a separate repo as you work on it, and to get feedback, and if it makes sense to merge here when closer to " done", then do that?

tmalsburg commented 3 years ago

From what I see bibtex-completion uses completing-read and is already supported by Selectrum as is.

bibtex-completion has no front-end for seaching bibtex entries. It uses completing-read only in some actions that can be applied to entries once they have been selected. If selectrum supports bibtex-completion there must be some supporting glue code for that somewhere. A small handful of lines is probably enough to get something basic rolling.

minad commented 3 years ago

@tmalsburg @mtreca

The most economic approach is the following - Add a completion category to the completing-read calls in bibtex-completion! Then provide Embark actions and Marginalia annotations in separate packages bibtex-marginalia bibtex-embark. This way you can simply reuse the bibtex-completion package without modifications. This is the intended design of Marginalia and Embark. Note that Marginalia and Embark have been designed to extend existing commands, for example commands living in the Emacs code base itself, like find-file, switch-to-buffer etc.

If you don't want to add a completion category in bibtex-completion, you can also add a prompt classifier to Marginalia such that the completion category of the bibtex-completion completing-read is adjusted.

minad commented 3 years ago

@tmalsburg

bibtex-completion has no front-end for seaching bibtex entries. It uses completing-read only in some actions that can be applied to entries once they have been selected. If selectrum supports bibtex-completion there must be some supporting glue code for that somewhere. A small handful of lines is probably enough to get something basic rolling.

Why not? If you would add this, annotations and actions could be simply injected from outside without modifications to the bibtex-completion package itself.

tmalsburg commented 3 years ago

Why not?

Sorry, I don't understand your question. Are you asking why bibtex-completion doesn't have a front-end for searching entries? That's because it's supposed to be a font-end agnostic back-end to be used by whatever front-end you might want to use with it. Initially there was only a helm-bibtex package but we factored out bibtex-completion when users wanted an ivy front-end with similar capabilities. So bibtex-completion is responsible for parsing the content of a bibliography (and some other things) and that content can then be used by various front-ends. Helm-bibtex and ivy-bibtex are simply very thin facades that provide access to the business logic implemented in bibtex-completion. Hope that makes sense.

minad commented 3 years ago

@tmalsburg Okay, I see. I expected from the name bibtex-completion that it provides basic completion support and I saw completing-read calls in the code base - therefore I made this assumption. Maybe it would make sense to add such a very basic frontend, based on completing-read directly to the bibtex-completion package. Then this bibtex-completion package would work out of the box with the Emacs default completion, Icomplete and Selectrum. This is for example the approach taken by https://github.com/d12frosted/flyspell-correct, which provides a core package based on completing-read and additional packages for Helm and Ivy. As I argued the Marginalia and Embark add-ons could be provided separately.

mtreca commented 3 years ago

@bdarcus Excellent idea. I am not sure I should be leading these efforts though, given that I am just discovering the selectrum/marginalia/consult/embark framework myself and that @minad seems pretty interested in this project.

@minad I agree with your points of providing marginalia/embark actions as add-ons that can easily be added to bibtex-completion. That way, native completion-read users do not need to depend on either packages. However, AFAIK and as @tmalsburg was saying, you do need to specify which action to use when calling ivy/helm-bibtex, which would make embark a hard requirement to selectrum-bibtex for now (since ivy and helm both provide mechanisms to select an action on candidates).

I decided to include my work so far for reference (once again bear with me, this was the result of around an hour of looking at ivy-bibtex sources and consult/embark READMEs):

(require 'consult)
(require 'selectrum)
(require 'bibtex-completion)

(defmacro selectrum-bibtex-selectrify-action (action name)
  "Wraps the function ACTION in another function named NAME which
extracts the key from the candidate selected in selectrum and
passes it to ACTION."
  `(defun ,name (candidate)
     (interactive "s ")
     (let ((key (cdr (assoc "=key=" (assoc candidate (bibtex-completion-candidates))))))
       (,action (list key)))))

(selectrum-bibtex-selectrify-action bibtex-completion-open-any selectrum-bibtex-open-any)
(selectrum-bibtex-selectrify-action bibtex-completion-open-pdf selectrum-bibtex-open-pdf)
(selectrum-bibtex-selectrify-action bibtex-completion-open-url-or-doi selectrum-bibtex-open-url-or-doi)
(selectrum-bibtex-selectrify-action bibtex-completion-insert-citation selectrum-bibtex-insert-citation)
(selectrum-bibtex-selectrify-action bibtex-completion-insert-reference selectrum-bibtex-insert-reference)
(selectrum-bibtex-selectrify-action bibtex-completion-insert-key selectrum-bibtex-insert-key)
(selectrum-bibtex-selectrify-action bibtex-completion-insert-bibtex selectrum-bibtex-insert-bibtex)
(selectrum-bibtex-selectrify-action bibtex-completion-add-PDF-attachment selectrum-bibtex-add-PDF-attachment)
(selectrum-bibtex-selectrify-action bibtex-completion-edit-notes selectrum-bibtex-edit-notes)
(selectrum-bibtex-selectrify-action bibtex-completion-show-entry selectrum-bibtex-show-entry)
(selectrum-bibtex-selectrify-action bibtex-completion-add-pdf-to-library selectrum-bibtex-add-pdf-to-library)

;;;###autoload
(defun selectrum-bibtex (bib-entry)
  (interactive
   (list (consult--read "Bib: " (bibtex-completion-candidates) :category 'bib)))
  ;; TODO Call selectrum-bibtex-default-action if no embark action is selected
  )

(embark-define-keymap embark-bibtex-map
  "Keymap for actions for bibtex."
  ("p" selectrum-bibtex-open-pdf)
  ("u" selectrum-bibtex-open-url-or-doi)
  ("c" selectrum-bibtex-insert-citation)
  ("r" selectrum-bibtex-insert-reference)
  ("k" selectrum-bibtex-insert-key)
  ("b" selectrum-bibtex-insert-bibtex)
  ("a" selectrum-bibtex-add-PDF-attachment)
  ("e" selectrum-bibtex-edit-notes)
  ("s" selectrum-bibtex-show-entry)
  ("l" selectrum-bibtex-add-pdf-to-library))

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

(provide 'selectrum-bibtex)
tmalsburg commented 3 years ago

Maybe it would make sense to add such a very basic frontend, based on completing-read directly to the bibtex-completion package.

I think such a front-end might be too basic to be useful. The power of helm/ivy-bibtex comes from the actions that can be performed on selected entries (opening PDF, editing notes, etc.) but with this basic front-end, there would be no way to access these actions. All this front-end would be able to do is to insert bibtex keys. But perhaps I'm misunderstanding your proposal.

minad commented 3 years ago

@mtreca My intention was not to take this away from you. I only wanted to provide some details regarding the implementation of Selectrum, Marginalia and Embark. Please continue with your efforts and ping me if you have questions.

I think such a front-end might be too basic to be useful.

@tmalsburg You are probably right. In the way bibtex-completion is designed, everything is centered around these actions, since it has been heavily influenced by Helm and Ivy.

The Embark design is a bit different, since it is designed to work with the default completion system. There every action is simply a normal command which, in this case, would select from the list of bibtex candidates. In the code by @mtreca above, selectrify turns the action functions into these commands. Then Embark glues the actions to the completing-read menu, such that you can invoke these commands directly on the selected candidate.

Example:

;; Define the bibtex commands

(defun bibtex-insert-bibtex ()
   (interactive)
   (let ((selected-item (completing-read "Insert bibtex: " ...))) ;; report completion category bib!
    ...))

(defun bibtex-open-pdf ()
   (interactive)
   (let ((selected-item (completing-read "Open PDF for bibtex: " ...))) ;; report completion category bib!
    ...))

;; Embark keymap
(embark-define-keymap embark-bibtex-map
  "Keymap for actions for bibtex."
  ("p" bibtex-open-pdf)
  ("b" bibtex-insert-bibtex)
  ...)

;; Enable the actions for the completion category bib
(add-to-list 'embark-keymap-alist '(bib . embark-bibtex-map))
tmalsburg commented 3 years ago

In the way bibtex-completion is designed, everything is centered around these actions, since it has been heavily influenced by Helm and Ivy.

I'm sure that the infrastructure in bibtex-completion could be used also for other types of completion frameworks with different UI assumptions but I think it would probably be non-trivial enough to warrant a separate package. The sketch you give is a good example and looks sensible to me.

minad commented 3 years ago

I'm sure that the infrastructure in bibtex-completion could be used also for other types of completion frameworks with different UI assumptions but I think it would probably be non-trivial enough to warrant a separate package.

Yes, the approach by @mtreca is perfectly sensible.

bdarcus commented 3 years ago

In the code by @mtreca above, selectrify turns the action functions into these commands.

While there's a disadvantage to using selectrum in this case (the added dependency), is there some advantage?

Does selectrify minimize the code vs just using embark?

minad commented 3 years ago

@bdarcus Selectrify turns the bibtex functions into proper commands which is needed for Embark. See my comment above https://github.com/tmalsburg/helm-bibtex/issues/353#issuecomment-766856371. But maybe the name should rather be embarkify ;)

Instead of adding a dependency on consult--read you can simply use completing-read in the selectrum-bibtex command, which should be called something like bibtex-completing-read. This avoids the consult dependency.

Besides that there is no Selectrum dependency. The only dependencies you will get are Marginalia and Embark. But I am not even sure about Marginalia since you may not need annotations?

Overview:

mtreca commented 3 years ago

@minad Very good points regarding limiting dependencies. Will change these in my next iteration.

bdarcus commented 3 years ago

@bdarcus Selectrify turns the bibtex functions into proper commands which is needed for Embark.

Oh right; it's a macro!

So something like this, which requires just embark and completiion-bibtex, should cover it, and be usable with selectrum.

minad commented 3 years ago

@bdarcus Yes something like this, but there is a typo in this embark-bibtex function. It should just use plain completing-read. About the embarkify actions, would it make sense to write them differently such that each of the actions is turned into a proper command, as I suggested above. This would allow invoking all the commands independently, each of the command would get the full alternative action menu.

(defun command-bibtex-open-pdf ()
   (interactive)
   (let ((selected-item (completing-read "Open PDF for bibtex: " ...))) ;; report completion category bib!
      ;;; here call the original bibtex-open-pdf function is called
   ))

EDIT: And then if you take it to the logical conclusion, all these functions could be implemented directly as commands in bibtex-completion. And the completing-read-based embark-bibtex could also go there. At this point bibtex-completion would just work out of the box with Selectrum and Emacs default completion. For actions you would only need the Embark keymaps. But obviously, it is not necessary to do this and probably you would like to keep the current design. Maybe this explains how things are put together with Embark - the idea is that you have normal commands and normal keymaps and then you enhance the existing completing-read session by making these keymaps available there.

bdarcus commented 3 years ago

Thanks @minad. I was just trying to see what the change would look like. If @mtreca agrees, he can adapt it further.

... all these functions could be implemented directly as commands in bibtex-completion.

I don't really understand this part (elisp newbie), and don't want to waste your time, but the functions are already in bibtex-completion; right (here's the open-pdf one)? Just they could, in this scenario, be tweaked to report the completion category? And if that were the case, then accessing the functions would just require the embark map?

tmalsburg commented 3 years ago

but the functions are already in bibtex-completion; right (here's the open-pdf one)?

These functions only perform actions on entries when you've already selected an entry. So they do not have any searching and selecting capabilities themselves. I don't think there is currently a way to simply add Embark to bibtex-completion to make it work stand-alone (i.e. without Ivy or Helm). Even if there was, I wouldn't want to add it because that would make Embark a dependency even for Helm-bibtex and Ivy-bibtex users who don't need it. Anything involving Selectrum, Embark, Marginalia, etc. should be a separate package that uses bibtex-completion as its backend.

minad commented 3 years ago

These functions only perform actions on entries when you've already selected an entry. So they do not have any searching and selecting capabilities themselves.

Yes. But if interactive specifications would be added the actions would work directly as commands and could be used by Embark as actions without a wrapper. This is want I tried to explain above.

For example, currently you have:

(defun bibtex-completion-open-any (keys)
  (bibtex-completion-open-pdf keys 'bibtex-completion-open-url-or-doi))

This would become:

(defun bibtex-completion-open-any (keys)
  (interactive (list (completing-read "Bibtex - open any: " (bibtex-completion-candidates) ...)))
  (bibtex-completion-open-pdf keys 'bibtex-completion-open-url-or-doi))

(This is only an example to illustrate the point, I don't know in which exact format the functions receives the KEYS)

I wouldn't want to add it because that would make Embark a dependency even for Helm-bibtex and Ivy-bibtex users who don't need it.

I am not proposing to add any new dependencies.

@bdarcus Does this also answer your question?

bdarcus commented 3 years ago

@bdarcus Does this also answer your question?

Yes; thank you!

tmalsburg commented 3 years ago

@minad, this makes sense, although I have to confess that I don't understand the benefit of this UI compared to Helm and Ivy. In helm-bitbex you can apply an action to multiple entries in one go, you can apply multiple actions on one entry, and you can apply multiple actions to multiple entries in one go. Personally I use this a lot and it saves so much time. Ivy, I think, has similar capabilities (not sure). With the UI proposed here, you could only apply one action to one entry at a time. Am I missing something? But for some users that may be enough and to each their own of course.

Just two comments on the implementation: 1. Since the proposed addition is UI, i.e. user-facing front-end code, I'd place it in a separate package (e.g., completing-read-bibtex) to keep the separation of front- and back-end, as explained above. 2. Note that bibtex-completion-candidates returns a complex data structure not just a list of strings, so you'd probably have to preprocess it in some way before passing it to completing-read. Since the completing-read needs to return a bibtex key, I suppose you'd have to pass it a list of bibtex keys, right? But how would the user identify the target entry given that most bibliographies use fairly cryptic keys?

minad commented 3 years ago

@tmalsburg

bitbex you can apply an action to multiple entries in one go, you can apply multiple actions on one entry, and you can apply multiple actions to multiple entries in one go.

Yes, that is right. With Embark you can keep the completion ui open and apply actions on the candidates you are interested in. But there is no facility to select candidates. However there is also the Embark collect buffer which allows to export the currently filtered candidates to a buffer (Similar to ivy-occur). Then you can execute actions on the candidates in this persistent buffer. Furthermore you can directly use a buffer, e.g., via the Emacs default completion. Note that I am not trying to convince you here to change your workflow from Helm or Ivy. I am just trying to explain the design of Embark/Selectrum and how it could work with your package.

Just two comments on the implementation: 1. Since the proposed addition is UI, i.e. user-facing front-end code, I'd place it in a separate package (e.g., completing-read-bibtex) to keep the separation of front- and back-end, as explained above.

Sure. This is a valid design too. From my perspective, the downside is that you need wrapper commands, which could be avoided if you add interactive specifications to all the action functions, elevating them from the function status to the status of a command.

  1. Note that bibtex-completion-candidates returns a complex data structure not just a list of strings, so you'd probably have to preprocess it in some way before passing it to completing-read.

Yes, this should be the same as with ivy-read. I didn't work out the details, I only tried to illustrate my point in my previous comment.

Since the completing-read needs to return a bibtex key, I suppose you'd have to pass it a list of bibtex keys, right?

In practice you would pass a list of formatted candidates as an alist to completing-read, '((formatted-string . key) ...). Then after completing-read you would have to do a lookup in this alist. The only requirement is that the formatted-strings are unique. This should be the case for bibtex entries. In other scenarios this is not the case. Then you have to do some additional disambiguation, but I don't want to go too much into the details here.

minad commented 3 years ago

CC @oantolin, who is the author of the Embark package. Note that I am only the coauthor of Marginalia and Consult, so maybe I cannot convey the motivation for Embark as well as Omar. Omar, maybe you are interested in this discussion too?

tmalsburg commented 3 years ago

elevating them from the function status to the status of a command.

Ah, I understand this proposal better now and do see the appeal. However, I think there are benefits to having action functions that just take bibtex keys (simple interface), but as you explained in your last paragraph, the actions would need to take something more complex than just keys, which means that the the helm and ivy front-ends would need to deal with that as well (leaky abstraction), or the action functions would need to accept both formats, plain keys and ((formatted-string . key) ...). In either case, the change to commands is perhaps not quite as elegant and minimally invasive as it appears at first. Since there's no real downside to making the completing-read approach into its own package, it's perhaps the preferable solution at least for now. But we can revisit this question once we have a solid first implementation.

minad commented 3 years ago

I agree that it is the best approach to first make this work separately as the first step! And yes, changing the API of the action functions would only make sense if it can be done in a way such that it remains compatible with Ivy and Helm. I believe it can be done in a "minimally invasive" way, but I have to look into the details. The idea would be that the action functions still only take keys (no change in the API). You call a small bibtex-completion--read-keys helper function in their interactive specifications. The bibtex-completion--read-keys would present the candidates to the user via completing-read and return the list of keys as is required by the bibtex action API. So the changes would probably sum up to one additional line per action (interactive specification) plus this small bibtex-completion--read-keys helper function. Maybe after having something externally working, the idea could be reconsidered, however it is not necessary to do that.

mtreca commented 3 years ago

I am happy that this issue started this lengthy discussion. I have a paper deadline coming up (and ivy-bibtex is helping me a lot in writing it!) but I will work on the prototype using your ideas starting next Monday.

mtreca commented 3 years ago

I just opened a draft PR so progress can be tracked on this. Feedback is more than welcome @minad @bdarcus @tmalsburg

bdarcus commented 3 years ago

For people thinking this would be cool, these ideas evolved into this independent project:

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

It's now on MELPA, and soon to be included in the Doom biblio module (to accompany helm-bibtex and ivy-bibtex) once its selectrum module is merged.

oantolin commented 3 years ago

Why wait on the merging of the selectrum module? I don't think your bibtex-actions package depends on selectrum, does it?

bdarcus commented 3 years ago

Why wait on the merging of the selectrum module? I don't think your bibtex-actions package depends on selectrum, does it?

It doesn't, but doom has this module config system where you selectively install and load packages based on module and "flag" choice.

So bottom line, the bib module package line for this will look something like this:

(when (featurep! :completion selectrum)
  (package! bibtex-actions :pin "743f548c0cd46e3418a7ca4736bde8c86f97c073"))

As in, if you select the biblio tool module and the selectrum completion module, doom will install and configure bibtex-actions, including keybindings, which-key menus, etc.

Of course, you can install and configure it manually however you want, as I do now, but this just makes it easy for the user.

Edit: I did raise a philosophical and practical question on the doom discord about whether the new module should be called selectrum, given the now multiple vertical completing-read-based completion packages, but that wasn't too popular (I can understand given the volume of support they need to do).