"Tor-björn Claesson" <[email protected]> writes: >> What about making an override a defcustom - >> org-<command>-overriding-menu? Then, users can set it to nil per >> command, thus allowing the effects of org-menu-disable-overrides and >> org-menu-system-overrides into a single place. > > I implemented this (also removing the global override alist, and making > org-menu-mode on by default). It feels clean and seems to work.
You went with <command>-override variable. I find that naming confusing. For org-attach, we have: - org-attach-default-action - org-attach-override - (and no org-attach-actions, because actions are dynamic) We also have a couple of "normal" customizations like org-attach-directory, and org-attach-method. There are several problems I see with naming: 1. The menu-related customization blend with global org-attach options. We should probably use something like org-attach-menu-default-action, org-attach-menu-actions. 2. org-attach-override meaning is not obvious. Even with org-attach-menu-override, it is not obvious that we are talking about menu system. Maybe we should name it as org-attach-menu-system-override > I think we should keep org-menu-disable-overrides. As we convert menus to > org-menu, there will be a lot of places where the overrides would have to > be disabled otherwise. Same thing with naming here, but otherwise I do not mind. Additionally, since we have overriding menu systems, this makes me think about another, related, use case. If some users want to use org-menu, but does not want menus for specific commands. For example, consider someone who wants org-attach menus, but does not want menu to be displayed for org-cite-basic-follow, preserving old behavior. We probably want a customization for that as well. Like org-cite-basic-follow-menu-disable or something. > ;;;###autoload > -(defun org-attach () > +(defun old-org-attach () This is against Elisp conventions. We should keep org-attach prefix in all the function names. Maybe something like org-attach-dispatch-classic (and use similar convention when we rewrite other Org menus). > +(defun org-attach--commands-to-transient-specification () > + "Produces a transient menu specification from `org-attach-commands'." > + `[["Select an Attachment Command:" > + ,@(seq-map (lambda (item) > + (pcase item > + (`(,keys ,command ,docstring) > + `(,(substring-no-properties (prin1-char (car keys)) 1) > + ,docstring > + (org-attach-at-destination (command-execute > #',command)))))) > + org-attach-commands)]]) The classic menu allows several bindings for the same command. For example: (?f ?\C-f). This way, users can conveniently type C-c C-a C-f holding control all the time. For transient, we probably have to add several duplicate items for each attach command. Maybe need to ask upstream implementing multi-binding suffixes. Also, the original dispatch shows current attach dir location. Can we do it in transient? If yes, will it break conversion of menu actions into Emacs popup menus? > +(org-menu-define org-attach () You forgot autoload cookie from the original version. > +*** Org menu > +Org menu is a flexible menu system, wich stores menus in a format similar to > +transient menus and can present them with different user interfaces. > + > +By disabling ~org-menu-mode~, a default action will be taken instead of > +showing a menu. This default action can be invoked when ~org-menu-mode~ is > +active (or the menu shown when ~org-menu-mode~ is inactive) by supplying a > +prefix argument specified in ~org-menu-switch~, by default C--. > + > +The menu frontend used is specified in ~org-menu-system~. This can be > overriden > +using menu specific override options. To always use the frontend > +set in ~org-menu-system~, set ~org-menu-disable-overrides~ to a non-nil > value. > + > +Org menus are implemented for ~org-cite-basic-follow~ and ~org-attach~. For > +~org-attach~, the menu system is by default overriden to use the original > +~org-attach~ function. The format of ~org-attach-commands~ remains > unchanged. This reads rather technical. The description you wrote may be ok (ish) for people hacking Org code, but ordinary users who do not care about menu formats will likely be confused. We can change this item later (it is not a high priority), but I invite you to think about the feature you are writing from a perspective of Org mode users who do not know Elisp technicalities - they know normal menu commands like org-attach, org-export, org-agenda, and tried transient (as users) occasionally (e.g. via casual package). If they were to read a news item, what they should be aware about? What is the key new thing they should try (or not try) to use? How can they roll everything back to previous default if they do not like new changes? Writing this version is useful to zoom out a think about the whole thing from UI perspective. > +;;; Minor mode > +(define-minor-mode org-menu-mode > + "Org menu mode. > +When Org menu mode is enabled, a menu prompting the user for an action > +will be presented upon activating certain objects. This is no longer accurate after we generalized the org-menu. > +(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))))) This will likely fail e.g. when we have an infix. More generally, I think we need to test defining menus for two other classes or menu commands in Org mode: 1. org-export-dispatch, where we have custom implementation on infixes where users can toggle certain export options right inside the menu. In particular, we need to make sure that those infixes are correctly mapped onto tmm/popup menus: toggles are probably not possible there, but we need to make sure that the suffixes using infix values can catch the correct default and are generally not broken when switching from transient to tmm/popup menus. 2. org-fast-tag-selection, where the whole point of the menu is having toggles. I am not yet sure if we can map this into, say, tmm menu at all. > +;;; Menu systems > +(defun org-menu-popup (description specification &rest _) > + "Show an org-menu using a `popup-menu'. > + > +This function is a valid value for `org-menu-system': > +DESCRIPTION is the title for the menu, while SPECIFICATION is an org-menu > +specification as per `org-cite-basic-follow-actions'. > +ARGS contains the arguments used to call the menu." > + (let ((menu-keymap (org-menu--specification-to-menu description > specification))) > + (popup-menu menu-keymap))) > + > +(defun org-menu-tmm-prompt (description specification &rest _) > + "Show an org-menu using a `tmm-prompt'. > + > +This function is a valid value for `org-menu-system': > +DESCRIPTION is the title for the menu, while SPECIFICATION is an org-menu > +specification as per `org-cite-basic-follow-actions'. > +ARGS contains the arguments used to call the menu." > + (let ((menu-keymap (org-menu--specification-to-menu description > specification))) > + (tmm-prompt menu-keymap))) We will eventually need to add which-key menus, I think. Low priority for now though. Writing so that it is not forgotten. > Also, I really must thank you again for all the help and guidance in this > project. I work in a specialised non technical field and often get to mentor > beginners or visiting fellows. Now I sometimes think that I should try to do > this as well as Ihor :-) Thanks for the kind words. Now, I just hope that I'm steering things in the right direction. In the past, we had a complex patch to add support for ANSI escape fontification, with me ending up realizing that the changes also require rewriting fontification code - I had to suspend that patch with lots of work invested. This is why I am trying to explore potential blockers and think more about the API. -- 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>
