search

oantolin / embark

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

prevent auto closing live-occur #53

Closed minad closed 3 years ago

minad commented 3 years ago

As I wrote I wonder if occur and live occur could be unified. If not (you probably have good reasons to keep them separate?), it would still be good to have some option which prevents closing the live occur buffer. Like having an extra function embark-live-occur-noclose. Does that make sense?

oantolin commented 3 years ago

Do you find you almost always want to keep the live occur after closing the minibuffer? If so, I understand the current system is annoying. We'll figure something out in that case. If, on the other hand, you only occasionally want to keep the live occur buffer around, just use embark-occur (or embark-export, if you prefer) to "take a snapshot" of the live occur buffer when it is in the state you want to keep. You can run embark-occur either from the minibuffer or from the live occur buffer.

I'm happy to discuss this further, maybe there is some unified behavior that would work for everyone. Let me describe how I use the two commands. First, I actually use embark live occur as my completion system instead of selectrum or icomplete-vertical. For me, every single minibuffer completion session opens up an embark live occur buffer, so I definitely want them to be ephemeral, or I'd be drowning in *Embark Live Occur*<78> buffers. I use embark-occur, as I described above, to take a snapshot of the current state of the live occur buffer if I want to keep it.

So there are two distinction the current system conflates: live vs snapshot, and ephemeral vs permanent. All four combinations make at least some sense, but currently embark only has embark-live-occur which is live and ephemeral, and embark-occur which takes a permanent snapshot. You are asking for live but permanent (of course, if stops being live when you close the minibuffer, but that's reasonable).

I think the ephemeral snapshot version doesn't sound very useful, does it? So maybe we are only missing a live but permanent command.

minad commented 3 years ago

Well, I am mostly interested in simplifying and unifying behavior if it is possible. But as you know, I am not familiar with the way occur and actions work in ivy/helm, so you should take all my suggestions with a grain of salt.

I agree with the two axes of the distinction and I wonder if it is possible to simplify somethings in the code base, since the live-occur buffer somehow seems to subsume the snapshot behavior (if you disable the update) and it subsumes the permanent behavior (if you disable the kill).

Right now my overview of the code base is still a bit superficial but it looks as if you have occur and live occur as somehow separate mechanisms. If we replace this by one general mechanism and replace all possible (and useful) combinations it may be an improvement?

oantolin commented 3 years ago

Mmh. They don't feel that separate to me, the common bits are handled by embark-occur-noselect. I guess they could be unified further.

minad commented 3 years ago

Mmh. They don't feel that separate to me, the common bits are handled by embark-occur-noselect. I guess they could be unified further.

I will take a look, as I said - I am not yet that familiar with that part of the code. Maybe we should separate the two discussions here:

minad commented 3 years ago

I wonder if it is possible to switch of the active completion system when going to live occur such that you get live-occur+default-completion. This would be very helpful for example for the consult yank commands, when browsing multiline candidates.

oantolin commented 3 years ago

You mean in a completion system agnostic way? That sounds hard. It would be good, though. We can definitely do it for, say, icomplete and selectrum. I doubt users of other completion systems would be using embark-live-occur anyway.

minad commented 3 years ago

It would be worth a try. I see the appeal of using live occur and its more verbose candidate lists for multiline candidates, but for quick interactions I very much prefer the faster selectrum/icomplete style. This would give me the best of both worlds :)

Regarding an agnostic implementation, I was thinking about something like this: First jump out of the minibuffer section, like an exiting action, then restart the completion and somehow call directly into the default completion system (completing-read-default), but this will certainly work only if the overwriting completion system does not do other manipulations. I am pretty sure it won't work due to minibuffer hooks etc. Otherwise one could go with the uglier solution of detecting and disabling/enabling the active completion system.

minad commented 3 years ago

If something like this becomes available, this would solve all the occur needs I have. I would switch to the live+persistent occur completion, narrow as long as I want, explicitly close the minibuffer and explicitly close the occur buffer when done. For normal stuff I would continue using icomplete/selectrum style completion.

oantolin commented 3 years ago

I would switch to the live+persistent occur completion, narrow as long as I want, explicitly close the minibuffer and explicitly close the occur buffer when done.

Except for automatically switching off selectrum, you can have the rest of this workflow today! Use embark-live-occur, narrow as long as you want, and when you want to close the minibuffer run embark-occur, explicitly close the occur buffer when done.

oantolin commented 3 years ago

It's exactly what you said, except that you said "explicitly close the minibuffer" without saying how you wanted to do that, I'm saying your method of closing the minibuffer can be to run embark-occur.

minad commented 3 years ago

Yes, that's right, probably this is good enough. I think I would still prefer my proposal for my default use case since I don't have to go "minibuffer-completion -> live-occur -> occur", but this is certainly not a strong argument.

