oantolin / embark

Emacs Mini-Buffer Actions Rooted in Keymaps
GNU General Public License v3.0
943 stars 56 forks source link

Use menu-items to restrict actions to the minibuffer/buffer-only #406

Closed minad closed 1 year ago

minad commented 3 years ago

There was this long discussion with @roshanshariff and @bdarcus about cluttered action maps. I wondered if it makes sense to use menu items to introduce a context dependency additionally to the target type. I don't have a concrete proposal - only putting this up for discussion. One could extend embark-define-keymap to support definitions like this:

(embark-define-keymap embark-general-map
  "Keymap for Embark general actions."
  :parent embark-meta-map
  ...
  ("SPC" mark :buffer-only) ;; or some alternative syntax...
  ...)

Such a feature would allow fine tuning for some special commands. I don't think one should overuse this, but it may help in some cases and help third-party packages like Citar. Such a feature would require better menu-item support (#383).

Another alternative idea is to add embark-buffer-only-actions, embark-minibuffer-only-actions (or an alist with values buffer/minibuffer) customizable variables, which are used to filter the keymaps. Maybe that is more in line with the Embark design, where the keymaps are relatively pristine and additional special features are fine-tuned via separate customizable variables.

In any case, I think it is better to not duplicate keymaps, e.g. create separate embark-symbol-in-buffer and embark-symbol-in-minibuffer keymaps. This is a pattern which is currently used in Citar and there is a discussion if this can be avoided (https://github.com/bdarcus/citar/issues/381).

bdarcus commented 3 years ago

Another alternative idea is to add embark-buffer-only-actions, embark-minibuffer-only-actions (or an alist with values buffer/minibuffer) customizable variables, which are used to filter the keymaps. Maybe that is more in line with the Embark design, where the keymaps are relatively pristine and additional special features are fine-tuned via separate customizable variables.

I don't have a strong opinion, but this seems likely the case.

Would allow me to keep the standard keymaps, but reduce them to one.

roshanshariff commented 3 years ago

An alternative way to accomplish this is to allow the entries in embark-keymap-alist to be functions returning keymaps rather than just symbols whose value is a keymap. Maybe that function can even be passed the current target. The function can then use minibufferp, look at the current major mode, etc. to modify what it returns. To make it easier to implement this function, Embark can provide a utility function to filter out commands from a keymap based on a predicate (and of course other packages can implement whatever logic they like).

I haven't considered the pros and cons of doing it this way, just proposing an alternative.

minad commented 3 years ago

@roshanshariff That would work too. Alternatively Embark could have a customizable hook variable embark-keymap-filter-functions. The functions are called on the finished keymap in order to perform custom filtering. But I think I prefer a less general solution, a solution specifically tailored for the problem at hand. We can generalize later on if there is a need.

As a simple example, one may want to filter out the mark action in the minibuffer. Similarly in Citar you also have only a single or maybe two actions you want to filter out.

I should note that I don't really bother too much with the existence of unneeded actions in the keymaps - ideally a solution should be minimally intrusive and not require huge changes for a marginal improvement. In my opinion this precludes solutions like using multiple keymaps for different buffer contexts (minibuffer vs buffer), configurable dynamic keymap building based on the target, which is like an additional second dispatching mechanism. A simple regexp/symbol based filter like embark-verbose-indicator-excluded-actions should do - it also has advantages for customization.

The other idea to use menu-items seems quite ugly. But the point is that you can already put menu-items in keymaps if you want to - so I wonder if one could simply use this feature and make Embark cope with it, without really adding additional machinery to Embark. But in terms of overall complexity it may come close or even exceed such a functionality which builds keymaps dynamically.

roshanshariff commented 2 years ago

But in terms of overall complexity it may come close or even exceed such a functionality which builds keymaps dynamically.

Yeah, I don't think it'll be common to build keymaps dynamically, and even if we have functions returning keymaps I mostly see it being used just to filter keymaps (if actually needed; as you say, it won't be most of the time).

But I have a different perspective on "generality". I see Emacs code as being a very nice medium to express configuration. We have eldoc, xref, tracing tools, a debugger, etc. to deal with it. Often you can get both simplicity and generality by using code-as-configuration rather than having lots of special-purpose data structures that are examined by static code. You just provide basic functions to do something (e.g. filtering keymaps in this case) and let users (end-users or third party packages) compose them together as needed.

The result is often much simpler than anticipating particular use cases and providing exactly the knobs and switches needed to accomplish them (which tend to accumulate and get complicated over time). As an example in Emacs itself, I would point at programmable completion and how it made it possible to create new completion tables, compose them together, extend them, etc. much beyond the "basic" completion that was probably the original vision. Or even the idea of an init file rather than a list of customized variable values.

But this is just an aesthetic preference, so I'll leave it at that :)

minad commented 2 years ago

@roshanshariff I see your point. My opinion is different. I think it is sometimes better to have more restrictions, think static typing vs dynamic typing. Since elisp is already excessively flexible and dynamic it can be helpful to go with the simpler solution. Using a simpler solution will guide users in how to use the functionality. Your example of dynamic completion tables is in fact a bad one, completion tables are under specified which created many problems down the road. But there is a large spectrum of course to where such questions apply, so it should be decided on a case by case basis. Overall I think Embark belongs firmly into the camp of quite general Emacs utilities.

The dynamic keymap building facility you proposed is overly general for multiple reasons: You said yourself that it won't be used often. As already mentioned a list is much easier to use and customize (also via the customize interface). I am not talking anyone into using the customization interface, it is not that great for actual customization, but it is not that bad for exploration of the various knobs. Since the keymaps are created dynamically, this adds a new mechanism on top of the already existing dispatching mechanism of Embark - so it overlaps with some existing functionality. I prefer orthogonality of features.

This is all debatable. Having a separate variable like embark-action-context-alist at least seems to fit the current code base. There are many examples of variables where per-action behavior is configured.

In order to convince me I would probably have to see multiple features, which can be replaced by the single feature of dynamic keymap builders. But even if one has these dynamic builders one will probably still end up with a list of actions which are to be filtered out depending on context like some embark-action-context-alist, such that the dynamic keymap building is only an implementation detail.

Hugo-Heagren commented 2 years ago

I don't know if anyone has made any progress on this since the original conversation, but I'd thought I'd add a recent experience. I was writing up some notes for an idea I had about a general "org-headline-act" command, which could run down an alist of (PREDICATE . FUNCTION) pairs, and funcall the first function for which the predicate matched. The alist would be a custom var, so users could define their own pairs (for example, I have a lot of zoom calls in my agenda. I could just add the zoom ID as a :ZOOMID: property, write a predicate which checks for that property, and a function which gets the value, and opens a zoom meeting with that ID). Of course, most of these functions will be irrelevant most of the time -- I have enough zoom meetings to worth coding this, but the majority of my org headings are not zoom meetings. This was the reason for the predicates -- I need only use the relevant action.

But then I realised that sometimes more than predicate/action pair might apply and so I would need a command which presented them all and let the user select one by pressing a key.

This is starting to sound a lot like embark-dwim and embark-act. The only difference is that which actions are presented in an embark promp cannot be controlled by a predicate. So for this usecase (and perhaps more in the future) I for one would find a solution like this very useful -- ideally one which let me embed the predicates into the definition of the embark keymap itself (such as menu-items, though I agree that working with them is quite ugly). Ideally for me, it should also be possible to specify context predicates in embark-default-action-overrides, so that the default action is overriden just if that predicate returns non-nil (this would make acting on my zoom headings really easy!).

And then I could just add embark-dwim to org-ctrl-c-ctrl-c-hook, and everything would work perfectly!

minad commented 2 years ago

@Hugo-Heagren Well, I think the Embark way of solving your problem is to introduce a special target type for zoom headings. I don't think we need predicates on top for this use case.

oantolin commented 1 year ago

Maybe we can revisit this now that I finally added support for menu-items and their :filter properties.

minad commented 1 year ago

@oantolin I am not so fond anymore of the idea. Maybe introducing a predicate alist for actions could be an alternative. But I don't think I need any of this.

oantolin commented 1 year ago

My current opinion is that it is good to have support for menu-item and :filter just so embark can use existing keymaps that might happen to use that, such as some keymaps from lsp. But I also have never missed this feature and don't think I'll be adding any menu-items with fancy :filter predicates to either embark's default configuration or to my personal configuration.

oantolin commented 1 year ago

I'm closing this issue: I don't think we really need it. People that feel strongly about it can already do it in their configuration: you can always define narrower target finders and smaller action keymaps, or, now with menu-item :filter support, you can keep the current target finders and keymaps and just change the bindings to menu-items that filter out the bindings you don't want. I don't really see myself separating the minibuffer general actions from the regular buffer general actions (there are very few I don't use in both places, at least occasionally), but as always, we can continue the discussion.