Tor-björn Claesson <tclaes...@gmail.com> writes:
> Here is attached a new version of the patch for review. NEWS, commit
> message and docstrings are placeholders until we get the code right.
>
> The parameters of the menu are now customiseable, we have org-menu-mode,
> and I have been using the current version for a couple of days writing my
> thesis-book without problem.
Thanks!
I have some comments.
> I have not tested using a menu system other than transient. What would be a
> good candidate for this?
Probably which-key (it is built-in).
> +(org-menu-define org-cite-basic-follow (citation-object)
> + "Follow citations"
You can use (declare (indent defun)) in macro definition to make the
indentation work as in defun. See various macros in org-macs.el as examples.
> + '[["Open"
Is quote necessary here?
> + ("b" "bibliography entry" (org-cite-basic-goto
> !citation-object !prefix))]
> ...
> + :default-action 'org-cite-basic-goto
There is inconsistency here.
You can specify the exact arguments passed to menu-actions, but not to
default action. I think that :default-action should follow a shortened
menu-action spec.
> + ["Copy"
> + ("d" "DOI"
> + (let ((doi (org-cite-basic--get-field
> + 'doi
> + (org-cite-basic--get-key
> !citation-object))))
> + (if (not (s-blank? doi))
> + (kill-new doi)
> + (user-error "No DOI for `%s'"
> (org-cite-basic--get-key !citation-object)))))
Anonymous action definition is good for testing, but we should factor it
out to a separate function at the end.
> + ["Browse"
> + ("w" "Browse URL/DOI"
Maybe it should be under "Open" section as well.
> + :parameter-types ('citation 'citation-reference))
I believe the :parameter-types is way too specific for menu system for
be truly generic. Imagine if someone wants to have more than one
argument in a menu: (org-menu-define test (arg1 arg2 &optional arg3)
Rather than implicitly defining interactive spec as in
> + (interactive
> + (list (let ((obj (org-element-context)))
> + (pcase (org-element-type obj)
> + ((or ,@types) obj)
> + (_ (user-error "Wrong object type"))))))
you should let the caller handle interactive spec and other
argument pre-processing.
> +(cl-defmacro org-menu-define (name
> + arglist
> + docstring
> + contents
> + &key ((:default-action default-action) nil)
> + &key ((:parameter-types types) nil))
I suggest adding &rest body to the macro definition
and then placing that body directly before
> + (cond ((not (xor org-menu-mode
WDYT?
> +(defun org-menu--wrap-specification (specification arg-list)
> + "Handle special syntax for `org-cite-basic-follow-actions'."
Please describe in details what this special syntax as and what the
return value is.
> +
> +;;; Main macro definition
> +(cl-defmacro org-menu-define (name
> + arglist
> + docstring
> + contents
> + &key ((:default-action default-action) nil)
> + &key ((:parameter-types types) nil))
> ...
> + (complete-arglist (if (member 'prefix arglist)
> + arglist
> + `(,@arglist prefix))))
> + `(progn
> +
> +;;; Local customization
You are overdoing the page breaks and comments.
Please do not split macro body with section comments and page breaks.
Instead, I recommend defining a set of helper macros:
1. For each defcustom
2. For defun
3. for transient-define-suffic
Then, you can easily separate code for and place comments without
breaking a defmacro body.
> + (defcustom ,menu-actions ,contents
> + ,(concat "Actions in the `"
> + (symbol-name name)
> + "' org 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 ARGUMENTS can be used
> +to access those values.")
> + :group 'org-menu
> + :package-version '(Org . "9.8")
:package-version makes no sense here.
Also, it is a good idea to provide some examples in the docstring.
For users and also to make sure that we understand the org-menu design
ourselves :)
--
Ihor Radchenko // yantar92,
Org mode maintainer,
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>
