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