oantolin / embark

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

Unifying embark cycle and become #399

Closed roshanshariff closed 3 years ago

roshanshariff commented 3 years ago

Currently embark-become and embark-cycle have mutually exclusive uses:

I propose that both of these pseudo-commands be unified. Furthermore, just as is done now with embark-cycle, the cycle-or-become action can be bound to the same key as embark-act when Embark is active. This allows users to remove the B binding in embark-general-map in favour of just calling embark-act twice.

This unification is also semantically meaningful, because the two actions are analogous:

minad commented 3 years ago

This unification is also semantically meaningful, because the two actions are analogous

I think the analogy is a bit far fetched. embark-become would be more analogous to embark-cycle if it would cycle within a set of related commands, changing the type in each step. Furthermore it is odd to first act and then call the act keybinding again to "cycle".

While it is appealing to reuse keybindings for truly analogous things I think I wouldn't like this change. It is less confusing if embark-become is treated separately with its own keybinding. Anyways if you use embark-become often it deserves its own keybinding in order to speed up the command switching.

Now this cycling thought brings me to the idea that one could indeed add a embark-cycle/dwim version of embark-become, which cycles between the different possible commands, such that C-B C-B.. would cycle for example between switch-to-buffer, find-file etc.

roshanshariff commented 3 years ago

Interesting! I think we have two different mental models of how this should work. Perhaps a good question to ponder is what it would take to be able to compose Embark pseudo-commands in a flexible way.

For now, maybe one straightforward idea is to have different parent keymaps based on whether one is in the minibuffer or not (since Export, Snapshot, Live Collect, and Become make sense only in the minibuffer while Cycle is only useful at point). Then the special-case code that binds embark-cycle to the same key as embark-act is no longer needed, and one can do whatever one wishes with that key (or leave it unused).


Thinking about the differences between Become and Collect,

  1. When acting at point, there are only a few possible contexts, so it makes sense to switch between them in a cyclic way using one action repeatedly.
  2. When acting in the minibuffer, there are potentially many contexts (in general, any minibuffer-using command) so Become reuses the Emacs command loop to switch contexts, following the Embark ethos. This requires at least two steps, one to indicate that Become should take over the command loop and another to actually pick the new context.

one could indeed add a embark-cycle/dwim version of embark-become, which cycles between the different possible commands

If I may restate your wish, you would like there to be a small subset of Become-able actions (based on the current command) for which the cyclic interaction model can be used. On the other hand, my thinking is that both interactions are just switching contexts, but with different mechanics for how the context is chosen.

I will think more about a design that accommodates both these concerns (and potentially others that I haven't considered). Thanks for taking the time to explain your thinking!

Furthermore it is odd to first act and then call the act keybinding again to "cycle".

I think this just comes down to our keybindings :) I have embark-act on C-., and I think of it as "prompt for relevant action" rather than "act" (which is closer to embark-dwim on M-.). Then, the way I think, one of the relevant actions is "change context", which is accomplished by embark-cycle in regular buffers and embark-become in the minibuffer. Luckily, both have bindings available by default, C-. and B respectively, but it's not possible to give both of them the same binding.

minad commented 3 years ago

For now, maybe one straightforward idea is to have different parent keymaps based on whether one is in the minibuffer or not (since Export, Snapshot, Live Collect, and Become make sense only in the minibuffer while Cycle is only useful at point). Then the special-case code that binds embark-cycle to the same key as embark-act is no longer needed, and one can do whatever one wishes with that key (or leave it unused).

Yes, we had discussed this idea before. Maybe you will find the old discussion when searching through the tracker. The conclusion back then was to keep it simple and not distinguish these different maps. In particular since having these unnecessary bindings does not really hurt. Note that you can always invoke arbitrary commands via embark-act M-x. The keymaps are only a convenience feature. Distinguishing the keymaps leads only to consistency issues. I guess one would should not use the same key for different actions and same target type depending on the minibuffer/buffer context and vice versa. The only reason to keep the maps distinct is if you use the verbose embark indicator or the which key indicator, because then it is clearer if only a subset of the actions is shown. If you use the minimal indicator this becomes less of an issue. Back then I probably argued more in favor of distinguishing minibuffer/buffer maps (as citar is doing). But now I think my opinion is more firmly that the status quo is good enough or even better than having the distinction. Only by looking at the Embark code, duplicating all the keymaps would feel like a major redundancy.

oantolin commented 3 years ago

since Export, Snapshot, Live Collect, and Become make sense only in the minibuffer while Cycle is only useful at point

Maybe I'm weird, but I use Live Collect from non-minibuffers fairly regularly. I have a candidate collector that collects the candidates that consult-outline:

