Tor-björn Claesson <[email protected]> writes:
> The small one: should we drop the description field and always use the
> first line of the docstring for menu titles? This feels much smoother.
This will be easier. And we can always add description field in the
future if the need arises.
> The big: should this be about choosing an "action" at point, or a
> more general framework for presenting menus? If it remains small in
> scope, should we find an other name than org-menu?
org-menu is already pretty general name.
Consider menus like org-capture that can be displayed from arbitrary
buffers - querying Org parser would make no sense if we try to implement
org-capture menu using your library.
If you want to widen the scope even further, we will need to reach our
to the emacs-devel, and there will be even more requirements.
> +(defun org-cite-basic--get-key (citation-or-citation-reference)
> + "Return citation key for CITATION-OR-CITATION-KEY."
> +(defun org-cite-basic--get-url (citation-or-citation-reference)
> + "Return URL for CITATION-OR-CITATION-KEY."
> +(defun org-cite-basic--get-doi (citation-or-citation-reference)
> + "Return DOI for CITATION-OR-CITATION-KEY."
*CITATION-OR-CITATION-REFERENCE.
> +(org-menu-define org-cite-basic-follow (citation-object &optional
> prefix-argument)
> + "Basic citation follower.
> +
> +Open citations by applying the function in
> +`org-cite-basic-follow-default-action'. If `org-menu-mode' is active,
> display a
> +menu specified in `org-cite-basic-follow-actions'. This behaviour can be
> inverted
> +by giving the prefix argument in `org-menu-switch'. See `org-menu-mode' for
> more information."
I think that most of this docstring is pretty general. Maybe
org-menu-define itself can auto-append it to the docstring?
> + ("w" "Browse URL/DOI"
> + (let ((url (org-cite-basic--get-url !citation-object))
> + (doi (org-cite-basic--get-doi !citation-object)))
> + (cond ((org-string-nw-p url)
> + (browse-url url))
> + ((org-string-nw-p doi)
> + (if (string-match "^http" doi)
> + (browse-url doi)
> + (browse-url (format "http://dx.doi.org/%s"; doi))))
> + (t (user-error "No URL or DOI for `%s'"
> + (org-cite-basic--get-key !citation-object))))))]
This can probably be a separate new function instead of adding inline lambda.
> +(defun org-menu--specification-to-menu (description specification)
> + "Make a flattened menu keymap out of an org menu specification.
> +
> +SPECIFICATION should be of the form of `org-cite-basic-follow-actions'.
> +
> +The title of the menu keymap is DESCRIPTION."
> + (let ((new-map (make-sparse-keymap description)))
> + (named-let populate-menu-keymap
> + ;; First, make a flat reversed list of menu items. Items have the
> forms:
> + ;; ("title" MENU-HEADING) or
> + ;; (KEY DESCRIPTION BINDING)
> + ((posts (named-let build-list ((menu specification))
This looks like tail recursion. Any chance you can write the same using
some kind of loop/mapcar? (I admit that I do not 100% understand the logic)
> +(defun org-menu-popup (description specification)
> +(defun org-menu-tmm-prompt (description specification)
We should probably list these functions in defcustom specification for
org-menu-system.
> +This function is a valid value for `org-menu-system', which takes two
> arguments:
> +DESCRIPTION is the title for the menu, while SPECIFICATION is an org-menu
> +specification as per `org-cite-basic-follow-actions'."
> +(defun org-menu-tmm-prompt (description specification)
> + "Show an org-menu using a `tmm-prompt'.
> +
> +This function is a valid value for `org-menu-system', which takes two
> arguments:
> +DESCRIPTION is the title for the menu, while SPECIFICATION is an org-menu
> +specification as per `org-cite-basic-follow-actions'."
", which takes two arguments" is redundant. After removing, the
docstring will read just fine.
> +(defmacro org-menu--defcustom-actions (menu-actions value menu-name)
> + "Define MENU-ACTIONS option for MENU-NAME with default VALUE."
> + `(defcustom ,menu-actions ,value
> + ,(concat "Actions in the `" (symbol-name menu-name) "' org menu.
Since we are defining variables and functions dynamically, we should
probably take care about helping Emacs finding their definition. See
13.4.1 Finding Definitions section in Elisp manual.
> +(defun org-menu--strip-argument-decorators (arglist)
> + "Return a copy of ARGLIST without &optional, &body, &key, &aux, or &rest."
You forgot to update the docstring. &key and &aux are no longer cleaned up.
> +;;; Main macro definition
> +(cl-defmacro org-menu-define
> + (name arglist docstring description contents default-action &body body)
I am thinking if we should do (name arglist docstring &key contents
default-action &body body)
That will make menu definition more readable, as CONTENTS and
DEFAULT-ACTION will be clearly marked.
> + (transient-define-prefix
> + ,name ,arglist
I am looking at transient-define-prefix docs and the code and it looks
like ARGLIST cannot be cl-style. If so, we can simplify
`org-menu--strip-argument-decorators' even further, dropping '&body test.
> + (let ((bound-arguments
> + (list ,@(cl-mapcar
> + (lambda (param)
> + `(list
> + ',(intern (concat "!" (symbol-name param)))
> + `',,param))
> + (org-menu--strip-argument-decorators arglist)))))
This is pretty similar to `org-menu--with-arguments'. Can we somehow
reuse it?
--
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>
