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>