Ihor Radchenko <[email protected]> writes: > "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. >
I changed the names as you suggested, this feels better. > 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. Added! > >> ;;;###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). Fixed. > >> +(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? > I will look into this for the next patch revision! >> +(org-menu-define org-attach () > > You forgot autoload cookie from the original version. > Does this mean "add ;;;###autoload before (org-menu-define org-attach () ...)"? If I do this compilation complains alot along the lines of: WARNING: No org-loaddefs.el file could be found from where org.el is loaded. You need to run "make" or "make autoloads" from Org lisp directory Is there some kind of dependency issue, that org-menu is not autoloaded? I have not found any good resource on this in the manual yet, pointers would be appreciated! >> +*** 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. > Thanks for the feedback/calibration! I will make another go at this! >> +;;; 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. > I do not understand this, is it related to the following? >> +(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. I think we have differing ideas about what org-menu should be. Let's discuss and find and agreement! When we discussed the scope of org-menu previously (now years ago!), I came away with the understanding that it would be menus, not dialogues with state, and that specifically not to replace org-export (for this we though that for example a transient would work better.) I have worked under the assumption that org-menu should be for simple pop up menu-style selection things. What should be our path forward? Could it be any of 1. Broaden org-menu to also encompass dialogues with state/toggles. Now we limit the possible menu-system backends, but can use org-menu to replace more things. 2. Use org-menu for the simple pop up menu use case and use transient or some abstraction (org-dialogue?) for more complicated things. This way org-menu can use more diverse backends for the menu-systems, and stay relatively simple. 3. Drop org-menu and use transients to replace menus and dialogues. This is a very clean solution, transient is mature and we do not have to reinvent the wheel, but can replace a multitude of ad hoc dialogues with a uniform and well known UI. Are there people who do not like or can not use transient? That could be a problem. I would favor 2 or 3. I feel that if we broaden the scope to include things like org-export, we will have a more complicated abstraction, with fewer suitable menu-systems, and I think the choice of different menu-systems is the point of org-menu. What do you think? Are there other options? > >> +;;; 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. I will look at this when we have sorted out the bigger things:-) > >> 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. I feel the important thing is to have a shared idea about what we want org-menu to do. No matter what the future holds for org-menu, I am very happy to have learnt a lot about programming and for the nice interaction with you Ihor (I mean, do not feel bad if the correct choice turns out to be dropping org-menu in favour of transients :-) Cheers, Tor-björn
