[email protected] writes:
>>> 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?
> ...
> I meant this the other way around - should we keep the scope limited?
> We started out designing this to be a way to chose an action at point,
> but now we are considering making it into a general menu system.
Well. It is now too tempting :)
> I think this is fine, in the way that we remove the forced
> org-element-context, but at least in its current form I do not think
> this system is a good fit for e.g. org export where we want to toggle async
> and such,
> but rather transients are better suited for those dialogue style uses.
> What do you think?
Good point. This highlights the reality that not every possible
transient menu can be displayed using non-transient menu backends. Nor
it should (or we will end up rewriting transient).
On the other hand, there is nothing stopping us (maybe in distant
future) from implementing some way to support toggles and other common
features of transient for other menus.
Note that I do not ask you to do it here and now. Rather your question
made me think about defining complex menus that cannot work without
transient at all. Or need completely custom menu backend.
Maybe some menus can say :menu-systems '(transient) to force using
transient even when other menu-backend is specified by user. That option
might be exposed to users. Then, we can easily incorporate
transient-only menus or even menus that employ a completely custom menu
implementation. It feels like an easy generalization that does not
require too much effort.
>>This can probably be a separate new function instead of adding inline lambda.
>
> I added this as org-cite-basic--browse.
I think it would be better if `org-cite-basic--browse' used its
argument rather than referring to !citation-object :)
Also, the function does not work in general because of stray comment (";").
>>> +(defun org-menu--specification-to-menu (description 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)
>
> We need to achieve two things:
> All entries (including titles and separators) in the keymap need unique keys.
> Entries must be added in reverse to the keymap.
> ...
I came up with the following version
(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))
(prev-item-was-header nil))
(letrec ((define-menu
(lambda (menu)
(seq-map
(lambda (item)
(message "%S" item)
(when prev-item-was-header
;; Add separator in all but last header.
(setq prev-item-was-header nil)
(define-key new-map `[,(gensym "separator-")] '(menu-item
"--")))
(pcase item
((pred vectorp) (funcall define-menu item))
((pred stringp)
;; FIXME: Use `keymap-set' when we drop Emacs 28 support.
(define-key new-map `[,(gensym "header-")] `(menu-item
,item))
(setq prev-item-was-header t))
(`(,key ,desc ,function)
(define-key new-map key `(menu-item ,desc ,function)))))
(seq-reverse menu)))))
(funcall define-menu specification)
new-map)))
>>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.
>
> Thanks. org-menu--defcustom-actions and org-menu--defcustom-default-action
> now add the definition-name property to the defined variables. I made this
> point
> to the overall menu definition, e.g. org-cite-basic-follow in our example. Is
> this
> good, or should it point to the macros in om.el?
What you did looks right.
>>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.
>
> I changed this so that we have keyword arguments CONTENTS, DEFAULT_ACTION, and
> INTERACTIVE-SPEC (defaulting to '(interactive), and removing the forced
> org-element-context.
Hmm. What about replacing :contents with :menu?
Also, :interactive-spec seems misleading. It is basically function body.
What about defining it as &body? It feels intuitive.
>>> + (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?
>
> I think they are different enough that at least trying to make a macro of the
> mapcar part would be more complicated than what we already have. It is very
> possible that I miss an obvious way to improve this,and will have to think
> about it for a while.
I tried to come up with something, but also could not.
--
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>
