Handling of `g` binding in `embark-export`

purcell commented 3 years ago

g usually connotes refresh, and indeed I used to use this in ivy-occur to refresh the ripgrep results in an occur buffer after making changes. In embark-export buffers, which are in grep-mode, g invokes "recompile", which will generally run a completely different command. I wonder if there's a way to handle this and make refresh possible in embark...

oantolin commented 3 years ago

I'd love to have this feature too. Also for the occur export for consult-line. Currently the necessary information to regenerate the list is not stored, but it would be fairly easy to do so. I'll work on it when I get the chance.

Up to now this has been one of the main distinctions between embark collect and export: collect buffers have a working revert. But even for collect buffers, revert only works while the buffer you called embark-collect-{snapshot,live} from is still live. So in the case of calling embark-collect-snapshot from the minibuffer (the most common case to call it from!) doesn't actually have a working revert either.

manuel-uberti commented 3 years ago

Oh, I was about to open an issue for this. Thank you @purcell for doing it first. :)

DamienCassou commented 3 years ago

I would also love that and made some research:

grep-mode inherits from compilation-mode.
It seems like setting the compilation-arguments variable in the grep buffer would be enough to make g run grep with the same arguments again.
embark-consult-export-grep just take candidates as input: the grep command and arguments got lost midway. I don't know how to propagate this information to the embark framework.
ivy.el doesn't use the compilation-arguments but instead binds g to ivy-occur-revert-buffer and the initial buffer is created with ivy-occur.

dschrempf commented 2 years ago

Has there been any progress on this? I keep getting compilation errors because I press g r in Embark Export Grep buffers :-).

dschrempf commented 2 years ago

Not sure if this fits here, but q should probably also close the buffer and not define a key macro.

minad commented 2 years ago

@dschrempf I think there has been progress recently. @oantolin simplified the Embark collect/export infrastructure greatly, which should help in picking up this issue again (and also other collect/export-related issues). However I am not aware of anyone working on this particular issue currently. I would also like to have this feature. It would be nice to have the ability to refresh the buffer after making edits. This affects in particular consult-line and consult-grep buffers. I guess @oantolin would also appreciate contributions, if you are interested in digging into the Embark code base. :)

oantolin commented 2 years ago

I was going to say what @minad said, about the recent simplification probably helping. I do want to work on this but haven't found the time. One would need to store a revert-buffer-function that does the right thing: for export from a minibuffer, it should rerun the command that opened the minibuffer, retype the minibuffer contents, wait a configurable delay (for async searches like consult-grep), then gather the new candidates; for export from a non-mini buffer, it can probably just rerun the candidate collector.

minad commented 2 years ago

Oh indeed, asynchronous sources pose a big complication here. I didn't think of that. Maybe one could check if the asynchronous background process has finished via polling. Currently there doesn't exist a reliable protocol which can be used by Embark to determine if the asynchronous minibuffer process has terminated. The only indication is that the minibuffer prompt status overlay changes when the asynchronous process finished.

oantolin commented 2 years ago

I pushed an initial implementation that doesn't work in all cases, but you can try it out with dired export (rename a file, and revert to see the effect), or with consult-line. This initial implementation does not cover the case of exporting from consult-grep (or any async consult search command). See the commit message for more details.

oantolin commented 2 years ago

I've fixed the jumping windows problem (05cf792f8fdc061cfacf8686e1bb39d9c6e092ac) and now reverting an export buffer reuses whatever windows currently display it. Now the main thing remaining is making this work with consult-grep and other async search commands.

oantolin commented 2 years ago

OK, a working prototype is available! b57c947f20dc36faff7a43076b9a3b0358f87921 To handle consult async searches, I just wait for the little star to turn into a colon. 🤣 Is there anything better I could do, @minad?

oantolin commented 2 years ago

Quick updates:

I added revert support for dired buffers that came from consult-find (thanks @minad for reminding me I had forgotten to do that).
g is now bound to revert-buffer in exported grep buffers (revert-buffer already worked there, but g was bound to recompile as @purcell pointed out).

minad commented 2 years ago

embark-collect-mode buffers also need a proper revert function. In a consult-grep snapshot buffer revert does not seem to work.

oantolin commented 2 years ago

Yes, I can confirm reverting an embark collect buffer is not supposed to work yet: these changes only apply to embark export buffers so far. I need to think a bit about the collect case, since embark-live already uses the revert machinery and expects it to behave a certain way.

Also, the collect revert function can be much less hacky since we control buffer creation in that case. Most of the exporters use some Emacs function that creates and display a buffer for you whether you want it or not, you can't say: "please just fill in this pre-existing buffer". But we can do that with collect.

