Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create occur buffer in background on minibuffer exit #85

Closed
clemera opened this issue Jan 5, 2021 · 62 comments
Closed

Create occur buffer in background on minibuffer exit #85

clemera opened this issue Jan 5, 2021 · 62 comments

Comments

@clemera
Copy link
Contributor

clemera commented Jan 5, 2021

When discussing selectrum-repeat it occurred to me that it may be nice to have an automatic occur buffer created on minibuffer exit which would basically replace the functionality of such a command. This way you could always go back to your last results. What do you think?

@minad
Copy link
Contributor

minad commented Jan 5, 2021

It would also be the perfect reason to why I wouldn't want to implement a consult-resume command. See also minad/consult#6 (comment).

@oantolin
Copy link
Owner

oantolin commented Jan 5, 2021

Does (add-hook 'minibuffer-exit-hook 'embark-occur) work or does the hook run a little to late? I'll test later when I'm at the computer.

@minad
Copy link
Contributor

minad commented Jan 5, 2021

(add-hook 'minibuffer-exit-hook 'embark-occur)

It works 🥳 But it is not particularily useful this way since you will always get these buffers opening. The idea is more to have some kind of hidden buffer and only a single one for the last minibuffer session. This buffer could then be renamed and made visible by some embark-resurrect command!

@minad
Copy link
Contributor

minad commented Jan 5, 2021

Oh, and the slowdown is annoying if you implement it that way, since the marginalia annotators are executed always. In order to support this properly, only the un-annotated candidate list should be stored and then copied to an occur buffer and annotated by embark-resurrect. It seems this is not a feature we can get for free. But I still think Embark is the right place for this. Certainly better than having a selectrum/consult-repeat/resume command!

@oantolin
Copy link
Owner

oantolin commented Jan 5, 2021

The occur buffer could be requested in grid view, which runs no annotator, and then be put in the correct initial view for the type when resurrected.

@oantolin
Copy link
Owner

oantolin commented Jan 5, 2021

We could also just store the information needed to make the buffer and not actually make it until it is time to ressurect. That's slightly more work, but probably better.

@minad
Copy link
Contributor

minad commented Jan 5, 2021

The occur buffer could be requested in grid view, which runs no annotator, and then be put in the correct initial view for the type when resurrected.

Right!

We could also just store the information needed to make the buffer and not actually make it until it is time to ressurect. That's slightly more work, but probably better.

This is what I would like! I would prefer the most efficient implementation, since this will be executed every time and in almost all cases it will be executed unnecessarily. If the resurrect feature would result in any noticeable slowdown, I probably wouldn't want to have this.

@minad minad mentioned this issue Jan 5, 2021
20 tasks
@oantolin
Copy link
Owner

oantolin commented Jan 5, 2021

How about the name embark-occurred?

@oantolin
Copy link
Owner

oantolin commented Jan 5, 2021

Also, instead of storing the candidates and later making an occur buffer, we could store the minibuffer input and rerun the command. That feels more like what Ivy resume does.

@oantolin
Copy link
Owner

oantolin commented Jan 5, 2021

The version we discussed first would be an embark-occur after the fact, the last thing I proposed would be Embark-become after the fact, becoming the same command again.

@oantolin
Copy link
Owner

oantolin commented Jan 5, 2021

Maybe it even makes sense to have both.

@minad
Copy link
Contributor

minad commented Jan 5, 2021

How about the name embark-occurred?

I like to have a different name for faster completion when typing it. Furthermore I think more distinct names are better.

Also, instead of storing the candidates and later making an occur buffer, we could store the minibuffer input and rerun the command. That feels more like what Ivy resume does.

Okay, this works too, and maybe this is even better, since you can always get an occur buffer later. This is basically ivy-resume for free? I would then vote to have this! If we are going with embark-occur, we could then simply name it embark-resume.

@oantolin
Copy link
Owner

oantolin commented Jan 5, 2021

I like to have a different name for faster completion when typing it. Furthermore I think more distinct names are better.

Good point.

@clemera
Copy link
Contributor Author

clemera commented Jan 5, 2021

Also, instead of storing the candidates and later making an occur buffer, we could store the minibuffer input and rerun the command.

Hm, this sounds more difficult as a command could prompt you multiple times, how would that be handled?

@minad
Copy link
Contributor

minad commented Jan 5, 2021

Hm, this sounds more difficult as a command could prompt you multiple times, how would that be handled?

I think it is more important to handle the common case of one completion. Does the default action even work if you have multiple completions?

