Org-cite: Replace basic follow-processor with transient menu?

Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

Hello!

I recently switched from org-ref to org-cite, and would like to thank
eveyone who has worked on citation handling in org-mode!
Your work is of incredible value to my research productivity!

Since I use org-roam-ref, I initially went with citar and installed
vertico, marginalia and embark, but this felt a bit invasive,
so I went back to the built in basic processors,
which fill all my needs except for the follow-processor.

To improve following, I made a transient which offers options other than
opening the bibliography entry. This works really well, and can easily
be extended by adding new suffixes.

In order to make the basic follow processor more useful, would you be
interested in replacing it with a transient menu?

As an example, I attach my transient, including examples on extensions.
It would obviously need some work on wording and thought as to what
commands should be made available by default. Also I am not used to
elisp, and the code can probably be improved!

I hope that this example demonstrates how more useful and extensible
the basic citation follower would be in form of a transient menu,
and would be happy to work this into something fit for inclusion
in org-mode, in case you would be interested.

Best regards,
Tor-björn Claesson

(transient-define-prefix tbc/follow-reference (datum &optional _)
  "How should we follow references?"
  [["Open"
    ("b" "bibliography entry"
     (lambda ()
       (interactive)
       (org-cite-basic-goto
        (car  (oref (transient-prefix-object) scope))
        (cadr (oref (transient-prefix-object) scope)))))]
   ["Copy"
    ("c" "citation key"
     (lambda ()
       (interactive)
       (kill-new
        (org-element-property :key (car (oref (transient-prefix-object) scope))))))]]
  (interactive)
  (transient-setup 'tbc/follow-reference nil nil :scope (list 
                                                         datum
                                                         _)))
(org-cite-register-processor 'tbc
  :follow #'tbc/follow-reference)
(setq org-cite-follow-processor 'tbc)

(transient-append-suffix 'tbc/follow-reference "b"
  '("p" "pdf"
    (lambda ()
      (interactive)
      (find-file-other-window
       (concat
        tbc/projektet ; path to my research files
        "Referensartiklar/"
        (org-element-property :key (car (oref (transient-prefix-object) scope)))
        ".pdf")))))

(transient-append-suffix 'tbc/follow-reference "p"
  '("n" "note"
    (lambda ()
      (interactive)
      (orb-edit-note
       (org-element-property
        :key (car  (oref (transient-prefix-object) scope)))))))

;; Adapted from org-ref
(transient-append-suffix 'tbc/follow-reference "c"
  '("d" "DOI"
     (lambda ()
       (interactive)
       (kill-new 
        (save-excursion 
          (with-temp-buffer
            (mapc #'insert-file-contents org-cite-global-bibliography)
            (bibtex-set-dialect (parsebib-find-bibtex-dialect) t)
            (bibtex-search-entry (org-element-property :key (car (oref (transient-prefix-object) scope))))
            (setq doi (bibtex-autokey-get-field "doi"))
            (replace-regexp-in-string "^http://dx.doi.org/" "" doi)))))))

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

> Since I use org-roam-ref, I initially went with citar and installed
> vertico, marginalia and embark, but this felt a bit invasive,
> so I went back to the built in basic processors,
> which fill all my needs except for the follow-processor.
>
> To improve following, I made a transient which offers options other than
> opening the bibliography entry. This works really well, and can easily
> be extended by adding new suffixes.
>
> In order to make the basic follow processor more useful, would you be
> interested in replacing it with a transient menu?

I do think that having extended menus for org-open-at-point could be
useful. Not by default, but, for example, with a prefix argument.

> As an example, I attach my transient, including examples on extensions.
> It would obviously need some work on wording and thought as to what
> commands should be made available by default. Also I am not used to
> elisp, and the code can probably be improved!

Your example demonstrates the following options:
1. Plain old opening bibtex entry
2. Copying citation key
3. Opening DOI-derived link in browser
4. Opening PDF (but I am not sure how you want to find the PDF name from
   bibtex record)

I am not sure how useful is copying the citation key, but various extra
menus like opening DOI/ISBN/URL links might be of use.
PDFs might be useful, but it is not clear how to know where such PDF is
located for arbitrary user.

Any other suggestions?
Maybe from
https://github.com/jkitchin/org-ref/blob/fd178abf12a85f8e12005d1df683564bdc534124/org-ref-citation-links.el#L525 ?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

Hi and thanks for replying!

Ihor Radchenko <yantar92@posteo.net> writes:
>
> I do think that having extended menus for org-open-at-point could be
> useful. Not by default, but, for example, with a prefix argument.
>

This is a good point, but of much larger scope than just replacing the
follower of the basic citation-processor.

>
> Your example demonstrates the following options:
> 1. Plain old opening bibtex entry
> 2. Copying citation key
> 3. Opening DOI-derived link in browser
> 4. Opening PDF (but I am not sure how you want to find the PDF name from
>    bibtex record)
>
> I am not sure how useful is copying the citation key, but various extra
> menus like opening DOI/ISBN/URL links might be of use.
> PDFs might be useful, but it is not clear how to know where such PDF is
> located for arbitrary user.
>
> Any other suggestions?
> Maybe from
> https://github.com/jkitchin/org-ref/blob/fd178abf12a85f8e12005d1df683564bdc534124/org-ref-citation-links.el#L525 ?

Notes and PDF depends heavily on user preferences - and should maybe be
left out for now? org-ref allows customizing org-ref-open-pdf-function,
with a default one using bibtex-completion-find-pdf, which finds a pdf in
bibtex-completion-library-path called citekey with one of the extension
listed in bibtex-completion-pdf-extension.

Maybe it is a good idea to start small, for example provide
1. Open bibtex-entry
2. Copy DOI
3. Opening DOI/ISBN/URL links in browser

Further functionality can easily be added per user, and good solutions
incorporated by default in the future?

I have played some more with this - would it be a good idea to include
macros to get citekey, datum and _? I would be happy to clean this up a
bit, add DOI/ISBN/URL-functionality, documentation and prepare a bug report/patch.