On the other hand maybe the best thing would be to make collect and export even more similar. In fact, I could imagine collect becoming just a default exporter...

dschrempf commented 2 years ago

To be honest, I don't really understing the difference between collect and export (there are different keybindings and such, but I don't get the purpose).

oantolin commented 2 years ago

@dschrempf Well, export uses a major mode appropriate for the type of candidates: dired for files, ibuffer for buffers, occur for locations in buffers, grep for locations in files. Now, not every type of candidate has an appropriate major mode for it, so collect is a generic fallback that can store any type of candidate. Also, collect can store multiple types of candidates in a single buffer and still offer you type specific actions.

For example, if you try exporting from consult-buffer several things can happen:

If all current candidates are actual buffers, you get ibuffer.
If all current candidates are recent files, you get dired.
If you have a mix of candidates, export "gives up" and you get a collect buffer with all of them, where you still get offered buffer actions for the buffers and file actions for the files.

Now, if a type of candidate does have an appropriate major mode, are there some circumstances in which you'd still want collect or is export always better? I think it's fine to always prefer export, but I like having the option of using collect even when export would do something different, because collect buffers all behave the same way and that predictability might be nice sometimes.

dschrempf commented 2 years ago

I see, thank you for the explanation. I usually use export, but I understand now why you may want to keep both.

minad commented 2 years ago

Copying my comment from https://github.com/minad/marginalia/issues/136#issuecomment-1095343734

What do you think about solving the revert problem in a simpler way - what if we don't support reverting for Consult Async commands, but instead do the following when reverting?

Close the grep/snapshot buffer
Restart the command with the input
Let the user do the rest

This special behavior could be configurable per command or completion category via an exclusion configuration variable. For normal synchronous completion commands revert of snapshot/export should simply work (there shouldn't be an issue?). The advantage of this is also that one could reedit the input. I think it makes sense to provide two commands anyway

revert-buffer
embark-collect-edit/embark-export-edit

and for async commands revert-buffer would just be the same as embark-collect-edit/embark-export-edit.

oantolin commented 2 years ago

That sounds pretty reasonable to me, and it's certainly easy to implement. My only worry is that it might be a case of shifting work onto the users to make the implementation easier. If the vast majority of times, after the command is rerun and the input reentered, people are just going to wait for the search to finish and then export again, it would be nice to automate that.

In another matter, if we do go this route there is also a configurationless option: to treat sync and async the same, in all cases just rerun the command and reenter the input, then less users do the rest. Would that be terrible?

minad commented 2 years ago

My only worry is that it might be a case of shifting work onto the users to make the implementation easier. If the vast majority of times, after the command is rerun and the input reentered, people are just going to wait for the search to finish and then export again, it would be nice to automate that.

Yes, however in the case of async commands there are simply too many downsides and it seems to me that there are quite a few arguments against it.

We need special coordination between Consult and Embark. Currently we don't have any such special coordination in embark-consult, we rely only on completion categories and the data attached to the candidates, which is a protocol which is easy to understand.
Reverting buffers for async commands is not well defined since commands don't have to stop producing candidates. Commands could continuously produce candidates forever, the Consult API does not prevent that.
The minibuffer popping up is annoying and looks like a glitch. This only matters for Vertico or Mct and it wouldn't affect the manual default UI. We could try to enforce that the default UI is used during revert. But then we would need an additional progress report, which is adhoc complexity.
In the case of Vertico, there is vertico-repeat. Of course this is not an exact substitute but still helps.
This is only a personal preference but I am willing to sacrifice minor conveniences for more conceptual clarity. Restarting a command and allowing the user to do the rest is simple and works in all scenarios. On the other hand, revert for async commands requires a rather ad-hoc implementation.
You can define arbitrary candidate collectors. How to restart, revert those? It seems to me that the revert feature just doesn't fit well conceptually into the Embark/Consult framework. Embark is very general, the particular revert we have in mind is special and does not cover all scenarios. Of course the reedit command is also adhoc, so this is probably not an argument :-P

In another matter, if we do go this route there is also a configurationless option: to treat sync and async the same, in all cases just rerun the command and reenter the input, then less users do the rest. Would that be terrible?

No, for me it would be fine. I mean we have to compare with the status quo (or the status before the current incomplete revert implementation) and we should keep adhoc complexity in mind.

oantolin commented 2 years ago

The minibuffer popping up is annoying and looks like a glitch.

No, no, it's a very tall progress bar! 😛

But seriously, I don't like it either, and certainly if we redefine "revert" to just mean "rerun the command and reenter the input", then the minbuffer popping up would be completely reasonable.

minad commented 2 years ago

But seriously, I don't like it either, and certainly if we redefine "revert" to just mean "rerun the command and reenter the input", then the minbuffer popping up would be completely reasonable.

Exactly. We just redefine revert to mean something else which makes sense for Embark/Consult :)