@oantolin
Copy link
Owner

oantolin commented Jan 5, 2021

Also, instead of storing the candidates and later making an occur buffer, we could store the minibuffer input and rerun the command.

Hm, this sounds more difficult as a command could prompt you multiple times, how would that be handled?

By ignoring the issue! This is what embark-default-action and embark-become already do and I haven't received any complaints. 😆

@minad
Copy link
Contributor

minad commented Jan 5, 2021

By ignoring the issue! This is what embark-default-action and embark-become already do and I haven't received any complaints. laughing

This is exactly how I would have "solved" this too! 👍

@oantolin
Copy link
Owner

oantolin commented Jan 5, 2021

embark-default-action means "take this completion from the nth time the command prompted you, ignore the value of n, rerun the same command, and inject the target at the first prompt, exit minibuffer".

@minad
Copy link
Contributor

minad commented Jan 5, 2021

Yes, and by doing exactly the same we get embark-resume almost for free. I like this very much! It is a hack but it will work in most cases. Having multiple subsequent completion sessions is not common. One more reason to have consult-async/consult-grep implemented as is, with a single completing-read session.

@clemera
Copy link
Contributor Author

clemera commented Jan 5, 2021

Not multiple completions sessions but read-string and then completing-read or stuff like that can happen, I have seen it ;) But I'm okay with this, too maybe not worth to complicate it.

@minad
Copy link
Contributor

minad commented Jan 5, 2021

We could also consider solving this for real, recording the sequence of completing-read selections and injecting them in order. It wouldn't be difficult to do this I guess. But this discussion is orthogonal to the embark-resume discussion. If we get embark-resume based on default action it will be fine for now and if we decide to fix the multi completing-read thing later, it will automatically work!

@oantolin
Copy link
Owner

oantolin commented Jan 5, 2021

Good. I'll implement the cheap version later today. I think I'll call it embark-repeat rather than resume, because the built in commands are called repeat and repeat-complex-command

@clemera
Copy link
Contributor Author

clemera commented Jan 5, 2021

Sounds great, thank you!

oantolin added a commit that referenced this issue Jan 6, 2021
This probably needs a little refinement to make it convenient:

- Maybe keep a ring of the last 10 commands and inputs?
- This records every command that uses the minibuffer, including, for
  example M-x. Maybe some commands should be skipped.
@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

That was easy. It works, but maybe it needs a little design work.

  • Maybe keep a ring of the last 10 commands and inputs?

  • This records every command that uses the minibuffer, including, for example M-x. Maybe some commands should be skipped.

Play around with it, let me know what you think.

@clemera
Copy link
Contributor Author

clemera commented Jan 6, 2021

When repeating a session where you accepted the candidate the input isn't restored but the last submitted candidate. Is the idea of saving the candidates and restoring an occur buffer from it abandoned? I liked the idea that you just would need to switch to the buffer (no additional keybinding to remember). If it is to slow maybe the buffer could be created via an idle timer so it would be hard to notice and the exit wouldn't be slowed down.

When the minibuffer session gets restored there might also be the problem that there is additional state (filtering method) that isn't restored which was the reason for the Selectrum issue I linked to. Maybe we could also combine both and the session could be restored from the occur buffer? Maybe this could even be a general feature for occur buffers?

Maybe keep a ring of the last 10 commands and inputs?

That sounds cool, could be useful from time to time.

This records every command that uses the minibuffer, including, for example M-x. Maybe some commands should be skipped.

Not sure about that, the purpose for me would be to automatically remember the last thing I did and go back to it.

@kljohann
Copy link

kljohann commented Jan 6, 2021

One minor nit: When Emacs has just started, the "last-command-and-input" is nil and the current implementation tries to (command-execute nil). This is due to the following peculiarity of pcase-let:

Each EXP should match (i.e. be of compatible structure) to its
respective PATTERN; a mismatch may signal an error or may go
undetected, binding variables to arbitrary values, such as nil.

I think there are two sensible fixes:

  ;; either emit a verbose message and exit:
  (unless embark--last-command-and-input
    (user-error "No last command"))
  (pcase-let ...)

  ;; or silently do nothing by e.g. replacing pcase-let:
  (when-let ((cmd (car embark--last-command-and-input))
             (input (cdr embark--last-command-and-input)))
    ...)

@minad
Copy link
Contributor

minad commented Jan 6, 2021

When repeating a session where you accepted the candidate the input isn't restored but the last submitted candidate.

