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

[Ihor Radchenko <yantar92@posteo.net>]

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>