Regarding the live-occur switching off the default completion system, maybe this is better solved outside of embark via wrapping embark-live-occur, then you don't have to pollute embark with icomplete/selectrum specifics?

minad commented 3 years ago

I think my concerns regarding live-occur vs occur are more based on the thought why are those distinct - why is there not just this one occur thing and I can configure it, persistent/ephemeral and live/snapshot.

oantolin commented 3 years ago

Yes, that's right, probably this is good enough. I think I would still prefer my proposal for my default use case since I don't have to go "minibuffer-completion -> live-occur -> occur", but this is certainly not a strong argument.

I'm not opposed to adding a live+persistent variant too. I'd like to do it by refactoring to clearly separate liveness and persistence. I'll look into it.

Regarding the live-occur switching off the default completion system, maybe this is better solved outside of embark via wrapping embark-live-occur, then you don't have to pollute embark with icomplete/selectrum specifics?

Yeah, maybe that's for the best. This could be documented on the wikis (both embark and selectrum, I think).

oantolin commented 3 years ago

I think my concerns regarding live-occur vs occur are more based on the thought why are those distinct - why is there not just this one occur thing and I can configure it, persistent/ephemeral and live/snapshot.

Yes, this would be good.

oantolin commented 3 years ago

I wonder if it is possible to switch of the active completion system when going to live occur such that you get live-occur+default-completion. This would be very helpful for example for the consult yank commands, when browsing multiline candidates.

By the way, the consult-yank commands now start in "zebra mode" by default. Alternate kill-ring entries are highlighted, so you can clearly see where each starts and ends.

oantolin commented 3 years ago

This is closer now, I've done a little bit of cleanup of embark-occur and embark-live-occur and they are more similar than before. I'm not sure what the best interface is. I think there are actually three binary choices: live/snapshot, persistent/ephemeral, exiting/non-exiting. Not all 8 combinations make sense, for example, live+exiting is pretty silly. Also snapshot+ephemeral+exiting is pointless, since you don't get to use the buffer, it's just like pressing C-g. So that still leaves 5 possibilities. But, for example, while snapshot+ephemeral makes sense (it's kinds of like the built-in *Completions* buffer), I think nobody is asking for that version.

I'm contemplating the following design:

minad commented 3 years ago

From my pov it would be great if we just get the backend somehow flexible, such that it takes two boolean flags live/snapshot, persistent/ephemeral. The interface on the frontend could be discussed next. Are we already there? :) I think it would also be nice to offer this as some kind of semi-public api, users could write their own wrappers which set the flags accordingly.

  1. Get rid of the ephemeral versions! embark-live-occur would behave like you want it to: live and persistent.

If you also do 3., that would be okay I think.

  1. Make embark-occur exit agnostic: make it so calling it from embark-act exits and from embark-act-noexit does not exit. I'd have to choose what happens if you call it directly, not as an action.

Sounds good.

  1. For use as a completion system, arrange for a single Embark Completions buffer that is recycled (I think that's what the built-in Completions buffer and also Helm do).

:+1:

oantolin commented 3 years ago

From my pov it would be great if we just get the backend somehow flexible, such that it takes two boolean flags live/snapshot, persistent/ephemeral. The interface on the frontend could be discussed next. Are we already there? :)

Not yet, but it's not hard to do now.

Also, I just remembered what I didn't like about recycling an *Embark Completions* buffer: if you use recursive minibuffers it can be nice to have access to the completions of all the parent minibuffers, so instead of recyling the Live Occur buffer I took to making numbered ones and killing them. It's not a big deal though, I sort of like the possibility of seeing the completion lists for parent minibuffers but I never actually use it. 😄

minad commented 3 years ago

I think it would be nice to annotate the *Embark Completions* with their recursion level, e.g., *Embark Completions Level 2*.

oantolin commented 3 years ago

I think it would be nice to annotate the *Embark Completions* with their recursion level, e.g., *Embark Completions Level 2*.

That's what happens right now, but it says *Embark Live Occur*<2>.

minad commented 3 years ago

That's what happens right now, but it says Embark Live Occur<2>.

Right, this happens since your live-occur auto closes such that the only buffers around are the ones of lower levels. I would like to keep this behavior then ;)

oantolin commented 3 years ago

Now that I'm close to doing this, maybe it's worth discussing the name? There was some talk of not liking the occur name, and I think I suggested embark-collect. Is that name OK? I'd probably keep the other names as obsolete aliases for a while.

minad commented 3 years ago

I discussed this with @hmelman. I think collect is okay, but I got more accustomed to occur by now :laughing:

hmelman commented 3 years ago

I agree that embark-occur isn't obvious at first what the command is used for, but given that embark-export is already used (and for a command that perfectly matches its intent) and this aligns with with ivy-occur and helm-occur do, I wouldn't change embark-occur now unless you come up with a name you think is really great.