Yes, this would be better! @oantolin Any chance to make this work? This certainly needs some special casing! Using just the selected candidate is clearly not what is needed for a proper embark-repeat. It should also not trigger the default action, but just start the session without the RET after the injection of input. See also what I write below regarding history management!

Is the idea of saving the candidates and restoring an occur buffer from it abandoned? I liked the idea that you just would need to switch to the buffer (no additional keybinding to remember). If it is to slow maybe the buffer could be created via an idle timer so it would be hard to notice and the exit wouldn't be slowed down.

No, I wouldn't like this. This sounds like an ugly hack for something simple (instead only the candidates should be stored quickly as a list). But I am in favor of abandoning this idea and instead focus on embark-repeat with the input!

Maybe keep a ring of the last 10 commands and inputs?

That sounds cool, could be useful from time to time.

Somehow I have the feeling that this is starting to overlap with the input history functionality, radian-software/selectrum#185 and minad/consult#137. I think we should get back to the drawing board instead of rushing to a solution. Maybe better to revert 622f6ea for now? Maybe this whole history management should go into Embark?

@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

Wow, differing timezones means there's lots of stuff for me to read now! 😄

@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

@clemera:

When repeating a session where you accepted the candidate the input isn't restored but the last submitted candidate

Damn, I didn't even notice! Technically the candidate is the last input, depending on how you exit. For example, minibuffer-force-complete-and-exit modifies the minibuffer contents so it includes the full candidate and then exits. You are right that instead of the last input we should store "the last minibuffer contents before RET".

This records every command that uses the minibuffer, including, for example M-x. Maybe some commands should be skipped.

Not sure about that, the purpose for me would be to automatically remember the last thing I did and go back to it.

I just meant that if you do M-x some-command and some-command does not use the minibuffer, then the command that used the minibuffer is execute-extended-command, while you might want that one to "not count".

@kljohann I noticed the issue with pcase-let, it's annoying. I like your error message version.

@clemera
Copy link
Contributor Author

clemera commented Jan 6, 2021

@minad Looks like a good path to take, just saving the input in post-command-hook is a great idea, and this works for selectrum, too :) I'm also for removing selectrum-repeat when the alternative is available.

@clemera
Copy link
Contributor Author

clemera commented Jan 6, 2021

@oantolin

I just meant that if you do M-x some-command and some-command does not use the minibuffer, then the command that used the minibuffer is execute-extended-command, while you might want that one to "not count".

I see, could be nice to have it as an configureable option

@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

Yes, maybe reconsider the idea of making it another package. The overlap that I am seeing is the following: If we have a proper input history, all Embark needs to do is take the last element from the input history and inject it as input without a RET to the last recorded command.

Yes, a separate package sounds better. (And I like the name chronicle you suggest later!) This package would in charge of defining what "last input" even means! I thought it meant the minibuffer contents as you are exiting, but that's not it, since, depending on what completion UI your are using and what you have RET bound to, often the selected candidate is the last minibuffer contents contents. Figuring out what we mean by last input sounds like the job of a separate package, albeit a small one.

Another thing is that @oantolin is proposing another ring of commands+last input, sounds also like a history to me, but a very special one. Maybe I am overthinking this and maybe I am messing up orthogonal things here.

Yes, I'd say it is yet another type of history.

All I want is that we do not rush to half baked solutions, putting history functionality overly quickly into Embark, Consult or Selectrum. Note that the input history has been proposed originally for Selectrum, then we discussed about moving it to Consult. Then consult-repeat has been proposed for Consult and we thought that it fits better in Embark etc. It seems to be not completely trivial to find a good solution here.

Agreed! I will remove embark-repeat in favor of discussing this more and reaching a fully fledged design.

Yes sure this problem exists too. But I am more in favor of in-band information these days [...] So maybe the idea of additional hidden state should be given up and instead we should focus only on good support for the in-band info. Note that the history also does not store the hidden state [...]. I think making this hidden state thing work is not a good idea, it works against how Emacs histories work.

Agreed!

@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

The missing piece is the input injection and this is provided by Embark. But it is not much code so we could also do that without Embark. But maybe this should just be extra package. It has a narrow scope, but it is different from how both Consult and Embark operate as of now.

Agreed. The injection is really is not much code at all, see embark-repeat. Dealing with injection is not enough reason to think this belongs in Embark. I don't oppose adding an appropriate definition of "last input" to Embark, and doing this there, but I'm starting to favor the separate package @minad suggested.