oantolin commented 2 years ago

I like this proposal, and think I'd start without any configuration options, those can always be added later.

oantolin commented 2 years ago

Ok, I think this should be good to go. I implemented @minad's suggestion for simpler revert semantics, for both embark-export and embark-collect:

If you ran embark-export/collect from the minibuffer, running revert-buffer in the export/collect buffer kills the buffer, reruns the command that had opened the minibuffer and re-enters the input that was in the minibuffer at the time you exported/collected.
If you ran embark-export/collect from a non-mini buffer, rever-buffer just runs it again.

I like the functionality, it feels straightforward and uniform. And I like being able to tweak the "search terms" (minibuffer contents) before re-exporting.

Please test and report any problems.

minad commented 2 years ago

Great, thanks! I will try it soon.

oantolin commented 2 years ago

You'll also like, @minad, that I removed the embark-collect--candidates variable. 😛

minad commented 2 years ago

Sure all simplifications are welcome! 😄

minad commented 2 years ago

It seems as if you got rid of a lot of global state. Very nice :)

minad commented 2 years ago

I tested this and it works great. I like this behavior, it is simple and coherent. I like that there is a way back from the collect/export buffer to the completion command. Exporting again from there is not a big deal. However I wonder if we should print a message?

For revert of normal buffer exports: "Recollecting buffer..."
For going back to minibuffer: "Restarting completion command..."

minad commented 2 years ago

It should also be possible to implement a poor mans revert function based on this base functionality? Not as part of Embark but we could put it into the wiki.

Open the minibuffer
Wait for a second (or even use polling of the consult-grep prompt)
Run export/collect

oantolin commented 2 years ago

Good idea, specially for the normal buffer case, where it's not clear something happened otherwise.

oantolin commented 2 years ago

Although, the usual Emacs behavior seems to be to revert without any message at all (except if you are prompted for confirmation, of course). I just tested a normal file, a dired buffer and a help buffer.

oantolin commented 2 years ago

Weird: revert-buffer doesn't print a message (if you are in a no-confirmation case) but revert-buffer-quick does.

oantolin commented 2 years ago

Oh, I missed your other comment, about the poor man's revert function, @minad. That could definitely be done, but I'd wait to see if there's demand. I wasn't sure I'd like this behavior but now I'm sold, maybe others will also find they don't need a full revert.

minad commented 2 years ago

Weird: revert-buffer doesn't print a message (if you are in a no-confirmation case) but revert-buffer-quick does.

Oh okay, maybe I am already accustomed to C-x x g :)

Oh, I missed your other comment, about the poor man's revert function, @minad. That could definitely be done, but I'd wait to see if there's demand. I wasn't sure I'd like this behavior but now I'm sold, maybe others will also find they don't need a full revert.

Sure. I just meant that with the current facilities we could still easily implement this on top.

Good idea, specially for the normal buffer case, where it's not clear something happened otherwise.

I thought that it could also be useful in the minibuffer case just to explain what is going on, since it deviates from usual buffer revert. But it is probably sufficient to document this well in the docstrings or the readme. Furthermore a message will get annoying anyway. Generally Emacs is way to noisy for me and I suppress a lot of messages. So maybe I shouldn't propose adding more. But I think a ding here and there would certainly be appreciated by most of the Embark user base. :-P

oantolin commented 2 years ago

Thanks for mentioning documentation, I had forgotten! ab778510d5db9935a2d436b13d98c47b824172ee

jaor commented 2 years ago

On Wed, Apr 13 2022, Omar Antolín Camarena wrote:

Oh, I missed your other comment, about the poor man's revert function, @minad. That could definitely be done, but I'd wait to see if there's demand.