(defmacro org-cite-basic-follow--citekey ()
  '(org-element-property :key (car (oref (transient-prefix-object) scope))))

(defmacro org-cite-basic-follow--datum ()
  '(car  (oref (transient-prefix-object) scope)))

(defmacro org-cite-basic-follow--_ ()
  '(cadr (oref (transient-prefix-object) scope)))

(transient-define-prefix org-cite-basic-follow (datum _)
  "How should we follow references?"
  [["Open"
    ("b" "bibliography entry"
     (lambda ()
       (interactive)
       (org-cite-basic-goto
        (org-cite-basic-follow--datum)
        (org-cite-basic-follow--_))))]
   ["Copy"
    ("d" "DOI"
     (lambda ()
       (interactive)
       (kill-new
        (save-excursion
          (with-temp-buffer
            (mapc #'insert-file-contents org-cite-global-bibliography)
            (bibtex-set-dialect (parsebib-find-bibtex-dialect) t)
            (bibtex-search-entry (org-cite-basic-follow--citekey))
            (setq doi (bibtex-autokey-get-field "doi"))
            (replace-regexp-in-string "^http://dx.doi.org/" "" doi))))))]]
  (interactive)
  (transient-setup 'org-cite-basic-follow nil nil :scope (list 
                                                         datum
                                                         _)))

Best regards,
Tor-björn Claesson

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

>> I do think that having extended menus for org-open-at-point could be
>> useful. Not by default, but, for example, with a prefix argument.
>>
> This is a good point, but of much larger scope than just replacing the
> follower of the basic citation-processor.

No problem. But my idea may still be used - without prefix argument,
just move to citation record; with prefix argument - invoke the menu.

Follow processors are provided with prefix argument passed to
`org-open-at-point':

(defun org-cite-basic-goto (datum _)...

The "_" argument is the currently ignored prefix argument.

> Maybe it is a good idea to start small, for example provide
> 1. Open bibtex-entry
> 2. Copy DOI
> 3. Opening DOI/ISBN/URL links in browser
>
> Further functionality can easily be added per user, and good solutions
> incorporated by default in the future?

+1.
Ideally, these options should simply be a customization.

> I have played some more with this - would it be a good idea to include
> macros to get citekey, datum and _? I would be happy to clean this up a
> bit, add DOI/ISBN/URL-functionality, documentation and prepare a bug report/patch.
>
> (defmacro org-cite-basic-follow--citekey ()
>   '(org-element-property :key (car (oref (transient-prefix-object) scope))))
>
> (defmacro org-cite-basic-follow--datum ()
>   '(car  (oref (transient-prefix-object) scope)))
>
> (defmacro org-cite-basic-follow--_ ()
>   '(cadr (oref (transient-prefix-object) scope)))
>
> (transient-define-prefix org-cite-basic-follow (datum _)
>   "How should we follow references?"
>   [["Open"
>     ("b" "bibliography entry"
>      (lambda ()
>        (interactive)
>        (org-cite-basic-goto
>         (org-cite-basic-follow--datum)
>         (org-cite-basic-follow--_))))]

This looks way too complicated.
Is there an easier way to access transient prefix command arguments from
suffixes? Maybe something provided by transient itself?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

Ihor Radchenko <yantar92@posteo.net> writes:

> Tor-björn Claesson <tclaesson@gmail.com> writes:
>
>>> I do think that having extended menus for org-open-at-point could be
>>> useful. Not by default, but, for example, with a prefix argument.
>>>
>> This is a good point, but of much larger scope than just replacing the
>> follower of the basic citation-processor.
>
> No problem. But my idea may still be used - without prefix argument,
> just move to citation record; with prefix argument - invoke the menu.
>
> Follow processors are provided with prefix argument passed to
> `org-open-at-point':
>
> (defun org-cite-basic-goto (datum _)...
>
> The "_" argument is the currently ignored prefix argument.
>

Ah, I did not understand. Thanks for explaining! This has the nice
effect of not changing the previous behaviour. I think this should be
customizeable.

>> I have played some more with this - would it be a good idea to include
>> macros to get citekey, datum and _? I would be happy to clean this up a
>> bit, add DOI/ISBN/URL-functionality, documentation and prepare a bug report/patch.
>>
>> (defmacro org-cite-basic-follow--citekey ()
>>   '(org-element-property :key (car (oref (transient-prefix-object) scope))))
>>
>> (defmacro org-cite-basic-follow--datum ()
>>   '(car  (oref (transient-prefix-object) scope)))
>>
>> (defmacro org-cite-basic-follow--_ ()
>>   '(cadr (oref (transient-prefix-object) scope)))
>>
>> (transient-define-prefix org-cite-basic-follow (datum _)
>>   "How should we follow references?"
>>   [["Open"
>>     ("b" "bibliography entry"
>>      (lambda ()
>>        (interactive)
>>        (org-cite-basic-goto
>>         (org-cite-basic-follow--datum)
>>         (org-cite-basic-follow--_))))]
>
> This looks way too complicated.
> Is there an easier way to access transient prefix command arguments from
> suffixes? Maybe something provided by transient itself?

Yes it was way to complicated, thanks! A better way can be found reading
transient.el and magit sources. Together with your other feedback, I now have:

(defcustom org-cite-basic-follow-ask nil
  "Should org-cite-basic ask how to follow citations?"
  :group 'org-cite
  :type 'boolean)

(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
  "Follow a citation reference.

New actions can be added using transient-append-suffix. 
The body of such new actions should have the form:

(lambda (citation prefix) (interactive (oref (transient-prefix-object) scope)) ...)"
  [["Open"
    ("b" "bibliography entry" org-cite-basic-follow.open-bibliography)]
   ["Copy"
    ("d" "DOI" org-cite-basic-follow.copy-doi)]
   ["Browse"
    ("u" "url" org-cite-basic-follow.browse-url)]]
  (interactive)
  (if (or org-cite-basic-follow-ask prefix)
      (transient-setup 'org-cite-basic-follow nil nil :scope (list citation prefix))
    (org-cite-basic-goto citation prefix)))

(transient-define-suffix org-cite-basic-follow.open-bibliography (citation prefix)
  "Find bibliography entry for citation"
  (interactive (oref (transient-prefix-object) scope))
  (org-cite-basic-goto citation prefix))
  
...

And I can then for example add my own pdf-action like this:

(transient-append-suffix 'org-cite-basic-follow "b"
  '("p" "pdf"
    (lambda (citation prefix)
      (interactive (oref (transient-prefix-object) scope))
      (find-file-other-window
       (concat
        tbc/projektet
        "Referensartiklar"
        "/"
        (org-element-property :key citation)
        ".pdf")))))


Does this start to look ok, and is there something else I should
address if this was to be included in org-mode?

Cheers,
Tor-björn

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

>> Is there an easier way to access transient prefix command arguments from
>> suffixes? Maybe something provided by transient itself?
>
> Yes it was way to complicated, thanks! A better way can be found reading
> transient.el and magit sources. Together with your other feedback, I now have:
>
> (defcustom org-cite-basic-follow-ask nil
>   "Should org-cite-basic ask how to follow citations?"
>   :group 'org-cite
>   :type 'boolean)
>
> (transient-define-prefix org-cite-basic-follow (citation &optional prefix)
>   "Follow a citation reference.
>
> New actions can be added using transient-append-suffix. 
> The body of such new actions should have the form:
>
> (lambda (citation prefix) (interactive (oref (transient-prefix-object) scope)) ...)"
>   [["Open"
>     ("b" "bibliography entry" org-cite-basic-follow.open-bibliography)]
>    ["Copy"
>     ("d" "DOI" org-cite-basic-follow.copy-doi)]
>    ["Browse"
>     ("u" "url" org-cite-basic-follow.browse-url)]]
>   (interactive)
>   (if (or org-cite-basic-follow-ask prefix)
>       (transient-setup 'org-cite-basic-follow nil nil :scope (list citation prefix))
>     (org-cite-basic-goto citation prefix)))

Thanks! This looks much more clean.
Even better would be having a defcustom that defines the transient
layout. The idea is to avoid hard-coding [["Open" ... ] ["Copy" ...]
...] and instead make it defcustom.

> (transient-define-suffix org-cite-basic-follow.open-bibliography (citation prefix)
>   "Find bibliography entry for citation"
>   (interactive (oref (transient-prefix-object) scope))
>   (org-cite-basic-goto citation prefix))
>   
> ...

(oref (transient-prefix-object) scope) is equivalent to (transient-scope)

> And I can then for example add my own pdf-action like this:
>
> (transient-append-suffix 'org-cite-basic-follow "b"
>   '("p" "pdf"
>     (lambda (citation prefix)
>       (interactive (oref (transient-prefix-object) scope))
>       (find-file-other-window
>        (concat
>         tbc/projektet
>         "Referensartiklar"
>         "/"
>         (org-element-property :key citation)
>         ".pdf")))))

It feels a bit too complex to demand knowledge of these transient
details (how to get the arglist) from users.

I am wondering if we can somehow plug the existing commands passing the
arguments without any extra setup on the user side.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

Ihor Radchenko <yantar92@posteo.net> writes:
> Thanks! This looks much more clean.
> Even better would be having a defcustom that defines the transient
> layout. The idea is to avoid hard-coding [["Open" ... ] ["Copy" ...]
> ...] and instead make it defcustom.

Here is a solution that works for me. Is this an OK use of eval, or is
there a better way of doing this?

(defcustom org-cite-basic-follow-actions
  '[["Open"
    ("b" "bibliography entry" org-cite-basic-follow.open-bibliography)]
   ["Copy"
    ("d" "DOI" org-cite-basic-follow.copy-doi)]
   ["Browse"
    ("u" "url" org-cite-basic-follow.browse-url)]]
  "Contents of the org-cite-basic-follow transient menu. 

This can be customized directly using the customization 
interface. Use setopt instead of setq if you change this option 
in elisp, to ensure that the transient is rebuilt.

Further  actions can be added using transient-define-suffix."
  :group 'org-cite
  :type 'sexp
  :set (lambda (option-name new-value)
         (eval
          `(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
             "Follow a citation reference.

The contents of this transient menu is set in org-cite-basic-follow-actions."
             ,new-value
             (interactive)
             (if (or org-cite-basic-follow-ask prefix)
                 (transient-setup 'org-cite-basic-follow nil nil :scope (list citation prefix))
               (org-cite-basic-goto citation prefix))))
         (set-default-toplevel-value option-name new-value)))

>> (transient-define-suffix org-cite-basic-follow.open-bibliography (citation prefix)
>>   "Find bibliography entry for citation"
>>   (interactive (oref (transient-prefix-object) scope))
>>   (org-cite-basic-goto citation prefix))
>>   
>> ...
>
> (oref (transient-prefix-object) scope) is equivalent to (transient-scope)
>

Ah, thanks!

>> And I can then for example add my own pdf-action like this:
>>
>> (transient-append-suffix 'org-cite-basic-follow "b"
>>   '("p" "pdf"
>>     (lambda (citation prefix)
>>       (interactive (oref (transient-prefix-object) scope))
>>       (find-file-other-window
>>        (concat
>>         tbc/projektet
>>         "Referensartiklar"
>>         "/"
>>         (org-element-property :key citation)
>>         ".pdf")))))
>
> It feels a bit too complex to demand knowledge of these transient
> details (how to get the arglist) from users.
>
> I am wondering if we can somehow plug the existing commands passing the
> arguments without any extra setup on the user side.

The lambda form is much neater with your (transient-scope) suggestion:
(lambda (citation prefix)
        (interactive (transient-scope))
        ...)

Is this simple enough? I don't feel a macro would improve the
situation.

Best regards,
Tor-björn

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

[ CCing emacs-devel and the author of transient; maybe we can have some
  more suggestions this way ]

For some context, we are trying to create a customizeable transient menu
with items configured via user option.

We are also trying to pass additional arguments from prefix to suffix
commands in a way that there is no need to write suffix commands
specially just for transient.

Tor-björn Claesson <tclaesson@gmail.com> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>> Thanks! This looks much more clean.
>> Even better would be having a defcustom that defines the transient
>> layout. The idea is to avoid hard-coding [["Open" ... ] ["Copy" ...]
>> ...] and instead make it defcustom.
>
> Here is a solution that works for me. Is this an OK use of eval, or is
> there a better way of doing this?
>
> (defcustom org-cite-basic-follow-actions
>   '[["Open"
>     ("b" "bibliography entry" org-cite-basic-follow.open-bibliography)]
>    ["Copy"
>     ("d" "DOI" org-cite-basic-follow.copy-doi)]
>    ["Browse"
>     ("u" "url" org-cite-basic-follow.browse-url)]]
>   "Contents of the org-cite-basic-follow transient menu. 
>
> This can be customized directly using the customization 
> interface. Use setopt instead of setq if you change this option 
> in elisp, to ensure that the transient is rebuilt.

+1

> Further  actions can be added using transient-define-suffix."
>   :group 'org-cite
>   :type 'sexp
>   :set (lambda (option-name new-value)
>          (eval
>           `(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
>              "Follow a citation reference.
>
> The contents of this transient menu is set in org-cite-basic-follow-actions."
>              ,new-value
>              (interactive)
>              (if (or org-cite-basic-follow-ask prefix)
>                  (transient-setup 'org-cite-basic-follow nil nil :scope (list citation prefix))
>                (org-cite-basic-goto citation prefix))))
>          (set-default-toplevel-value option-name new-value)))

This should work, but maybe Jonas can provide better ideas.

>>> And I can then for example add my own pdf-action like this:
>>>
>>> (transient-append-suffix 'org-cite-basic-follow "b"
>>>   '("p" "pdf"
>>>     (lambda (citation prefix)
>>>       (interactive (oref (transient-prefix-object) scope))
>>>       (find-file-other-window
>>>        (concat
>>>         tbc/projektet
>>>         "Referensartiklar"
>>>         "/"
>>>         (org-element-property :key citation)
>>>         ".pdf")))))
>>
>> It feels a bit too complex to demand knowledge of these transient
>> details (how to get the arglist) from users.
>>
>> I am wondering if we can somehow plug the existing commands passing the
>> arguments without any extra setup on the user side.
>
> The lambda form is much neater with your (transient-scope) suggestion:
> (lambda (citation prefix)
>         (interactive (transient-scope))
>         ...)
>
> Is this simple enough? I don't feel a macro would improve the
> situation.

It is not too bad, but what I really wanted is to reuse an existing
command/function without having to write a tailored interactive
statement. Again, I am hoping to get some insight from emacs-devel.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Jonas Bernoulli]

> (defcustom org-cite-basic-follow-actions
>   '[["Open"
>     ("b" "bibliography entry" org-cite-basic-follow.open-bibliography)]
>    ["Copy"
>     ("d" "DOI" org-cite-basic-follow.copy-doi)]
>    ["Browse"
>     ("u" "url" org-cite-basic-follow.browse-url)]]
>   "Contents of the org-cite-basic-follow transient menu. 
>
> This can be customized directly using the customization 
> interface. Use setopt instead of setq if you change this option 
> in elisp, to ensure that the transient is rebuilt.
>
> Further  actions can be added using transient-define-suffix."
>   :group 'org-cite
>   :type 'sexp
>   :set (lambda (option-name new-value)
>          (eval
>           `(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
>              "Follow a citation reference.
>
> The contents of this transient menu is set in org-cite-basic-follow-actions."
>              ,new-value
>              (interactive)
>              (if (or org-cite-basic-follow-ask prefix)
>                  (transient-setup 'org-cite-basic-follow nil nil :scope (list citation prefix))
>                (org-cite-basic-goto citation prefix))))
>          (set-default-toplevel-value option-name new-value)))

(defcustom org-cite-basic-follow-actions
  '[["Open"
    ("b" "bibliography entry" org-cite-basic-follow.open-bibliography)]
    ["Copy"
     ("d" "DOI" org-cite-basic-follow.copy-doi)]
    ["Browse"
     ("u" "url" org-cite-basic-follow.browse-url)]]
  ...)

(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
  [org-cite-basic-follow-actions]
  (interactive)
  (if (or org-cite-basic-follow-ask prefix)
      (transient-setup 'org-cite-basic-follow nil nil
                       ;; (off-topic) Add \n here ---^
                       :scope (list citation prefix))
    (org-cite-basic-goto citation prefix))))


I use something similar in Forge (forge--lists-group et al.), but
there the purpose is to share groups between different prefixes, not
to make them customizable.

To let users choose what commands to offer in the menu, I would
recommend directing users towards Transient's "layer" mechanism instead
of adding an option.

See [[info:transient#Enabling and Disabling Suffixes]].  To try it enter
any prefix (magit-diff would do) and type "C-x l".  Usage information is
displayed after that.

> :scope (list citation prefix)

Shouldn't that be just be
  :scope citation
and then
  (interactive (list (magit-scope)))
to access it?

>> It feels a bit too complex to demand knowledge of these transient
>> details (how to get the arglist) from users.
>>
>> I am wondering if we can somehow plug the existing commands passing the
>> arguments without any extra setup on the user side.
>
> The lambda form is much neater with your (transient-scope) suggestion:
> (lambda (citation prefix)
>         (interactive (transient-scope))
>         ...)
>
> Is this simple enough? I don't feel a macro would improve the
> situation.

Yes, obviously you have to call transient-scope somewhere.

I haven't seen enough of the commands you want to add as suffixes to
know whether it would make sense, and is even possible, to add an
abstraction, and the few examples I have seen already contain
non-commands and "find as pdf" doesn't even exist as a named function.

That being said, if there are multiple commands like

  (defun do-something-with-citation (citation)
    (interactive (list (read-citation "Do something with citation: ")))
    ...)

it might make sense to change read-citation like this

  (defun read-citation (prompt)
    (if (eq transient-current-prefix 'org-cite-basic-follow)
        (org-element-property :key (transient-scope))
      ...old body here...))

Cheers,
Jonas

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Jonas Bernoulli <jonas@bernoul.li> writes:

>>   :set (lambda (option-name new-value)
>>          (eval
>>           `(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
>>              "Follow a citation reference.
> ...
>
> (transient-define-prefix org-cite-basic-follow (citation &optional prefix)
>   [org-cite-basic-follow-actions]
> ...

This is more compact indeed. Thanks!

> I use something similar in Forge (forge--lists-group et al.), but
> there the purpose is to share groups between different prefixes, not
> to make them customizable.

And this is the problem. See the above :set we have to add in order to
re-evaluate the prefix definition.

It would be nice if the layout could be calculated dynamically rather
than frozen in place in the "defun".

> To let users choose what commands to offer in the menu, I would
> recommend directing users towards Transient's "layer" mechanism instead
> of adding an option.
>
> See [[info:transient#Enabling and Disabling Suffixes]].  To try it enter
> any prefix (magit-diff would do) and type "C-x l".  Usage information is
> displayed after that.

This will indeed help with customizing *pre-existing suffixes*. However,
what we really want here is different - we want a minimal set of
suffixes to be provided by Org mode, and more suffixes to be contributed
by individual packages or by users themselves.

In other words, we need some way to add new suffixes to the
org-cite-basic-follow prefix menu.

>> :scope (list citation prefix)
>
> Shouldn't that be just be
>   :scope citation
> and then
>   (interactive (list (magit-scope)))
> to access it?

We want the [suffix] commands to have information about the prefix argument used
to call `org-cite-basic-follow'.

> Yes, obviously you have to call transient-scope somewhere.
>
> I haven't seen enough of the commands you want to add as suffixes to
> know whether it would make sense, and is even possible, to add an
> abstraction, and the few examples I have seen already contain
> non-commands and "find as pdf" doesn't even exist as a named function.
>
> That being said, if there are multiple commands like
>
>   (defun do-something-with-citation (citation)
>     (interactive (list (read-citation "Do something with citation: ")))
>     ...)
>
> it might make sense to change read-citation like this
>
>   (defun read-citation (prompt)
>     (if (eq transient-current-prefix 'org-cite-basic-follow)
>         (org-element-property :key (transient-scope))
>       ...old body here...))

This is precisely what I want to avoid.

What we want is having normal commands/functions and then allowing to
add them as suffixes without having to change their interactive spec or
source code in general.

Currently, if we want suffix that is calling a function not specially
designed with transient support in mind, we need to do the ugly juggle
like

    (transient-append-suffix 'org-cite-basic-follow "b"
      '("g" "goto"
	(lambda (citation prefix)
	  (interactive (transient-scope))
          (org-cite-basic-goto citation prefix))))

It would be so much nicer to write something simpler like

(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
		   "Follow a citation reference.
      The contents of this transient menu is set in org-cite-basic-follow-actions."
		   [["Open"
                     ("b" "bibliography entry" org-cite-basic-goto :args (transient-args))]]
		   (interactive)
		   (if (or org-cite-basic-follow-ask prefix)
                       ;; Imaginary :args slot
		       (transient-setup 'org-cite-basic-follow nil nil :args (list citation prefix))
		     (org-cite-basic-goto citation prefix)))

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Jonas Bernoulli]

Ihor Radchenko <yantar92@posteo.net> writes:

> Jonas Bernoulli <jonas@bernoul.li> writes:
>
>>>   :set (lambda (option-name new-value)
>>>          (eval
>>>           `(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
>>>              "Follow a citation reference.
>> ...
>>
>> (transient-define-prefix org-cite-basic-follow (citation &optional prefix)
>>   [org-cite-basic-follow-actions]
>> ...
>
> This is more compact indeed. Thanks!
>
>> I use something similar in Forge (forge--lists-group et al.), but
>> there the purpose is to share groups between different prefixes, not
>> to make them customizable.
>
> And this is the problem. See the above :set we have to add in order to
> re-evaluate the prefix definition.
>
> It would be nice if the layout could be calculated dynamically rather
> than frozen in place in the "defun".

Look at notmuch-tag-transient from notmuch-transient.el [1].  It does
exactly that.  I.e., the :setup-children slot is what you are looking
for.

[1] https://github.com/tarsius/notmuch-transient

>> To let users choose what commands to offer in the menu, I would
>> recommend directing users towards Transient's "layer" mechanism instead
>> of adding an option.
>>
>> See [[info:transient#Enabling and Disabling Suffixes]].  To try it enter
>> any prefix (magit-diff would do) and type "C-x l".  Usage information is
>> displayed after that.
>
> This will indeed help with customizing *pre-existing suffixes*. However,
> what we really want here is different - we want a minimal set of
> suffixes to be provided by Org mode, and more suffixes to be contributed
> by individual packages or by users themselves.
>
> In other words, we need some way to add new suffixes to the
> org-cite-basic-follow prefix menu.

The recommended way of adding bindings is transient-insert-suffix, but
that won't work here because of the "we need to wrap the command" issue.

>>> :scope (list citation prefix)
>>
>> Shouldn't that be just be
>>   :scope citation
>> and then
>>   (interactive (list (magit-scope)))
>> to access it?
>
> We want the [suffix] commands to have information about the prefix argument used
> to call `org-cite-basic-follow'.

I asked because none of the provided examples actually used that
information (or maybe I just overlooked that).  But sure, if that
information is needed, then that's the way to provide it.

>> Yes, obviously you have to call transient-scope somewhere.
>>
>> I haven't seen enough of the commands you want to add as suffixes to
>> know whether it would make sense, and is even possible, to add an
>> abstraction, and the few examples I have seen already contain
>> non-commands and "find as pdf" doesn't even exist as a named function.
>>
>> That being said, if there are multiple commands like
>>
>>   (defun do-something-with-citation (citation)
>>     (interactive (list (read-citation "Do something with citation: ")))
>>     ...)
>>
>> it might make sense to change read-citation like this
>>
>>   (defun read-citation (prompt)
>>     (if (eq transient-current-prefix 'org-cite-basic-follow)
>>         (org-element-property :key (transient-scope))
>>       ...old body here...))
>
> This is precisely what I want to avoid.

I don't think I got my point across.

I am not saying that you should modify the interactive form of every
command, or each command's individual reader function.

Instead I am saying that IFF "all" of these commands already happen to
use the same reader function, then it would make sense to trivially
address the issue by adding the following two lines to that reader, and
be done with it.

 (defun read-citation (prompt)
+  (if (eq transient-current-prefix 'org-cite-basic-follow)
+      (org-element-property :key (transient-scope)
     ...old body here...))

But that doesn't seem to be the case, so...

> What we want is having normal commands/functions and then allowing to
> add them as suffixes without having to change their interactive spec or
> source code in general.
>
> Currently, if we want suffix that is calling a function not specially
> designed with transient support in mind, we need to do the ugly juggle
> like

(That code was unreadable in my mua, so I am trimming it to the relevant
part.)

> [["Open"
>  ("b" "bibliography entry" org-cite-basic-goto
>   :args (transient-args))]]

I was planning to suggest something vaguely along those lines, after
confirming that the "slightly modify the universally used reader"
approach above, turns out to not be applicable.

Adding a new slot for that makes sense but would need a new class.  See
the `forge--topic-set-state-command' class and the commands that use it,
for an example where I felt that approach makes sense.

But the approach I used for `notmuch-tag-transient' is more applicable
here.  Hm, that actually also adds a new class, which we *might* be able
to avoid here, let's look at the simpler `notmuch-search-transient'
for inspiration.

[Obviously completely untested:]

(defcustom org-cite-basic-follow-actions
  '[["Open"
     ("b" "bibliography entry" org-cite-basic-follow.open-bibliography (...))]
    ["Copy"
     ("d" "DOI" org-cite-basic-follow.copy-doi (...))]
    ["Browse"
     ("u" "url" org-cite-basic-follow.browse-url (...))]]
  ...)

(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
  [:class transient-column
   :setup-children org-cite-basic-follow--setup
   :pad-keys t]
  (interactive)
  (if (or org-cite-basic-follow-ask prefix)
      (transient-setup 'org-cite-basic-follow nil nil
                       :scope (list citation prefix))
    (org-cite-basic-goto citation prefix)))

(defun org-cite-basic-follow--setup (_)
  (transient-parse-suffixes
   'notmuch-search-transient
   (mapcar (pcase-lambda (`(,key ,desc ,fn ,transform))
               (list ,key ,desc
                     (lambda ()
                       (interactive)
                       (apply fn (eval transform)))))
           org-cite-basic-follow-actions)))

     Cheers,
     Jonas

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Jonas Bernoulli <jonas@bernoul.li> writes:

> ...
> But the approach I used for `notmuch-tag-transient' is more applicable
> here.  Hm, that actually also adds a new class, which we *might* be able
> to avoid here, let's look at the simpler `notmuch-search-transient'
> for inspiration.
>
> [Obviously completely untested:]
> ...
> (transient-define-prefix org-cite-basic-follow (citation &optional prefix)
>   [:class transient-column
>    :setup-children org-cite-basic-follow--setup

Thanks!
This seems to be good enough approach.
Tor-björn, does the proposed idea make sense to you?

> (That code was unreadable in my mua, so I am trimming it to the relevant
> part.)

Side node: If your mua is able to display raw message as plain text, it
will have the original formatting without newlines being removed/added.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

Hi and thanks for these suggestions! I have some time away from the family
tomorrow evening, and will try to make a new prototype along those lines:)

Getting back!

Cheers/Tor-björn

Den sön 27 okt. 2024 10.07Ihor Radchenko <yantar92@posteo.net> skrev:

> Jonas Bernoulli <jonas@bernoul.li> writes:
>
> > ...
> > But the approach I used for `notmuch-tag-transient' is more applicable
> > here.  Hm, that actually also adds a new class, which we *might* be able
> > to avoid here, let's look at the simpler `notmuch-search-transient'
> > for inspiration.
> >
> > [Obviously completely untested:]
> > ...
> > (transient-define-prefix org-cite-basic-follow (citation &optional
> prefix)
> >   [:class transient-column
> >    :setup-children org-cite-basic-follow--setup
>
> Thanks!
> This seems to be good enough approach.
> Tor-björn, does the proposed idea make sense to you?
>

[-- Attachment #2: Type: text/html, Size: 1471 bytes --]

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

Hi again!

Den lör 26 okt. 2024 kl 14:45 skrev Jonas Bernoulli <jonas@bernoul.li>:
> [Obviously completely untested:]
>
> (defcustom org-cite-basic-follow-actions
>   '[["Open"
>      ("b" "bibliography entry" org-cite-basic-follow.open-bibliography (...))]
>     ["Copy"
>      ("d" "DOI" org-cite-basic-follow.copy-doi (...))]
>     ["Browse"
>      ("u" "url" org-cite-basic-follow.browse-url (...))]]
>   ...)
>
> (transient-define-prefix org-cite-basic-follow (citation &optional prefix)
>   [:class transient-column
>    :setup-children org-cite-basic-follow--setup
>    :pad-keys t]
>   (interactive)
>   (if (or org-cite-basic-follow-ask prefix)
>       (transient-setup 'org-cite-basic-follow nil nil
>                        :scope (list citation prefix))
>     (org-cite-basic-goto citation prefix)))
>
> (defun org-cite-basic-follow--setup (_)
>   (transient-parse-suffixes
>    'notmuch-search-transient
>    (mapcar (pcase-lambda (`(,key ,desc ,fn ,transform))
>                (list ,key ,desc
>                      (lambda ()
>                        (interactive)
>                        (apply fn (eval transform)))))
>            org-cite-basic-follow-actions)))
>
>      Cheers,
>      Jonas

I didn't get this working yesterday, but it looks really nice from my
non-technical user point of view! Thanks!

I will keep trying, but I must find the spare time to learn more about
mapping and pattern matching in elisp, so this might take a while. In
case Ihor wants to just fix it, please go ahead :-)

It would be good if we could match against the case of '(key desc
suffix) as well, so that we could include otherwhere defined suffixes.

Cheers,
Tor-björn

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

> ...
>>    (mapcar (pcase-lambda (`(,key ,desc ,fn ,transform))
>>                (list ,key ,desc
>>                      (lambda ()
>>                        (interactive)
>>                        (apply fn (eval transform)))))
>>            org-cite-basic-follow-actions)))
>
> ...
> I will keep trying, but I must find the spare time to learn more about
> mapping and pattern matching in elisp, so this might take a while. In
> case Ihor wants to just fix it, please go ahead :-)

Check "11.4.4 Destructuring with ‘pcase’ Patterns" section of Elisp
manual. It has examples explaining how backquote pattern works.

> It would be good if we could match against the case of '(key desc
> suffix) as well, so that we could include otherwhere defined suffixes.

To not complicate things, I suggest something simple along the lines of

(lambda (&rest suffix-spec)
 (pcase suffix-spec
  (`(,key ,desc ,fn ,transform) ...)
  (`(,key ,desc ,suffix) ...)))

Remember that you can always play around with things in scratch buffer,
in IELM, or even just using C-x C-e from anywhere:

(pcase '(1 2 3 4)
  (`(,key ,desc ,fn ,transform)
    (format "key: %s; desc: %s; fn:%s; transform: %s"
       key  desc fn transform))
  (`(,key ,desc ,suffix)
    (format "key: %s; desc: %s; suffix:%s"
       key desc suffix)))
;; => "key: 1; desc: 2; fn:3; transform: 4"

(pcase '(1 2 3)
  (`(,key ,desc ,fn ,transform)
    (format "key: %s; desc: %s; fn:%s; transform: %s"
       key  desc fn transform))
  (`(,key ,desc ,suffix)
    (format "key: %s; desc: %s; suffix:%s"
       key desc suffix)))
;; => "key: 1; desc: 2; suffix:3"

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

This is a great learning experience, thank you Ihor and Jonas for the
friendly feedback!

(defcustom org-cite-basic-follow-actions
  '[["Open"
     ("b" "bibliography entry" org-cite-basic-goto (list (transient-scope) 0))]    
    ["Copy"
     ("d" "DOI" org-cite-basic-follow.copy-doi)]
    ["Browse"
     ("u" "url" org-cite-basic-follow.browse-url)]]
  "Hepp"
  :group 'org-cite
  :type 'sexp)

The bibliography entry demonstrates using any function (a bit ugly since
org-cite-basic-goto insists on the prefix argument).

(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
  [:class transient-columns
   :setup-children org-cite-basic-follow--setup
   :pad-keys t]
  (interactive)
  (if org-cite-basic-follow-ask
      (transient-setup 'org-cite-basic-follow nil nil
                       :scope (list citation))
    (org-cite-basic-goto citation prefix)))

Here I removed activating the transient menu if a prefix argument is
present. If org-cite-basic-follow-ask is nil (the default) and I want
to call org-cite-basic-goto with a prefix argument, this would cause the
transient menu to activate, which seems wrong.

(defun org-cite-basic-follow--setup (_)
  (transient-parse-suffixes
   'org-cite-basic-follow
   (cl-map 'vector (lambda (group)
          (cl-map 'vector (lambda (entry)
                    (pcase entry
                      ((and (pred stringp) label)
                       label)
                      (`(,key ,desc ,fn ,transform)
                       (list key desc `(lambda ()
                                         (interactive)
                                         (apply ',fn ,transform))))
                      (`(,key ,desc ,suffix)
                       (list key desc suffix))))
                  group))
        org-cite-basic-follow-actions)))

Prefixes of :class transient-columns expect a vector of vectors of a string and
lists, so we need to use a map that produces vectors.

This works. Do the code look OK to you? I'll prepare a patch if that is the
case.

Cheers,
Tor-björn

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

> (defcustom org-cite-basic-follow-actions
>   '[["Open"
>      ("b" "bibliography entry" org-cite-basic-goto (list (transient-scope) 0))]    
>     ["Copy"
>      ("d" "DOI" org-cite-basic-follow.copy-doi)]
>     ["Browse"
>      ("u" "url" org-cite-basic-follow.browse-url)]]
>   "Hepp"
>   :group 'org-cite
>   :type 'sexp)

Since we can now allow pretty much a custom suffix definition, what
about adding a bit of syntax sugar to the format.

Rather than doing

   ("b" "bibliography entry" org-cite-basic-goto (list (transient-scope) 0))

we can allow

   ("b" "bibliography entry" (org-cite-basic-goto !citation !prefix))

where !citation and !prefix can be pattern-matched and automatically
replaced with the appropriate values.

> (transient-define-prefix org-cite-basic-follow (citation &optional prefix)
>   [:class transient-columns
>    :setup-children org-cite-basic-follow--setup
>    :pad-keys t]
>   (interactive)
>   (if org-cite-basic-follow-ask
>       (transient-setup 'org-cite-basic-follow nil nil
>                        :scope (list citation))
>     (org-cite-basic-goto citation prefix)))
>
> Here I removed activating the transient menu if a prefix argument is
> present. If org-cite-basic-follow-ask is nil (the default) and I want
> to call org-cite-basic-goto with a prefix argument, this would cause the
> transient menu to activate, which seems wrong.

I would still prefer some way to force transient version of the command
in some way. I see it very useful for some people to set the proposed
`org-cite-basic-follow-ask' to nil _most of the time_, but occasionally
switch to transient version for non-standard operations.

Maybe we can utilize some other rarely used prefix argument to force
transient menu.

Say, something like C-- C-u <key sequence>. This will make prefix
argument have a value of '(-4).

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

Good ideas!

(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
  [:class transient-columns
   :setup-children org-cite-basic-follow--setup
   :pad-keys t]
  (interactive)
  (if (or org-cite-basic-follow-ask
          (eq prefix '(-4)))
      (transient-setup 'org-cite-basic-follow nil nil
                       :scope (list citation prefix))
    (org-cite-basic-goto citation prefix)))

This works nicely!

(defun org-cite-basic-follow--setup (_)
  (transient-parse-suffixes
   'org-cite-basic-follow
   (cl-map 'vector (lambda (group)
          (cl-map 'vector (lambda (suffix-spec)
                    (pcase suffix-spec
                      ((and (pred stringp) label)
                       label)
                      (`(,key ,desc (,fn !citation !prefix))
                       (list key desc `(lambda ()
                                         (interactive)
                                         (letrec ((scope (transient-scope))
                                                  (citation (car scope))
                                                  (prefix (cadr scope)))
                                             (,fn citation prefix)))))
                      (`(,key ,desc ,fn ,transform)
                       (list key desc `(lambda ()
                                         (interactive)
                                         (apply ',fn ,transform))))
                      (`(,key ,desc ,suffix)
                       (list key desc suffix))))
                  group))
        org-cite-basic-follow-actions)))

This too works really well! I think it would make sence to match (,fn
!citation), (,fn !citation-key) and (,fn !citation-key !prefix), does
this sound reasonable?

Thanks again for taking the time to help me with this=)

Cheers,
Tor-björn

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

> (defun org-cite-basic-follow--setup (_)
>   (transient-parse-suffixes
>    'org-cite-basic-follow
>    (cl-map 'vector (lambda (group)
>           (cl-map 'vector (lambda (suffix-spec)

This is getting complex enough that you can split out this lambda into a
dedicated private function.

>                     (pcase suffix-spec
>                       ((and (pred stringp) label)
>                        label)
>                       (`(,key ,desc (,fn !citation !prefix))

I think we can do yet better:

  `(,key ,desc (,fn . ,fn-args) . ,other)

Here, `(,a . ,b) is matching a list with 1+ elements, with b
collecting the cdr of the list.

Once we have fn-args, we can simply map over it and search for
'!citation or '!prefix symbols, replacing them with appropriate
values.

The final return value will be (using backquote-style list syntax:
10.4 Backquote in Elisp manual):

  `(,key ,desc (,fn ,@processed-args) ,@other)

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

Thanks!

Here is another take=)

(defcustom org-cite-basic-follow-actions
  '[["Open"
     ("b" "bibliography entry" (org-cite-basic-goto !citation !prefix))]    
    ["Copy"
     ("d" "DOI" org-cite-basic-follow.copy-doi)]
    ["Browse"
     ("u" "url" org-cite-basic-follow.browse-url)]]
  "Hepp"
  :group 'org-cite
  :type 'sexp)

(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
  [:class transient-columns
          :setup-children org-cite-basic-follow--setup
          :pad-keys t]
  (interactive)
  (if (or org-cite-basic-follow-ask
          (eq prefix '(-4)))
      (transient-setup 'org-cite-basic-follow nil nil
                       :scope (list citation prefix))
    (org-cite-basic-goto citation prefix)))

(defun org-cite-basic-follow--parse-suffix-specification (specification)
  (pcase specification
    ((and (pred stringp) label)
     label)
    (`(,key ,desc (,fn . ,fn-args) . ,other)
     (let ((function-args
            (mapcar
             (lambda (arg)
               (pcase arg
                 ('!citation
                  '(car (transient-scope)))
                 ('!prefix
                  '(cadr (transient-scope)))
                 ('!citation-key
                  '(org-element-property :key (car (transient-scope))))))
             fn-args)))
       `(,key ,desc
              (lambda ()
                (interactive)
                (,fn ,@function-args))
              ,other)))
    (`(,key ,desc ,suffix)
     (list key desc suffix))))

(defun org-cite-basic-follow--setup (_)
  (transient-parse-suffixes
   'org-cite-basic-follow
   (cl-map 'vector
           (lambda (group)
             (cl-map 'vector
                     #'org-cite-basic-follow--parse-suffix-specification
                     group))
           org-cite-basic-follow-actions)))

Cheers,
Tor-björn

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

But that was not good enough - we do not cover the case of specifying
a lambda in org-cite-basic-follow-actions,
or passing other arguments to the function than citation, prefix or
citation key.

This updated version fixes this, so the action can be either
1. a suffix (as in transient-define-suffix)
2. a lambda form (as in (lambda (citation prefix) (interactive
(transient-scope)) ...))
3. a function call, which will be wrapped in the ugly dance-lambda and
where !citation, !prefix, and !citation-key
    will be (recursively) substituted but other arguments preserved.

(defun org-cite-basic-follow--process-function-arguments (arguments)
  (cond ((null arguments)
         '())
        ((atom (car arguments))
         (cons
          (pcase (car arguments)
            ('!citation
             '(car (transient-scope)))
            ('!prefix
             '(cadr (transient-scope)))
            ('!citation-key
             '(org-element-property :key (car (transient-scope))))
            (argument
             argument))
          (org-cite-basic-follow--process-function-arguments (cdr arguments))))
        (t
         (cons
          (org-cite-basic-follow--process-function-arguments (car arguments))
          (org-cite-basic-follow--process-function-arguments (cdr
arguments))))))

(defun org-cite-basic-follow--parse-suffix-specification (specification)
  (pcase specification
    ((and (pred stringp) label)
     label)
    (`(,key ,desc (lambda . ,fn-args) . ,other)
     (list key desc `(lambda ,@fn-args) ,other))
    (`(,key ,desc (,fn . ,fn-args) . ,other)
     (let ((function-args
            (org-cite-basic-follow--process-function-arguments
             fn-args)))
       `(,key ,desc
              (lambda ()
                (interactive)
                (,fn ,@function-args))
              ,other)))
    (`(,key ,desc ,suffix)
     (list key desc suffix))))

(defun org-cite-basic-follow--setup (_)
  (transient-parse-suffixes
   'org-cite-basic-follow
   (cl-map 'vector
           (lambda (group)
             (cl-map 'vector #'org-cite-basic-follow--parse-suffix-specification
                     group))
           org-cite-basic-follow-actions)))

Cheers!
Tor-björn

Den tors 31 okt. 2024 kl 22:48 skrev Tor-björn Claesson <tclaesson@gmail.com>:
>
> Thanks!
>
> Here is another take=)
>
> (defcustom org-cite-basic-follow-actions
>   '[["Open"
>      ("b" "bibliography entry" (org-cite-basic-goto !citation !prefix))]
>     ["Copy"
>      ("d" "DOI" org-cite-basic-follow.copy-doi)]
>     ["Browse"
>      ("u" "url" org-cite-basic-follow.browse-url)]]
>   "Hepp"
>   :group 'org-cite
>   :type 'sexp)
>
> (transient-define-prefix org-cite-basic-follow (citation &optional prefix)
>   [:class transient-columns
>           :setup-children org-cite-basic-follow--setup
>           :pad-keys t]
>   (interactive)
>   (if (or org-cite-basic-follow-ask
>           (eq prefix '(-4)))
>       (transient-setup 'org-cite-basic-follow nil nil
>                        :scope (list citation prefix))
>     (org-cite-basic-goto citation prefix)))
>
> (defun org-cite-basic-follow--parse-suffix-specification (specification)
>   (pcase specification
>     ((and (pred stringp) label)
>      label)
>     (`(,key ,desc (,fn . ,fn-args) . ,other)
>      (let ((function-args
>             (mapcar
>              (lambda (arg)
>                (pcase arg
>                  ('!citation
>                   '(car (transient-scope)))
>                  ('!prefix
>                   '(cadr (transient-scope)))
>                  ('!citation-key
>                   '(org-element-property :key (car (transient-scope))))))
>              fn-args)))
>        `(,key ,desc
>               (lambda ()
>                 (interactive)
>                 (,fn ,@function-args))
>               ,other)))
>     (`(,key ,desc ,suffix)
>      (list key desc suffix))))
>
> (defun org-cite-basic-follow--setup (_)
>   (transient-parse-suffixes
>    'org-cite-basic-follow
>    (cl-map 'vector
>            (lambda (group)
>              (cl-map 'vector
>                      #'org-cite-basic-follow--parse-suffix-specification
>                      group))
>            org-cite-basic-follow-actions)))
>
> Cheers,
> Tor-björn

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

> But that was not good enough - we do not cover the case of specifying
> a lambda in org-cite-basic-follow-actions,
> or passing other arguments to the function than citation, prefix or
> citation key.
>
> This updated version fixes this, so the action can be either
> 1. a suffix (as in transient-define-suffix)
> 2. a lambda form (as in (lambda (citation prefix) (interactive
> (transient-scope)) ...))
> 3. a function call, which will be wrapped in the ugly dance-lambda and
> where !citation, !prefix, and !citation-key
>     will be (recursively) substituted but other arguments preserved.

Rather than going into recursive replacements, simply let-bind
!citation, !prefix, and anything else we may want to provide around the
lambda/function call.

>   (pcase specification
>     ((and (pred stringp) label)
>      label)
>     (`(,key ,desc (lambda . ,fn-args) . ,other)
>      (list key desc `(lambda ,@fn-args) ,other))
> ...
>     (`(,key ,desc ,suffix)
>      (list key desc suffix))))

All these special cases can be simply handled using

 (other other) ; if not a pattern we care about, just retain it unchanged

pcase clause.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

Ihor Radchenko <yantar92@posteo.net> writes:
> Rather than going into recursive replacements, simply let-bind
> !citation, !prefix, and anything else we may want to provide around the
> lambda/function call.

Clever! I had to put the let inside the lambda for it to work.

(defun org-cite-basic-follow--parse-suffix-specification (specification)
  (pcase specification
    (`(,key ,desc (lambda . ,fn-args) . ,other)
     (list key desc `(lambda ,@fn-args) ,other))
    (`(,key ,desc (,fn . ,fn-args) . ,other)
     `(,key ,desc
            (lambda ()
              (interactive)
              (let ((!citation (car (transient-scope)))
                    (!prefix (cadr (transient-scope)))
                    (!citation-key (org-element-property :key (car (transient-scope)))))
                (,fn ,@fn-args)))
            ,other))
    (other other)))

Does it make sense to keep matching the lambda separately? This just
keeps getting better=)

Thanks and cheers!
Tor-björn

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

> Clever! I had to put the let inside the lambda for it to work.

You probably do not have to once you use lexical binding (that is - not
C-x C-e ad-hoc, but put things into actual byte-compiled file)

But let inside the lambda body is perfectly fine.

> (defun org-cite-basic-follow--parse-suffix-specification (specification)
>   (pcase specification
>     (`(,key ,desc (lambda . ,fn-args) . ,other)
>      (list key desc `(lambda ,@fn-args) ,other))
>     (`(,key ,desc (,fn . ,fn-args) . ,other)
>      `(,key ,desc
>             (lambda ()
>               (interactive)
>               (let ((!citation (car (transient-scope)))
>                     (!prefix (cadr (transient-scope)))
>                     (!citation-key (org-element-property :key (car (transient-scope)))))
>                 (,fn ,@fn-args)))
>             ,other))
>     (other other)))
>
> Does it make sense to keep matching the lambda separately? This just
> keeps getting better=)

AFIU, you need to match against lambda simply to avoid the next clause
matching it. If so, you can change the clause to match all ,fn, except
lambda like the following:

`(,key
  ,desc
  (,(and fn (guard (not (eq fn 'lambda))))
   . ,fn-args)
  . ,other)

This is getting ugly though.
An alternative would be simply

(defun org-cite-basic-follow--parse-suffix-specification (specification)
  (pcase specification
    ((and val `(,key ,desc (,fn . ,fn-args) . ,other))
     (if (eq fn 'lambda) val
       `(,key ,desc
              (lambda ()
		(interactive)
		(let ((!citation (car (transient-scope)))
                      (!prefix (cadr (transient-scope)))
                      (!citation-key (org-element-property :key (car (transient-scope)))))
                  (,fn ,@fn-args)))
              ,@other)))
    (other other)))

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

Ihor Radchenko <yantar92@posteo.net> writes:
> AFIU, you need to match against lambda simply to avoid the next clause
> matching it.

Exactly.

> If so, you can change the clause to match all ,fn, except
> lambda like the following:
>
> `(,key
>   ,desc
>   (,(and fn (guard (not (eq fn 'lambda))))
>    . ,fn-args)
>   . ,other)
>
> This is getting ugly though.
> An alternative would be simply
>
> (defun org-cite-basic-follow--parse-suffix-specification (specification)
>   (pcase specification
>     ((and val `(,key ,desc (,fn . ,fn-args) . ,other))
>      (if (eq fn 'lambda) val
>        `(,key ,desc
>               (lambda ()
> 		(interactive)
> 		(let ((!citation (car (transient-scope)))
>                       (!prefix (cadr (transient-scope)))
>                       (!citation-key (org-element-property :key (car (transient-scope)))))
>                   (,fn ,@fn-args)))
>               ,@other)))
>     (other other)))

I feel that the guard option does the right thing by directly fixing
the pattern matching - but what approach do you prefer?

Is this starting to be a good time for me to produce a patch?

Cheers,
Tor-björn

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

> I feel that the guard option does the right thing by directly fixing
> the pattern matching - but what approach do you prefer?

I provided the guard example just for your reference.
The preference for actual code is more readable code.
IMHO, my second variant with (and val ...) is more readable.

> Is this starting to be a good time for me to produce a patch?

Yes. Thanks in advance!

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

Ihor Radchenko <yantar92@posteo.net> writes:

> Tor-björn Claesson <tclaesson@gmail.com> writes:
>
>> I feel that the guard option does the right thing by directly fixing
>> the pattern matching - but what approach do you prefer?
>
> I provided the guard example just for your reference.
> The preference for actual code is more readable code.
> IMHO, my second variant with (and val ...) is more readable.
>

A, thanks, I will use that then!

>> Is this starting to be a good time for me to produce a patch?
>
> Yes. Thanks in advance!

Would it make sense to add two functions for getting the doi and url
from a citation? That would be generally useful, and simplify the
transient menu specification.

Also, is this change big enough that I should apply for copyright
assignment?

Cheers,
Tor-björn

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

>>> Is this starting to be a good time for me to produce a patch?
>>
>> Yes. Thanks in advance!
>
> Would it make sense to add two functions for getting the doi and url
> from a citation? That would be generally useful, and simplify the
> transient menu specification.

Do you mean opening citation URL derived from URL/DOI fields in browser?

> Also, is this change big enough that I should apply for copyright
> assignment?

FSF threshold for "tiny" changes is 15 lines of code.
I doubt that you can fit the changes within 15 lines, so yes, you do
need copyright assignment. See
https://orgmode.org/worg/org-contribute.html#copyright

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

Den lör 9 nov. 2024 kl 16:07 skrev Ihor Radchenko <yantar92@posteo.net>:
> > Would it make sense to add two functions for getting the doi and url
> > from a citation? That would be generally useful, and simplify the
> > transient menu specification.
>
> Do you mean opening citation URL derived from URL/DOI fields in browser?

No, that I think belongs in the transient suffix. Rather I meant a
function to get the
DOI from a citation. I thought about this, and would like to only
include the open
bibliography entry in the patch, and look into other options later.
Would this be ok?

>
> FSF threshold for "tiny" changes is 15 lines of code.
> I doubt that you can fit the changes within 15 lines, so yes, you do
> need copyright assignment. See
> https://orgmode.org/worg/org-contribute.html#copyright

Ok, thanks!

Cheers
Tor-björn

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

> ... I thought about this, and would like to only
> include the open
> bibliography entry in the patch, and look into other options later.
> Would this be ok?

Yup.

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

Here is my first attempt.
I have read the commit guidelines, but it is very possible that I have
misunderstood or just missed something, so I'm grateful for any
feedback!

Cheers,
Tor-björn

Den sön 10 nov. 2024 kl 18:40 skrev Ihor Radchenko <yantar92@posteo.net>:
>
> Tor-björn Claesson <tclaesson@gmail.com> writes:
>
> > ... I thought about this, and would like to only
> > include the open
> > bibliography entry in the patch, and look into other options later.
> > Would this be ok?
>
> Yup.

[-- Attachment #2: 0001-lisp-oc-basic.el-Transient-menu-for-following-citati.patch --]
[-- Type: text/x-patch, Size: 7625 bytes --]

From c8731ecd5db2beecc434a65448a7302212aab95a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tor-bj=C3=B6rn=20Claesson?= <tclaesson@gmail.com>
>
Date: Mon, 11 Nov 2024 11:46:32 +0200
Subject: [PATCH] lisp/oc-basic.el: Transient menu for following citations

* lisp/oc-basic.el (require 'transient): Pull in transient.
(org-cite-basic-follow-ask): New customization option. should
`org-cite-basic-follow' should prompt the user for an action?
(org-cite-basic-follow-actions): New customization option, that
specifies the contents of the transient menu.
(org-cite-basic-follow): New function.  Displays a menu asking how to
follow a citation if called with a negative prefix, or
`org-cite-basic-follow-ask' is non-nil. Otherwise, it retains the
default behaviour of opening the bibliography entry.
(org-cite-basic-follow--parse-suffix-specification,
org-cite-basic-follow--setup): Helper functions for
`org-cite-basic-follow'.
(org-cite-register-processor 'basic): Update the basic citation
processor to follow citations using `org-cite-basic-follow'.

* etc/ORG_NEWS (Menu for choosing how to follow citations): Describe
the new feature
(New option ~org-cite-basic-follow-ask~): Describe this new
customization option.
(New option ~org-cite-basic-follow-actions~): Describe this new
customization option, which specifies the layaout of the
`org-cite-basic-follow' transient menu.

This change was co-authored with much support from Ihor Radchenko and
Jonas Bernoulli, thanks!
---
 etc/ORG-NEWS     | 22 +++++++++++++
 lisp/oc-basic.el | 84 ++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 103 insertions(+), 3 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index de4f11b25..cf87e11bf 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -114,6 +114,17 @@ The keybindings in the repeat-maps can be changed by customizing
 
 See the new [[info:org#Repeating commands]["Repeating commands"]] section in Org mode manual.
 
+*** Menu for choosing how to follow citations
+If invoked with a prefix of C-- C-u, following citations with
+the org-cite-basic citation backend will no present a transient menu,
+offering choices for how to follow citations.
+
+The contents of this menu can be customized in
+~org-cite-basic-follow-actions~.
+
+In order to always show this menu, set ~org-cite-basic-follow-ask~
+to non-nil.
+
 ** New and changed options
 
 # Chanes deadling with changing default values of customizations,
@@ -158,6 +169,17 @@ English.  The default value is ~t~ as the CSL standard assumes that
 English titles are specified in sentence-case but the bibtex
 bibliography format requires them to be written in title-case.
 
+*** New option ~org-cite-basic-follow-ask~
+When this option is non-nil, following a citation with the basic citation
+backend will present a transient menu with choices for how to follow the
+citation.
+If nil, following a citation will open its bibliography entry.
+
+*** New option ~org-cite-basic-follow-actions~
+This option specifies the options presented by ~org-cite-basic-follow~
+if it is invoked with a C-- C-u prefix or ~org-cite-basic-follow-ask~
+is non-nil.
+
 ** New functions and changes in function arguments
 
 # This also includes changes in function behavior from Elisp perspective.
diff --git a/lisp/oc-basic.el b/lisp/oc-basic.el
index e207a1997..8add329b7 100644
--- a/lisp/oc-basic.el
+++ b/lisp/oc-basic.el
@@ -74,6 +74,7 @@
 (require 'map)
 (require 'oc)
 (require 'seq)
+(require 'transient)
 
 (declare-function org-open-at-point "org" (&optional arg))
 (declare-function org-open-file "org" (path &optional in-emacs line search))
@@ -140,6 +141,36 @@
   :type 'face
   :safe #'facep)
 
+(defcustom org-cite-basic-follow-ask nil
+  "Should `org-cite-basic' ask how to follow citations?
+
+When this option is nil, `org-cite-basic-follow' opens the bibliography entry.
+Otherwise, `org-cite-basic-follow' will display a transient menu prompting the 
+user for an action. The contents of this menu can be customized in 
+`org-cite-basic-follow-actions'."
+  :group 'org-cite
+  :package-version '(Org . "9.8")
+  :type 'boolean)
+
+(defcustom org-cite-basic-follow-actions
+  '[["Open"
+     ("b" "bibliography entry" (org-cite-basic-goto !citation !prefix))]]
+  "Actions in the `org-cite-basic-follow' transient menu.
+
+This option uses the same syntax as `transient-define-prefix', see Info node
+`(transient)Binding Suffix and Infix Commands'. In addition, it is possible 
+to specify a function call for the COMMAND part, where !citation, 
+!prefix, and !citation-key can be used to access those values. 
+
+If COMMAND is a lambda form, it can access the citation and prefix like this:
+
+  (lambda (citation prefix)
+     (interactive (transient-scope))
+     ...)"
+  :group 'org-cite
+  :package-version '(Org . "9.8")
+  :type 'sexp)
+
 \f
 ;;; Internal variables
 (defvar org-cite-basic--bibliography-cache nil
@@ -832,6 +863,53 @@ present in the citation."
        (bibtex-set-dialect)
        (bibtex-search-entry key)))))
 
+(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
+  "Follow citation.
+
+This transient is invoked through `org-open-at-point'. 
+When `org-open-at-point' is invoked with a negative prefix,
+or `org-cite-basic-follow-ask' is non-nil, it will present
+a transient menu prompting the user for an action. Otherwise,
+it will open the bibliography entry for the citation at point.
+
+Suffixes can not be added to this transient menu using the ordinary
+`transient-append-suffix' or `transient-insert-suffix', instead, the
+contents of the menu are defined in the variable
+`org-cite-basic-follow-actions'."
+  [:class transient-columns
+          :setup-children org-cite-basic-follow--setup
+          :pad-keys t]
+  (interactive)
+  (if (or org-cite-basic-follow-ask
+          (equal prefix '(-4)))
+      (transient-setup 'org-cite-basic-follow nil nil
+                       :scope (list citation prefix))
+    (org-cite-basic-goto citation prefix)))
+
+(defun org-cite-basic-follow--parse-suffix-specification (specification)
+  (pcase specification
+    ((and val `(,key ,desc (,fn . ,fn-args) . ,other))
+     (if (eq fn 'lambda) val
+       `(,key ,desc
+              (lambda ()
+		(interactive)
+		(let ((!citation (car (transient-scope)))
+                      (!prefix (cadr (transient-scope)))
+                      (!citation-key
+                       (org-element-property :key (car (transient-scope)))))
+                  (,fn ,@fn-args)))
+              ,@other)))
+    (other other)))
+
+(defun org-cite-basic-follow--setup (_)
+  (transient-parse-suffixes
+   'org-cite-basic-follow
+   (cl-map 'vector
+           (lambda (group)
+             (cl-map 'vector #'org-cite-basic-follow--parse-suffix-specification
+                     group))
+           org-cite-basic-follow-actions)))
+
 \f
 ;;; "Insert" capability
 (defun org-cite-basic--complete-style (_)
@@ -920,9 +998,9 @@ Raise an error when no bibliography is set in the buffer."
   :activate #'org-cite-basic-activate
   :export-citation #'org-cite-basic-export-citation
   :export-bibliography #'org-cite-basic-export-bibliography
-  :follow #'org-cite-basic-goto
-  :insert (org-cite-make-insert-processor #'org-cite-basic--complete-key
-                                          #'org-cite-basic--complete-style)
+  :follow #'org-cite-basic-follow
+  :insert  (org-cite-make-insert-processor #'org-cite-basic--complete-key
+                                           #'org-cite-basic--complete-style)
   :cite-styles
   '((("author" "a") ("caps" "c"))
     (("noauthor" "na") ("bare" "b"))
-- 
2.45.2

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

> Here is my first attempt.
> I have read the commit guidelines, but it is very possible that I have
> misunderstood or just missed something, so I'm grateful for any
> feedback!

Thanks for the patch!

See my comments below.

> * lisp/oc-basic.el (require 'transient): Pull in transient.

Technically, we still support Emacs 27, which does not have
transient. But accounting for this would be a pain and Emacs 30 is
probably going to be released some time around the beginning of the next
year. So, let's not worry about this and accept that Org 9.8-pre (next
release) no longer supports Emacs 27.

If you are a user of Emacs 27 + Org main branch, and have strong
objections, please chime in.

> +*** Menu for choosing how to follow citations
> +If invoked with a prefix of C-- C-u, following citations with
> +the org-cite-basic citation backend will no present a transient menu,
> +offering choices for how to follow citations.

I imagine C-- C-u more as a toggle - if `org-cite-basic-follow-ask' is
t, it disables the menu; enables otherwise.

> +(defcustom org-cite-basic-follow-ask nil
> +  "Should `org-cite-basic' ask how to follow citations?
> +
> +When this option is nil, `org-cite-basic-follow' opens the bibliography entry.
> +Otherwise, `org-cite-basic-follow' will display a transient menu prompting the 
> +user for an action. The contents of this menu can be customized in 
> +`org-cite-basic-follow-actions'."

Note: our convention is to use double space between sentences.
There are also typos, but that's a minor thing I can fix myself before
merging.

> +(defcustom org-cite-basic-follow-actions
> +  '[["Open"
> +     ("b" "bibliography entry" (org-cite-basic-goto !citation !prefix))]]
> +  "Actions in the `org-cite-basic-follow' transient menu.
> +
> +This option uses the same syntax as `transient-define-prefix', see Info node
> +`(transient)Binding Suffix and Infix Commands'. In addition, it is possible 
> +to specify a function call for the COMMAND part, where !citation, 
> +!prefix, and !citation-key can be used to access those values. 
> +
> +If COMMAND is a lambda form, it can access the citation and prefix like this:
> +
> +  (lambda (citation prefix)
> +     (interactive (transient-scope))
> +     ...)"

Could we make !prefix, and !citation work in lambdas? We should be able
to.

> +(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
> +  "Follow citation.
> +
> +This transient is invoked through `org-open-at-point'. 
> +When `org-open-at-point' is invoked

It should not matter for this command where it is called from.
You can simply drop references to `org-open-at-point'

We can even make the prefix work as a standalone command. Simply using
interactive spec.

(interactive
   (list (let ((obj (org-element-context)))
           (pcase (org-element-type obj)
             ((or citation citation-reference) obj)
             (_ (user-error "No citation at point"))))))

> ... with a negative prefix, +or `org-cite-basic-follow-ask' is
> non-nil, it will present +a transient menu prompting the user for an
> action. Otherwise, +it will open the bibliography entry for the
> citation at point.

> +Suffixes can not be added to this transient menu using the ordinary
> +`transient-append-suffix' or `transient-insert-suffix', instead, the
> +contents of the menu are defined in the variable
> +`org-cite-basic-follow-actions'."

Suffixes can be added. Try

(transient-append-suffix 'org-cite-basic-follow nil
  '[["Other" ("t" "test" (lambda () (interactive) (message "Hello!")))]])

> +(defun org-cite-basic-follow--parse-suffix-specification (specification)
> +  (pcase specification

Please, add the docstring.

> +    ((and val `(,key ,desc (,fn . ,fn-args) . ,other))
> +     (if (eq fn 'lambda) val
> +       `(,key ,desc
> +              (lambda ()
> +		(interactive)
> +		(let ((!citation (car (transient-scope)))
> +                      (!prefix (cadr (transient-scope)))
> +                      (!citation-key
> +                       (org-element-property :key (car (transient-scope)))))

`org-open-at-point' may be called with point at citation rather than
citation reference. Citation object does not have :key property.

I think that we should drop !citation-key spec and instead specify that
the command may be called with citation or citation-reference object in !citation.

> +(defun org-cite-basic-follow--setup (_)
> +  (transient-parse-suffixes

Docstring here as well.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

[-- Attachment #1: Type: text/plain, Size: 4756 bytes --]

Ihor Radchenko <yantar92@posteo.net> writes:
> See my comments below.
Thanks for taking the time to look at this!

>> +*** Menu for choosing how to follow citations
>> +If invoked with a prefix of C-- C-u, following citations with
>> +the org-cite-basic citation backend will no present a transient menu,
>> +offering choices for how to follow citations.
>
> I imagine C-- C-u more as a toggle - if `org-cite-basic-follow-ask' is
> t, it disables the menu; enables otherwise.
That is more useful, thanks!

>
>> +(defcustom org-cite-basic-follow-ask nil
>> +  "Should `org-cite-basic' ask how to follow citations?
>> +
>> +When this option is nil, `org-cite-basic-follow' opens the bibliography entry.
>> +Otherwise, `org-cite-basic-follow' will display a transient menu prompting the 
>> +user for an action. The contents of this menu can be customized in 
>> +`org-cite-basic-follow-actions'."
>
> Note: our convention is to use double space between sentences.
> There are also typos, but that's a minor thing I can fix myself before
> merging.

Ah, bummer. I have fixed (at least some) of those typos.

>> +(defcustom org-cite-basic-follow-actions
>> +  '[["Open"
>> +     ("b" "bibliography entry" (org-cite-basic-goto !citation !prefix))]]
>> +  "Actions in the `org-cite-basic-follow' transient menu.
>> +
>> +This option uses the same syntax as `transient-define-prefix', see Info node
>> +`(transient)Binding Suffix and Infix Commands'. In addition, it is possible 
>> +to specify a function call for the COMMAND part, where !citation, 
>> +!prefix, and !citation-key can be used to access those values. 
>> +
>> +If COMMAND is a lambda form, it can access the citation and prefix like this:
>> +
>> +  (lambda (citation prefix)
>> +     (interactive (transient-scope))
>> +     ...)"
>
> Could we make !prefix, and !citation work in lambdas? We should be able
> to.

Sure! It took me some trial and error to get the list splicing right but
now it works in lambdas to. I added another helper to do let binding.

>> +(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
>> +  "Follow citation.
>> +
>> +This transient is invoked through `org-open-at-point'. 
>> +When `org-open-at-point' is invoked
>
> It should not matter for this command where it is called from.
> You can simply drop references to `org-open-at-point'
>
> We can even make the prefix work as a standalone command. Simply using
> interactive spec.
>
> (interactive
>    (list (let ((obj (org-element-context)))
>            (pcase (org-element-type obj)
>              ((or citation citation-reference) obj)
>              (_ (user-error "No citation at point"))))))

I was not able to get this to work.

>> ... with a negative prefix, +or `org-cite-basic-follow-ask' is
>> non-nil, it will present +a transient menu prompting the user for an
>> action. Otherwise, +it will open the bibliography entry for the
>> citation at point.
>
>> +Suffixes can not be added to this transient menu using the ordinary
>> +`transient-append-suffix' or `transient-insert-suffix', instead, the
>> +contents of the menu are defined in the variable
>> +`org-cite-basic-follow-actions'."
>
> Suffixes can be added. Try
>
> (transient-append-suffix 'org-cite-basic-follow nil
>   '[["Other" ("t" "test" (lambda () (interactive) (message "Hello!")))]])

Ah, I didn't know you could nil the position.

>> +(defun org-cite-basic-follow--parse-suffix-specification (specification)
>> +  (pcase specification
>
> Please, add the docstring.
>

Ok.

>> +    ((and val `(,key ,desc (,fn . ,fn-args) . ,other))
>> +     (if (eq fn 'lambda) val
>> +       `(,key ,desc
>> +              (lambda ()
>> +		(interactive)
>> +		(let ((!citation (car (transient-scope)))
>> +                      (!prefix (cadr (transient-scope)))
>> +                      (!citation-key
>> +                       (org-element-property :key (car (transient-scope)))))
>
> `org-open-at-point' may be called with point at citation rather than
> citation reference. Citation object does not have :key property.
>
> I think that we should drop !citation-key spec and instead specify that
> the command may be called with citation or citation-reference object in !citation.

`org-cite-basic-goto' handles this by prompting the user for a key, if
it is called with a citation object. I adopted this approach. (I find the
!citation-key useful for a lot of things I like to do to citations, and
would like to keep it.)

>> +(defun org-cite-basic-follow--setup (_)
>> +  (transient-parse-suffixes
>
> Docstring here as well.

Ok.

Version 2 of the patch is attached.

Cheers,
Tor-björn


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: transient-follower-version2 --]
[-- Type: text/x-diff, Size: 8075 bytes --]

From e91fff9f6c03e2c766dee410f46382398f883997 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tor-bj=C3=B6rn=20Claesson?= <tclaesson@gmail.com>
Date: Tue, 12 Nov 2024 11:09:16 +0200
Subject: [PATCH] lisp/oc-basic.el: Transient menu for following citations

* lisp/oc-basic.el (require 'transient): Pull in transient.
(org-cite-basic-follow-ask): New customization option. should
`org-cite-basic-follow' prompt the user for an action?
(org-cite-basic-follow-actions): New customization option, that
specifies the contents of the transient menu.
(org-cite-basic-follow): New function.  Displays a menu asking how to
follow a citation if `org-cite-basic-follow-ask' is
non-nil. Otherwise, it retains the default behaviour of opening the
bibliography entry. This can be inversed with a negative prefix argument.
(org-cite-basic-follow--parse-suffix-specification,
org-cite-basic-follow--wrap-function, and
org-cite-basic-follow--setup): Helper functions for
`org-cite-basic-follow'.
(org-cite-register-processor 'basic): Update the basic citation
processor to follow citations using `org-cite-basic-follow'.

* etc/ORG_NEWS (Menu for choosing how to follow citations): Describe
the new feature
(New option ~org-cite-basic-follow-ask~): Describe this new
customization option.
(New option ~org-cite-basic-follow-actions~): Describe this new
customization option, which specifies the layout of the
`org-cite-basic-follow' transient menu.

This change was co-authored with much support from Ihor Radchenko and
Jonas Bernoulli, thanks!
---
 etc/ORG-NEWS     | 22 +++++++++++
 lisp/oc-basic.el | 95 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 116 insertions(+), 1 deletion(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index de4f11b25..b859b0ada 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -114,6 +114,15 @@ The keybindings in the repeat-maps can be changed by customizing
 
 See the new [[info:org#Repeating commands]["Repeating commands"]] section in Org mode manual.
 
+*** Menu for choosing how to follow citations
+
+Following citations with the org-cite-basic citation backend can now present a
+transient menu. To show this menu, set ~org-cite-basic-follow-ask~ to non-nil. 
+This behaviour can be reversed with a negativ prefix.
+
+The contents of this menu can be customized in
+~org-cite-basic-follow-actions~.
+
 ** New and changed options
 
 # Chanes deadling with changing default values of customizations,
@@ -158,6 +167,19 @@ English.  The default value is ~t~ as the CSL standard assumes that
 English titles are specified in sentence-case but the bibtex
 bibliography format requires them to be written in title-case.
 
+*** New option ~org-cite-basic-follow-ask~
+
+When this option is non-nil, following a citation with the basic citation
+backend will present a transient menu with choices for how to follow the
+citation.
+If nil, following a citation will open its bibliography entry.
+
+This behaviour can be reversed with a negative prefix argument.
+
+*** New option ~org-cite-basic-follow-actions~
+
+This option specifies the options presented by ~org-cite-basic-follow~.
+
 ** New functions and changes in function arguments
 
 # This also includes changes in function behavior from Elisp perspective.
diff --git a/lisp/oc-basic.el b/lisp/oc-basic.el
index e207a1997..c3fc3f34d 100644
--- a/lisp/oc-basic.el
+++ b/lisp/oc-basic.el
@@ -74,6 +74,7 @@
 (require 'map)
 (require 'oc)
 (require 'seq)
+(require 'transient)
 
 (declare-function org-open-at-point "org" (&optional arg))
 (declare-function org-open-file "org" (path &optional in-emacs line search))
@@ -140,6 +141,30 @@
   :type 'face
   :safe #'facep)
 
+(defcustom org-cite-basic-follow-ask nil
+  "Should `org-cite-basic' ask how to follow citations?
+
+When this option is nil, `org-cite-basic-follow' opens the bibliography entry. 
+Otherwise, `org-cite-basic-follow' will display a transient menu prompting the 
+user for an action.  The contents of this menu can be customized in 
+`org-cite-basic-follow-actions'."
+  :group 'org-cite
+  :package-version '(Org . "9.8")
+  :type 'boolean)
+
+(defcustom org-cite-basic-follow-actions
+  '[["Open"
+     ("b" "bibliography entry" (org-cite-basic-goto !citation !prefix))]]
+  "Actions in the `org-cite-basic-follow' transient menu.
+
+This option uses the same syntax as `transient-define-prefix', see Info node
+`(transient)Binding Suffix and Infix Commands'.  In addition, it is possible 
+to specify a function call for the COMMAND part, where !citation, 
+!prefix, and !citation-key can be used to access those values."
+  :group 'org-cite
+  :package-version '(Org . "9.8")
+  :type 'sexp)
+
 \f
 ;;; Internal variables
 (defvar org-cite-basic--bibliography-cache nil
@@ -832,6 +857,74 @@ present in the citation."
        (bibtex-set-dialect)
        (bibtex-search-entry key)))))
 
+(transient-define-prefix org-cite-basic-follow (citation &optional prefix)
+  "Follow citation.
+
+If `org-cite-basic-follow-ask' is non-nil, this transient will present
+a menu prompting the user for an action. 
+Otherwise, it will open the bibliography entry for the citation at point.  
+This behaviour is inverted when the transient is called with a negative prefix
+argument.
+
+The contents of the menu are defined in the variable
+`org-cite-basic-follow-actions'."
+  [:class transient-columns
+          :setup-children org-cite-basic-follow--setup
+          :pad-keys t]
+  (interactive)
+  (if (xor org-cite-basic-follow-ask
+           (equal prefix '(-4)))
+      (transient-setup 'org-cite-basic-follow nil nil
+                       :scope (list citation prefix))
+    (org-cite-basic-goto citation prefix)))
+
+(defun org-cite-basic-follow--wrap-function (fn-args &optional fn)
+  "Provide !citation, !prefix, and !citation-key symbols."
+  (append 
+   `(let ((!citation (car (transient-scope)))
+          (!prefix (cadr (transient-scope)))
+          (!citation-key
+           (if (org-element-type-p (car (transient-scope)) 'citation-reference)
+               (org-element-property :key (car (transient-scope)))
+             (pcase (org-cite-get-references (car (transient-scope)) t)
+               (`(,key) key)
+               (keys
+                (or (completing-read "Select citation key: " keys nil t)
+                    (user-error "Aborted"))))))))
+   (if fn
+       `((,fn ,@fn-args))
+     fn-args)))
+
+(defun org-cite-basic-follow--parse-suffix-specification (specification)
+  "Handle special syntax for `org-cite-basic-follow-actions'."
+  (pcase specification
+    (`(,key ,desc (lambda ,args . ,fn-args) . ,other)
+     `(,key ,desc
+            (lambda ,args
+              ,(unless (and (listp (car fn-args))
+                            (equal (caar fn-args)
+                                   'interactive))
+                 '(interactive))
+              ,(org-cite-basic-follow--wrap-function fn-args))
+            ,@other))
+    (`(,key ,desc (,fn . ,fn-args) . ,other)
+     `(,key ,desc
+            (lambda ()
+	      (interactive)
+	      ,(org-cite-basic-follow--wrap-function fn-args fn))
+            ,@other))
+    (other other)))
+
+(defun org-cite-basic-follow--setup (_)
+  "Update `org-cite-basic-follow' when `org-cite-basic-follow-actions' changes."
+  (transient-parse-suffixes
+   'org-cite-basic-follow
+   (cl-map 'vector
+           (lambda (group)
+             (cl-map 'vector #'org-cite-basic-follow--parse-suffix-specification
+                     group))
+           org-cite-basic-follow-actions)))
+
 \f
 ;;; "Insert" capability
 (defun org-cite-basic--complete-style (_)
@@ -920,7 +1013,7 @@ Raise an error when no bibliography is set in the buffer."
   :activate #'org-cite-basic-activate
   :export-citation #'org-cite-basic-export-citation
   :export-bibliography #'org-cite-basic-export-bibliography
-  :follow #'org-cite-basic-goto
+  :follow #'org-cite-basic-follow
   :insert (org-cite-make-insert-processor #'org-cite-basic--complete-key
                                           #'org-cite-basic--complete-style)
   :cite-styles
-- 
2.46.0

Re: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

>> Could we make !prefix, and !citation work in lambdas? We should be able
>> to.
>
> Sure! It took me some trial and error to get the list splicing right but
> now it works in lambdas to. I added another helper to do let binding.

I think that a simple

(let ((!prefix ...) ...)
 (lambda ...))

will work.

But you should test without C-x C-e. Need to reload the whole library.

>> We can even make the prefix work as a standalone command. Simply using
>> interactive spec.
>>
>> (interactive
>>    (list (let ((obj (org-element-context)))
>>            (pcase (org-element-type obj)
>>              ((or citation citation-reference) obj)
>>              (_ (user-error "No citation at point"))))))
>
> I was not able to get this to work.

Just replace the interactive spec with my version and then
M-x org-cite-basic-follow with point on citation in Org document.

>> `org-open-at-point' may be called with point at citation rather than
>> citation reference. Citation object does not have :key property.
>>
>> I think that we should drop !citation-key spec and instead specify that
>> the command may be called with citation or citation-reference object in !citation.
>
> `org-cite-basic-goto' handles this by prompting the user for a key, if
> it is called with a citation object. I adopted this approach. (I find the
> !citation-key useful for a lot of things I like to do to citations, and
> would like to keep it.)

I am not sure if it is a good idea.
Commands in org-cite-basic-follow-actions may or may not need it, while
your code will _aways_ prompt user about citation key; even when the
citation key is never used.

If you realy, really want it, we can go into `cl-symbol-macrolet' and
lazy evaluation, but will be tricky (especially arranging for
(setq !citation-key ...) to work.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Fwd: Org-cite: Replace basic follow-processor with transient menu?

[Tor-björn Claesson]

[-- Attachment #1.1: Type: text/plain, Size: 1513 bytes --]

Ihor Radchenko <yantar92@posteo.net> writes:
> Tor-björn Claesson <tclaesson@gmail.com> writes:
>
>> Den tis 12 nov. 2024 kl 20:02 skrev Ihor Radchenko <yantar92@posteo.net>:
>>>
>>> I am not sure if it is a good idea.
>>> Commands in org-cite-basic-follow-actions may or may not need it, while
>>> your code will _aways_ prompt user about citation key; even when the
>>> citation key is never used.
>>>
>>> If you realy, really want it, we can go into `cl-symbol-macrolet' and
>>> lazy evaluation, but will be tricky (especially arranging for
>>> (setq !citation-key ...) to work.
>>>
>>
>> Wouldn't my beginner approach with recursive replacement fix this
problem?
>
> Nope. Mindlessly replacing instances of !citation-key with value may
> break the code. Consider, for example,
>
> (lambda ()
>   (let ((citation! (concat citation! "-foo"))) ...))
>
> There will be more complex cases as well.
>
> `cl-symbol-macrolet' it trying to handle what you tried with recursive
> replacement, but more carefully. But even `cl-symbol-macrolet' fails in
> certain edge cases.

Ah, then it has to go. Here comes a fixed patch. The code is much
simpler like this, but i kept the let under the lambda because for some
reason it did not work for me. Also, the interactive clause in the
transient,
while working, upset make test, but this could be fixed by requiring
org-element and quoting citation and citation-reference.

Thanks for taking the time to explain!

Cheers,
Tor-björn

[-- Attachment #1.2: Type: text/html, Size: 2088 bytes --]

[-- Attachment #2: 0001-lisp-oc-basic.el-Transient-menu-for-following-v3.patch --]
[-- Type: application/x-patch, Size: 7955 bytes --]

Re: Fwd: Org-cite: Replace basic follow-processor with transient menu?

[Ihor Radchenko]

Tor-björn Claesson <tclaesson@gmail.com> writes:

> Ah, then it has to go. Here comes a fixed patch. ...

Thanks.
See my comments below.

> +*** Menu for choosing how to follow citations

Maybe "New transient menu when following citations"

> +Following citations with the org-cite-basic citation backend can now present a
> +transient menu. To show this menu, set ~org-cite-basic-follow-ask~ to non-nil. 
> +This behaviour can be reversed with a negativ prefix.

"-4" prefix. Not negative.

> +This behaviour can be reversed with a negative prefix argument.

Same. "-4".

> +(defcustom org-cite-basic-follow-actions
> +  '[["Open"
> +     ("b" "bibliography entry" (org-cite-basic-goto !citation !prefix))]]
> +  "Actions in the `org-cite-basic-follow' transient menu.
> +
> +This option uses the same syntax as `transient-define-prefix', see Info node
> +`(transient)Binding Suffix and Infix Commands'.  In addition, it is possible 
> +to specify a function call for the COMMAND part, where !citation and 
> +!prefix can be used to access those values."
> +  :group 'org-cite
> +  :package-version '(Org . "9.8")
> +  :type 'sexp)

1. Ideally, we want at least one more menu entry here. Otherwise, the
   menu is not very useful.
2. It would be nice to provide some examples on using !citation and
   !prefix in the docstring. Also, lambdas.
3. We need to explain what !citation and !prefix refer to.

> +(transient-define-prefix org-cite-basic-follow (citation-object &optional prefix)
> ...
> +This behaviour is inverted when the transient is called with a negative prefix
> +argument.

-4 prefix

> +(defun org-cite-basic-follow--parse-suffix-specification (specification)
> +  "Handle special syntax for `org-cite-basic-follow-actions'."
> +  (pcase specification
> +    (`(,key ,desc (lambda ,args . ,fn-args) . ,other)
> +     `(,key ,desc
> +            (lambda ,args
> +              ,(unless (and (listp (car fn-args))
> +                            (equal (caar fn-args)
> +                                   'interactive))
> +                 '(interactive))
> +              (let ((!citation (car (transient-scope)))
> +                    (!prefix (cadr (transient-scope))))

This can be improved a bit.
Rather than storing transient scope as (list citation prefix), we can
use plist: (list :citation citation :prefix prefix). Then, we can do
(let ((!citation (plist-get (transient-scope) :citation))
      (!prefix (plist-get (transient-scope) :prefix))) ...)

It will be more readable.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>