@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

I have not tried running the chronicle prototype, but having read it, does it really record the last input? You save the minibuffer contents in a post command hook, doesn't that hook also run after minibuffer-force-complete-and-exit, before the exit hook? I would think so, so that you get the candidate again and not the "input".

@minad
Copy link
Contributor

minad commented Jan 6, 2021

I have not tried running the chronicle prototype, but having read it, does it really record the last input? You save the minibuffer contents in a post command hook, doesn't that hook also run after minibuffer-force-complete-and-exit, before the exit hook? I would think so, so that you get the candidate again and not the "input".

This is what I use in consult and it seems to work with icomplete and selectrum. I trust it based on that, but I am not 100% sure if there are not some edge cases.

@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

This is what I use in consult and it seems to work with icomplete and selectrum. I trust it based on that, but I am not 100% sure.

But it's tricky then, it depends on the fact that when you press RET, the exit hook runs before the post command hook? Is that what happens? Because that sounds weird.

@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

Well, no, maybe that is what happens, and it doesn't so weird...

@minad
Copy link
Contributor

minad commented Jan 6, 2021

But it's tricky then, it depends on the fact that when you press RET, the exit hook runs before the post command hook? Is that what happens? Because that sounds weird.

No idea tbh, I also find it weird. I found this by experimentation. I cannot give you a sound answer, sorry! Maybe it really is broken and should be replaced by something better in consult too!

@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

Also, I didn't know consult had an input history mechanism. 😆

@minad
Copy link
Contributor

minad commented Jan 6, 2021

@clemera @oantolin Regarding the history package, we seem to agree? So the plan is the following:

  • We create some separate package handling input histories, named for example chronicle or another name, maybe uchronia if we like weird names
  • @oantolin removes embark-repeat, since it is new and not yet used, probably just revert the relevant commit
  • @clemera removes selectrum-repeat as soon as some kind of chronicle becomes available
  • I will kick out the consult input history mechanism as soon as chronicle becomes available

@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

No idea tbh, I also find it weird. I found this by experimentation. I cannot give you a sound answer, sorry! Maybe it really is broken and should be replaced by something better in consult too!

No, I just ran some experiments and I think it's a good solution. What actually seems to happen is that the post command hook does not run at all for the command that exits the minibuffer (the one bound to RET)! Probably because exiting the minibuffer involves throw.

@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

@clemera @oantolin Regarding the history package, we seem to agree? So the plan is the following:

  • We create some separate package handling input histories, named for example chronicle or another name, maybe uchronia if we like weird names
  • @oantolin removes embark-repeat, since it is new and not yet used, probably just revert the relevant commit
  • @clemera removes selectrum-repeat as soon as some kind of chronicle becomes available
  • I will kick out the consult input history mechanism as soon as chronicle becomes available

I agree with all steps in this plan. And I do like the name chronicle, and with @minad's implementation strategy now that I know the post command hook does not run for RET.

@minad
Copy link
Contributor

minad commented Jan 6, 2021

I like uchronia now since it is less generic than chronicle and we are basically moving back in the history and rewrite it 🤣 But besides that it looks like we have a plan.

oantolin added a commit that referenced this issue Jan 6, 2021
This reverts commit 622f6ea.

The embark-repeat command was an experiment that failed because
"minibuffer contents during the minibuffer exit hook" is not a good
definition of "last input", since pressing RET can modify the
minibuffer contents to match the selected candidate.

As discussed in #85, we have decided to remove this experiment in
favor of a better behaved and more featureful solution that will be
made available as a separate package.
@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

embark-repeat has been reverted. uchronia is ok too, but it means a fictional time, no? Maybe there's a name that sounds more like going backwards in time.

@minad
Copy link
Contributor

minad commented Jan 6, 2021

Uchronia is a neologism, but it sounds like a proper word. https://en.wikipedia.org/wiki/Alternate_history
Uchronie apparently is a german word, I certainly didn't know that 🤣 And ucronía is a proper spanish word?

We could also go with ucronia but I would somehow miss the h then, since words like chronik, chronos etc are more familiar to me.

Wikipedia:

In Spanish, French, German, Portuguese, Italian, Catalan and Galician, the genre of alternate history is called uchronie / ucronia / ucronía / Uchronie, which has given rise to the term Uchronia in English.

@clemera
Copy link
Contributor Author

clemera commented Jan 6, 2021

I like the suggested names another option that came to mind: DeLorean