fwiw, the first thing i'll do is implementing a revert command like that (recomputing the collect buffer in one keystroke): that's what i want in most of the time. and a C-u prefix will give me the variant reopening the minibuffer for the remaining cases (i'd provide that behaviour directly in embark if it were my package, perhaps via C-u, but this is emacs, so we can both be opinionated in different directions at the same time! :))

oantolin commented 2 years ago

Mmh. Now that I removed the one-shot revert from embark-consult, I am reluctant to reintroduce it. It was hacky and probably a little unreliable, basically I don't want to give it any support. How about I reintroduce the hook it required and put the necessary configuration on the wiki, @jaor?

minad commented 2 years ago

@oantolin Is it even necessary to introduce a hook? Is it not sufficient to write something like this?

(defun embark-reexport ()
  (interactive)
  (run-at-time 0 nil #'message "Reverting...")
  (run-at-time
   1 nil ;; Instead of a fixed time one could use polling here
   (lambda ()
     (when-let (win (active-minibuffer-window))
       (with-selected-window win
         (embark-export))))) ;; embark-export or embark-collect, determine based on buffer name?
  (revert-buffer))

oantolin commented 2 years ago

Oh, right! Very nice @minad!

We can refine that function a bit and toss it on the wiki: add the polling for consult async search commands and use embark-collect for embark-collect-mode buffers and embark-export for all others.

minad commented 2 years ago

We can refine that function a bit and toss it on the wiki: add the polling for consult async search commands and use embark-collect for embark-collect-mode buffers and embark-export for all others.

Yes, this is what I meant by "poor man's implementation" based on the more basic functionality we have now in Embark. It is a nicely stacked design :)

oantolin commented 2 years ago

@dschrempf I forgot one case in which you might prefer embark collect to embark export even if there is a special major mode that export would use! You may want to mark some candidates and use embark-act-all on them. Now, you wouldn't do this with files or buffers, because then export produces dired or ibuffer buffers, both of which let you mark items and run commands on them. But many export buffers do not have candidate-marking functionality: occur, grep, apropos and customize buffers don't for example.

A case that just came up for me was that I wanted to insert a bunch of face names in an email message composition buffer, split into two different locations in the message. So I ran describe-face, typed a pattern that matched all the faces I needed and ran embark-collect. In the collect buffer I marked all the faces I wanted to insert at the first spot in the email and ran embark-act-all, embark-insert. Then I moved in the email buffer to the second spot where I wanted the other faces, returned to the embark collect buffer, toggled the marks and again used embark-act-all, embark-insert. It was much faster to do than to describe here!

In Embark's default configuration faces export to a customize buffer, which is great for tweaking the faces (o even just reading their docstrings!), but in this case I needed the ability to mark a subset of them and so went with embark-collect.

dschrempf commented 2 years ago

Thank you, that makes sense. Collect seems especially useful when the major mode used by embark does not allow marking.

oantolin commented 2 years ago

After using this a couple of days, I'm really enjoying this "revert means rerun" functionality. It feels like a good UI for two operations: reverting and rerunning with slightly modified input. There are, of course, alternatives such as (1) offer two separate commands, (2) offer a command that does one thing or the other depending on the prefix argument. But what I like about the current choice is that it has late binding: you first press g and later decide whether to just reexport or to tweak the input first. You get to see the updated results before deciding whether you want to change the input, which I really like.

minad commented 2 years ago

I observed one problem in exported grep buffers. After switching to wgrep and back, the keybinding of g is reset to recompile. Did you observe this behavior too?

oantolin commented 2 years ago

Good catch, @minad! Luckily, it was an easy fix.

oantolin commented 2 years ago

I'm closing this issue because I feel satisfied with the current functionality, but please feel free to continue the discussion or to reopen.

jaor commented 1 year ago

How about I reintroduce the hook it required and put the necessary configuration on the wiki, @jaor?

works for me, thanks!

mpereira commented 1 year ago

Was there a regression on this fix? Or maybe I'm doing something incorrectly.

Run M-x consult-grep or M-x consult-ripgrep
Run embark-act on the candidates buffer
Press E for embark-export, get "Grep mode" buffer
g r is mapped to recompile which tries to run make -k instead of recomputing and re-rendering grep or ripgrep results
M-x revert-buffer says revert-buffer--default: Buffer does not seem to be associated with any file

M-x embark-rerun-collect-or-export correctly re-runs the grep or ripgrep search and recreates the candidates buffer, but I'm looking for a way to recreate the "Grep mode" buffer (similar to what ivy-occur-revert-buffer does).

Software	Version
macOS	13.2
Emacs	29.0.60
Embark	0.21.1
Consult	0.33

dschrempf commented 1 year ago

I think this is not a regression. It is expected behavior. I quote from above:

After using this a couple of days, I'm really enjoying this "revert means rerun" functionality. It feels like a good UI for two operations: reverting and rerunning with slightly modified input. There are, of course, alternatives such as (1) offer two separate commands, (2) offer a command that does one thing or the other depending on the prefix argument. But what I like about the current choice is that it has late binding: you first press g and later decide whether to just reexport or to tweak the input first. You get to see the updated results before deciding whether you want to change the input, which I really like.

oantolin / embark

Handling of `g` binding in `embark-export` #171