oantolin commented 3 years ago

I don't like embark-occur that much, because it's not really like M-x occur, but as you mention @hmelman, I wanted the name to be familiar to ivy-occur users. I believe helm-occur is more correctly named, in the sense that it is a substitute for M-x occur. I'm not sure if Helm has general ivy/embark-occur functionality, but if it does have it, it is not called helm-occur.

I think embark-collect is a better name, but I also do not feel very strongly about it and worry that renaming might cause more trouble than its worth.

EDIT: I was missing "do not" in the previous sentence. 😆

oantolin commented 3 years ago

Ah, so I maybe Helm's version of ivy-occur is that it stores a history of all Helm sessions and provides a command helm-resume to access them. By default helm-resume resumes the last session, but with C-u it can resume any past session.

minad commented 3 years ago

Let's rename it! It will be okay! This ecosystem is new so we can also correct things. In particular if ivy and helm are already differ in its usage!

oantolin commented 3 years ago

OK! That trio of exclamation marks you used convinced me, @minad. For several weeks, I'll use define-obsolete-function-alias to keep from breaking people's key-bindings if they made any, and bind embark-collect to both O and C in embark-general-map. (Or rather bind embark-collect to C and keep the O binding for what will become an obsolete funcion alias.)

minad commented 3 years ago

❗

protesilaos commented 3 years ago

I am using Embark full time and follow almost all the commits, but I do not track the issues here---sorry for not participating as much as I could!

minad: I think it would be nice to annotate the Embark Completions with their recursion level, e.g., Embark Completions Level 2.

oantolin: That's what happens right now, but it says Embark Live Occur<2>.

I think this is a very interesting concept. The numbering scheme does not scale well because it offers no hint as to what its contents are. Whereas, say, *Embark Collection for <USER INPUT>* would be unambiguous. Other variants: *Embark {Dired,Grep,Ibuffer} for INPUT*.

[ this is a general problem with uniquify, e.g. for Info buffers ]

That would increase the value proposition of the "snapshot" utility, however it may be called (and I do use that, by the way, such as when I want to read and re-read the doc strings of certain functions).

On the topic of refactoring Embark occur, I am fine with it. For me what matters is that we can have the functionality of (1) the "live completions" that respond to minibuffer input, while (2) retaining the option to store a snapshot in a regular buffer.

oantolin: Now that I'm close to doing this, maybe it's worth discussing the name? There was some talk of not liking the occur name, and I think I suggested embark-collect. Is that name OK? I'd probably keep the other names as obsolete aliases for a while.

[...] I don't like embark-occur that much, because it's not really like M-x occur [...]

In principle I agree and think that "collect" is semantically more appropriate. What about key bindings though? C-o and M-o are fine as embark-occur mnemonics for the minibuffer. M-c would override the capitalize command that might be useful in some cases (especially if a user opts for case-fold-search nil). C-c is a prefix...

Obviously we can still use whatever we want. Just noting this.

oantolin commented 3 years ago

Whereas, say, *Embark Collection for <USER INPUT>* would be unambiguous. Other variants: *Embark {Dired,Grep,Ibuffer} for INPUT*.

That's a good idea, but I am a bit reluctant to do it because it only makes sense when embark-occur (or soon embark-collect) is called from the minibuffer. Maybe that's such a common case it does deserve special treatment, though.

oantolin commented 3 years ago

What about key bindings though?

I'm not too worried about the key bindings, because the way I'm currently "advertising" in the readme is to run it as an action. embark-collect-snapshot would be bound to C in embark-general-map.

oantolin commented 3 years ago

OK, an initial implementation is ready in the unified-collect branch! @protesilaos @minad

Big changes:

Some things remain to be done before merging:

Please test.

minad commented 3 years ago

I am happy with this - looks great! Makes sense to exclude the snapshot/ephemeral combination. And I like the choice of names snapshot/live/completions.

Figure out if there is a better way to do the following: the :snapshot and :live buffers are meant to survive the current minibuffer and they do. Should the window they are displayed in also survive. I kind of think so, but that window disappears naturally when the minibuffer exits. My current work around is to redisplay the window in the minibuffer-exit-hook. Maybe there's a better way.

Could you explain that a bit better - why is the window naturally disappearing? Because the minibuffer exit restores the layout somehow? But redisplaying the window/buffer in the end does not sound too bad.

I will test this soon!

oantolin commented 3 years ago

Here's a simple example of the disappearing window problem, @minad. Evaluate this:

(defun example ()
  (interactive)
  (let ((buf (get-buffer-create "*Example*")))
    (with-current-buffer buf
      (insert "Hello!\n"))
    (display-buffer buf)))