@oantolin
Copy link
Owner

oantolin commented Jan 6, 2021

I did not know ucronía was a Spanish word! It means "a reconstruction of history based on hypothetical data".

You know, what? I like uchronia now. You can argue that rerunning an old command and its input starts an alternate timeline.

Plus, as you say, it's less generic than chronicle. And chronicle sounds like it should be some journalling package or something.

@minad
Copy link
Contributor

minad commented Jan 6, 2021

I think DeLorean should be reserved for something more sophisticated 🤣

@minad
Copy link
Contributor

minad commented Jan 6, 2021

You can argue that rerunning an old command and its input starts an alternate timeline.

This is exactly what I thought!

@minad
Copy link
Contributor

minad commented Jan 6, 2021

I've made a small uchronia mode here: https://github.com/minad/uchronia/blob/main/uchronia.el. It works well, but I am still not 100% convinced that this is the right thing since it is very small, because of the split etc. Sorry for being so undecided...

Now there is an alternative I would like to consider:

  • Keep the input history special handling in consult, every command can decide by itself if it wants to use input or not by using consult--read or the technique from there!
  • Add consult-repeat, consult-repeat-select and consult-repeat-history which is basically uchronia-repeat, uchronia-select and uchronia-command-history, this will just enhance all existing commands and allow repeating sessions. We could as well put this into Embark if we prefer that!
  • Do not provide the functionality of the history toggler. This is some functionality I don't like. I don't think it is useful and I would neither like to see it in selectrum/embark/consult. But since this has been discussed in Add ability to use input history radian-software/selectrum#185, I would like to hear why this has been considered? Maybe @clemera could enlighten me.
  • Unfortunately at this point the possibility to use the input history for existing commands has been lost. But this could be restored by replacing history elements with input, similar to how consult--read does it.

Basically I just propose a downstripped uchronia without the possibility to toggle between input/normal histories. And I think at this point it is just so small that I would rather put it into consult/embark itself!

EDIT:

@clemera
Copy link
Contributor Author

clemera commented Jan 6, 2021

Do not provide the functionality of the history toggler. This is some functionality I don't like. I don't think it is useful and I would neither like to see it in selectrum/embark/consult. But since this has been discussed in radian-software/selectrum#185, I would like to hear why this has been considered? Maybe @clemera could enlighten me.

I don't remember exactly, I think it was considered because we thought both can be useful and it is convenient to toggle to the behaviour you are interested in. The candidate history is useful if you are looking for a candidate, the input history if you are looking for search results you previously had or want to do the same search (the candidate set might have changed).

@minad
Copy link
Contributor

minad commented Jan 6, 2021

@clemera Yes, I agree that it is not totally useless. But still - I don't think the tradeoffs are really worth it. By Emacs standards, there is one history per command and my proposal is to just allow to define it as either input or candidate. I don't like the text property solution, I rather introduced separate variables now in (https://github.com/minad/uchronia/blob/4cf9d72cf673352080fe835324d8f35e2b9ee43f/uchronia.el). I am worried that the text property solution could potentially break other things. But maybe if I would embrace the text property solution, I would feel more convinced than I am now, since then we would at least not get the history variable duplication. But I argue that having only one history type covers like 99% of the use cases. You can always obtain the selected candidate again, by repeating with the input history and then making the selection. And then having a changed set of candidates - I guess this is such a rare use case. In Consult I also only have one history type per command and so do all Emacs commands. Do other packages also provide the functionality to toggle between the history types? All in all I am thinking if I would like to use such a toggler, and from my current feeling, I don't think I would. So it wouldn't pass this kind of check and I don't want to implement functionality of which I am not somehow convinced. Have you been in a situation where you would have wanted that functionality? I have not, but this is always easy to say since with the given restrictions it may also not have occurred to me that I could want something like this.

@clemera
Copy link
Contributor Author

clemera commented Jan 6, 2021

The history commands don't care about the text properties but I agree that you never know in Emacs land, maybe someone relies on the fact the history doesn't has text properties by default but if this becomes relevant you could also remove it then. If you only allow input history for a command you are also dependent on the filtering method used, when you switch the framework or default completion you might still want to be able to access previous candidates.

@minad
Copy link
Contributor

minad commented Jan 6, 2021

@clemera I will try the text property method, maybe I am more satisfied then since it may feel more "low cost", not introducing these auxiliary uchronia-input-history/... histories. We will see. At least it is good to do this kind of experimentation outside of Embark and Consult :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants