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

[Jonas Bernoulli <jonas@bernoul.li>]

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