(defun collect-outline ()
  (cons 'consult-location
        (mapcar
         (pcase-lambda (`(,hd ,num "")) (propertize hd 'line-prefix num))
         (mapcar (consult--line-prefix) (consult--outline-candidates)))))

I can then use: export to get an occur buffer with the outline headings; snapshot to get a static table of contents; and, best of all, live collect to get a live updating table of contents!

I feel that I probably underuse the idea of export/collect from non-minibuffers, but even the live TOC thing is already pretty useful sometimes.

oantolin commented 3 years ago

Furthermore it is odd to first act and then call the act keybinding again to "cycle".

That's configurable, of course. For example, I have embark-act on C-; and embark-cycle on ;. I noticed that I was used to letting go of all modifiers right after embark-act because almost all the actions I use are bound to a modifierless key, so it's more comfortable for me to have embark-cycle on a modifierless key too. I reuse the semicolon, because my finger is already there.

oantolin commented 3 years ago

You know, embark-become feels completely different from embark-cycle to me. I'm not sure it makes sense to have the same UI for them.

I use embark-become when I made a mistake: I ran the wrong command and want to run the right one instead, when that happens I'm in "command running mode" and I usually run commands by using key bindings, so that's the mechanism I want to use when picking which command to become. In fact, I think I haven't even learned the bindings in the embark-become map: I tend to use the global key bindings. Plus cycling among some chosen subset of commands would feel weird to me: I think I only want to start commands I intend to run, with some mistakes maybe, but cycling through commands I don't intend to finish running just feels weird.

On the other hand target cycling for me is an expand-region sort of experience. I definitely don't want to invent and memorize keybindings for all the types of targets, so cycling is perfect. I think a lot about the number and order of the targets, and tweak things until I'm not annoyed. Maybe an alternative UI that shows the target and lets you pick one with a single key would be even better, but cycling feels pretty comfortable to me for this.

And a non-cycling embark-become would not feel analogous to embark-cycle and shouldn't share a keybinding with it.

I don't know. Maybe if I tried it I'd change my mind, but my initial reaction is that it would be odd to try to unify cycling and becoming.

minad commented 3 years ago

@oantolin

I feel that I probably underuse the idea of export/collect from non-minibuffers, but even the live TOC thing is already pretty useful sometimes.

It would be cool to have more exporters/snapshot by default in normal buffers. I don't find the ones offered currently very useful (ibuffer, dired), but maybe I missed something? Why not add the live consult-outline or live consult-imenu to embark-consult? There are multiple packages which provide this feature in a sidebar. I find it kind of amazing that we basically get this feature almost for free with Embark+Consult.

That's configurable, of course. For example, I have embark-act on C-; and embark-cycle on ;.

My statement was about the proposal to act and cycle to become. But I am aware that the cycle key is configurable. Using a modifier-less keys is a nice idea.

oantolin commented 3 years ago

I don't find the ones offered currently very useful (ibuffer, dired)

Those indeed are not very useful. I occasionally (but really, almost never), run embark-collect just to get a grid view of files, but that's it, there's no other use I can think of.

A little more useful is the candidate collector for the *Completions* buffer, because I find Embark collect buffers to be basically the same thing but more featureful. Of course, this candidate collector is only useful if you are using default completion. I went through a phase like that, but I almost never use default completion any more.

Maybe we should add the outline collector I'm using to embark-consult. We could also add an imenu collector for people who prefer that. Of course, only one gets used. Maybe a "mixed collector" that you can configure to collect outline headings in some major modes and imenu items in other modes?

Another candidate collector I've thought might be useful is one that collects all links in a eww buffer.

oantolin commented 3 years ago

My statement was about the proposal to act and cycle to become.

Whoops, sorry. Read too quickly and got mixed up, @minad.

minad commented 3 years ago

run embark-collect just to get a grid view of files, but that's it, there's no other use I can think of.

Your beloved grid view. In order to not be jealous with my poor vertico ui, I recently added https://github.com/minad/vertico/blob/main/extensions/vertico-grid.el :-P

Maybe we should add the outline collector I'm using to embark-consult. We could also add an imenu collector for people who prefer that. Of course, only one gets used. Maybe a "mixed collector" that you can configure to collect outline headings in some major modes and imenu items in other modes?

Ah right. This reminds me of the idea of having multiple different kinds of exporters. Did we discuss this before? But maybe this complicates things too much and we should just not add such a collector. The more natural starting point is to go via consult, but then we miss the live functionality...

Another candidate collector I've thought might be useful is one that collects all links in a eww buffer.

Yes, I remember that you discussed about such a collector with @protesilaos. I guess there are more collectors of that kind - the question if it makes sense to rather add consult commands for all of them which search for something.

oantolin commented 3 years ago

In the case of links, I think a consult command is better than an embark candidate collector. I'm very likely to want to just follow one link, so consult optimizes for the common case.

Basically the candidate collector is probably only necessary if you want it to auto-update (which you probably don't need for eww). (And for those collectores you probably still would use a consult command most of the time!) And it's not like we've come up with a bunch of useful ones! I think maybe it is enough to have a single default candidate collector which gives you either outline headings or imenu items, configurable per major-mode. The easy way to implement it is to reuse consult functions that gather the headings and imenu items (and flatten the structure!), so I'd add this collector to embark-consult.

roshanshariff commented 3 years ago

Thank you @minad and @oantolin for this very interesting discussion! I learned a lot and I now understand why my original proposal doesn't make a lot of sense. I also realized that embark-become works fine for any minibuffer command, even outside embark (modulo some annoyance with the which-key indicator that I'll investigate later). It would be nice to be more flexible about parent keymaps, but I read through #258 and I understand the trade-offs.

@oantolin's idea with live collect buffers is very handy! I've often wished for that with embark-outline but I didn't know enough about embark-collect-live to make it happen.

I'll close this bug now, and thanks again for your time :)

oantolin commented 3 years ago

modulo some annoyance with the which-key indicator that I'll investigate later

What problem is there with the which-key indicator. One I'm aware of, isn't really an obstacle to using embark-become but it can be confusing: if you are running a command which isn't present in any of the keymaps listed in embark-become-keymaps, then the keymap handed to the indicator contains no bindings and which-key makes it sound like an error to display such a keymap: it says something "No bindings found in Become". At that point you can still use your normal key bindings to become any command you want, but the message is a little disorienting.

Maybe the which-key indicator should detect the empty keymap case and display a friendlier message.

hmelman commented 3 years ago

The easy way to implement it is to reuse consult functions that gather the headings and imenu items (and flatten the structure!), so I'd add this collector to embark-consult.

Just checking that this hasn't happened yet. It sounds like a neat addition and I don't want it to get lost.

oantolin commented 3 years ago

I'm adding it now, @hmelman, thanks for reminding me!

oantolin commented 3 years ago

Do you think a reasonable default is that the table of contents uses consult-outline for modes deriving from text-mode and consult-imenu for modes deriving from prog-mode?

minad commented 3 years ago

Do you think a reasonable default is that the table of contents uses consult-outline for modes deriving from text-mode and consult-imenu for modes deriving from prog-mode?

Yes, and consult-org-heading for Org! Will you make it configurable?

oantolin commented 3 years ago

Yes, it will be configurable, but I only implemented imenu and outline so far. I hadn't thought of using consult-org-heading for Org. I think I have actually have never used that function! I think consult-outline works pretty well in org mode buffers.

oantolin commented 3 years ago

Having just tried it, I think consult-outline candidates make for a better table of contents than consult-org-heading candidates. Flattening the hierarchy is definitely useful for completion, but it just makes the TOC look very cluttered. Maybe I'm missing something?

minad commented 3 years ago

Okay, you are right. But imenu is also cluttered thanks to the flattening. Maybe use consult-outline in all cases, since this leads to a better live updating TOC? Then you also don't have to make it configurable :-P

oantolin commented 3 years ago

consult-outline in every mode is what I have been using in my personal configuration. But I think the flattened imenu is OK in programming modes.

oantolin commented 3 years ago

I was planning three functions: one that uses outlines, one that uses imenu, and the configurable one that uses outline in some major modes and imenu in other modes (the list of modes would be configurable). But we can start simple: I can just add the two non-configurable ones, with only the outline one added to embark-candidate-collectors.

That way people can play around with both (but to test the imenu collector, you'd need to customize embark-candidate-collectors) but nothing needs to be configurable. Then if we like the idea of having outline in some modes and imenu in other cases, we can add the third one with a configurable list of modes.

oantolin commented 3 years ago

Done, it's 5182b7eb2200a9245f1e5f73806672d6dcdd0356. I'd appreciate some testing and feedback on whether to just remove the imenu one, or add a configurable imenu/outline combo.

oantolin commented 3 years ago

The candidate collector produces consult-location items, so export will make an occur buffer.

minad commented 3 years ago

I just tried it. Snapshot and export work well, but I got an error with live collect. Is live collect supposed to work? I assumed we get some auto updating toc.

oantolin commented 3 years ago

Live collect is supposed to work. And I think it did work on my computer. I'll investigate when I'm at home again.

minad commented 3 years ago

Live collect is supposed to work. And I think it did work on my computer. I'll investigate when I'm at home again.

Never mind, it was an issue in my configuration. I still have some improvement proposals in https://github.com/oantolin/embark/pull/404.

hmelman commented 3 years ago

I'm not sure I'm following how this will work. I'd assume that if I do consult-outline and then embark-collect-live I'd get a table of contents-like buffer that used consult-outline. And if I started with consult-imenu, I'd get one using it. Is this not how it works?

minad commented 3 years ago

@hmelman The idea is different. You simply call embark-collect-live directly in the buffer and you should get a live updating TOC, with all the entries from the buffer.

In comparison, if you start consult-imenu/outline first and then call embark-collect-live/snapshot the resulting Embark collect buffer reflects the list of minibuffer entries. As soon as you close the minibuffer it won't update anymore.

hmelman commented 3 years ago

Hmmmm, as someone that uses outline-minor-mode in programming modes a lot, I have to think if I'll always want a TOC generated from imenu instead of outlines. It's probably fine, but I do include extra stuff in outlines, e.g., ;;; and ;;;; comments in lisp that tend to indicate sections which would be really nice, but also other things like ;;;###autoload that I wouldn't like in a TOC (I include them in outlines since they indicate the entry-point functions).