(define-key minibuffer-local-map (kbd "C-M-e") #'example)

Then run a command that uses the minibuffer, type C-M-e while the minibuffer is open, and upon exit the window showing the *Example* disappears. Even if the command you run shouldn't do anything to the windows, for example try: M-x C-M-e pwd RET.

minad commented 3 years ago

Yes, I think this is due to the recursive editing state which is entered by the minibuffer. I think it is okay to just restore the buffer in the minibuffer-after-exit-hook.

protesilaos commented 3 years ago

oantolin: [on buffer renaming to include input] That's a good idea, but I am a bit reluctant to do it because it only makes sense when embark-occur (or soon embark-collect) is called from the minibuffer. Maybe that's such a common case it does deserve special treatment, though.

It seems that it only adds value to the "snapshot" collection and to the exported ones (maybe you export two sets of files and need to know what each set's common root is).

I guess it is a niche, so it really is a matter of whether you want to assume the burden of developing/maintaining the extra code.

I think it would be nice to have, but ultimately trust your judgement.

oantolin: I'm not too worried about the key bindings, because the way I'm currently "advertising" in the readme is to run it as an action. embark-collect-snapshot would be bound to C in embark-general-map.

Yes, that is okay.

oantolin: Big changes [...] Please test.

I will pull the changes now and give it a try right now.

EDIT: too eager with the "now".

oantolin commented 3 years ago

Oh, another thing needed before merging is putting in obsolete aliases for the occur names.

minad commented 3 years ago

@protesilaos

It seems that it only adds value to the "snapshot" collection and to the exported ones (maybe you export two sets of files and need to know what each set's common root is).

You are talking about buffer names, right? I think names also matter for the live buffers since one can recursively create new sessions and one can jump out of the minibuffer.

protesilaos commented 3 years ago

You are talking about buffer names, right? I think names also matter for the live buffers since one can recursively create new sessions and one can jump out of the minibuffer.

Ah yes, of course!

oantolin commented 3 years ago

I'd really like better buffer names too. In preparation for that I eliminated a couple of places where there was some strign matching of buffer names (not my best code, that)...

We could leave the non-minibuffer case alone, since I don't think many people even know you can run embark-occur/collect in a non-minibuffer.

protesilaos commented 3 years ago

With regard to the obsolete calls, is something like this okay?

(define-obsolete-function-alias
  'embark-target-occur-candidate
  'embark-target-collect-candidate
  "0.10")

I can just help you type them out by going through the diff.

oantolin commented 3 years ago

I think the main thing is deciding which of the commands and variables really need an obsolete alias. Maybe we can get away with not too many of them.

hmelman commented 3 years ago

We could leave the non-minibuffer case alone, since I don't think many people even know you can run embark-occur/collect in a non-minibuffer.

You can? What does it do? I just tried (with the melpa release) and it opened an empty embark-occur buffer.

Maybe we can get away with not too many of them.

Just end up consistent. If the term is collect, all the variables and commands (and their docs) should use it. And if that's the case, then all the internal functions will be clearer if they do too.

minad commented 3 years ago

@oantolin I am still renaming things in consult without obsolete aliases, but I haven't tagged a release yet :)

oantolin commented 3 years ago

We could leave the non-minibuffer case alone, since I don't think many people even know you can run embark-occur/collect in a non-minibuffer.

You can? What does it do? I just tried (with the melpa release) and it opened an empty embark-occur buffer.

Same as it always does: if you configured a candidate collector for the situation, it collects the candidates and puts them in a buffer. The only built-in candidate collectors are (1) for the minibuffer, collect the completions, (2) for dired buffers, collect the file, (3) for ibuffer, collect the buffers, (4) for a *Completions* buffer, collect the completions, (5) for an Embark Occur buffer, collect the entries.

So if you want a non-empty, non-minibuffer example, try using embark-occur from dired. I actually do do that sometimes, because I like the grid view.

But it's really mainly meant to be extended. I gave a cute example on reddit, where I configured occur in an Emacs Lisp buffer to collect all the bound symbols, for example.

Just end up consistent. If the term is collect, all the variables and commands (and their docs) should use it. And if that's the case, then all the internal functions will be clearer if they do too.

Oh, occur is gone in that branch! Everything is consistently collect. Here we were just talking about keeping some old occur names around aliased to the new collect names.

hmelman commented 3 years ago

Those are interesting occur examples. Nice.

IMHO, add all the obsoletes for anything not internal (--). Then remove them all in a few months (seems reasonable given how fast these packages are changing).

protesilaos commented 3 years ago

I am working on the obsolete part, by the way. Then you can check it and decide what to keep.

(I would read the diff anyhow, because I must prepare for the changes).

oantolin commented 3 years ago

Another cool example that @protesilaos suggested, but I haven't implemented yet, is that for eww buffers, you could collect all the links, @hmelman.

I agree with aliases for all non-internal things, except the one variable that disappeared, I guess.

oantolin commented 3 years ago

Thanks @protesilaos, that'll be a big help!