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

How can I edit in the collection buffer? #98

Closed
wd opened this issue Jan 9, 2021 · 72 comments
Closed

How can I edit in the collection buffer? #98

wd opened this issue Jan 9, 2021 · 72 comments

Comments

@wd
Copy link

wd commented Jan 9, 2021

I previously use swiper to search for something, and then I can use C-c C-o to export the candidates to an occur buffer, so I can edit them at that buffer.

How can I do this in the collection buffer?

@minad
Copy link
Contributor

minad commented Jan 9, 2021

@wd Are you using consult-line? If yes, this is unfortunately not yet possible and I am not sure if there is an easy way to make it possible. The only possibility I could imagine for now is the following: We could add a transformation to embark which converts the 'line category to a grep buffer if embark-export is used, such that the wgrep facilities become available.

As an alternative you can use consult-grep for now, export to a grep buffer via embark-export. As usual, in the grep buffer you can use wgrep to do the editing.

@oantolin What do you think? (One question for me to understand - as soon as you did the embark-export, you are not "inside embark" anymore but just in the normal mode of the exported buffer, right? Actions don't work anymore, in particular not the default action. I thought/proposed to use the export as the default instead of embark-collect-*, but this clearly does not make much sense.)

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Let me make sure I know what you mean @wd. Since you mention swiper, like @minad said, I think you are talking about using consult-line to search for something, then using embark-collect-snapshot to capture the search results in a buffer. And you would like to edit the results and have the changes propagate back to the original buffer? Is that right, do you mean when using consult-line?

Assuming that's what you mean, I don't think I will make the Embark collect buffer editable in place with the changes propagating back to the original buffer. The reason is that the Embark collect is generic, used for many types of targets, not just results of searching some buffer.

Instead, what I want to do is to provide an exporter for the line category that produces an occur-mode buffer, like what you get from the built-in M-x occur command. (This is already on the wishlist, see #95.) Once that exporter is in place you could do the following: use consult-line to search for something, call embark-export and then in the resulting occur-mode buffer use e to put it into occur-edit-mode. Then you can edit as you wish and press C-c C-c to have the changes propagate back to the original buffer. I prefer to do this with an exporter to reuse the great built-in occur-edit-mode rather an reimplementing a (probably inferior) version myself.

As @minad said, you can do something similar with consult-grep right now: embark-export gets you a grep-mode buffer and you can use the popular wgrep package to edit the results in place. (Of course, you can also use the built-in occur command, for now.)

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

@minad said:

We could add a transformation to embark which converts the 'line category to a grep buffer if embark-export is used, such that the wgrep facilities become available. What do you think?

For the line category I think export to occur-mode makes more sense than to grep-mode, and I already have that on the wishlist. But an exporter to grep-mode would also work, I guess.

(One question for me to understand - as soon as you did the embark-export, you are not "inside embark" anymore but just in the normal mode of the exported buffer, right? Actions don't work anymore, in particular not the default action. I thought/proposed to use the export as the default instead of embark-collect-*, but this clearly does not make much sense.)

Yeah, they really are different, and yes, you do lose the default action. But note that in the exporters we have there is almost always a way to do what the default action does anyway. Lots of commands prompt you for files, and if you export to a dired buffer, RET is often not going to do what the command did, but also there is usually a way to whatever the command does from dired. In the worst case, you can still use embark-act in the dired buffer and use the original command as an action! As another example, for commands that produce xref-location, we already fudge the default action so that it does exactly what RET in the exported grep buffer does.

I do think collect and export really are different and should stay separate, but I also think that it is perfectly reasonable for a user to decide to always prefer export.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

I just checked to see exactly what kind of buffer you get if you press C-c C-o while using swiper. It's in ivy-occur-grep-mode, contains results formatted like grep results, and for editing it delegates to the wgrep package.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

I do think collect and export really are different and should stay separate, but I also think that it is perfectly reasonable for a user to decide to always prefer export.

I agree with this 100%, they should stay separate! It was just my incomplete understanding before which lead me to make the suggestion.

For the line category I think export to occur-mode makes more sense than to grep-mode, and I already have that on the wishlist. But an exporter to grep-mode would also work, I guess.

Agree!

I just checked to see exactly what kind of buffer you get if you press C-c C-o while using swiper. It's in ivy-occur-grep-mode, contains results formatted like grep results, and for editing it delegates to the wgrep package.

I still think the occur buffer is probably better. But we should experiment in order to check which is better in the end.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

It seems that replace.el doesn't really give an easy way to reuse it's occur functionality to make your own occur buffers (why am I not surprised? :D), but the "protocol" is not too bad, you just need to put some text properties on the lines in the buffer, chiefly an occur-target property whose value is a marker. If you have the right text properties on the text, you can put the buffer in occur-mode and it just works.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

Does not look too bad too me! I suspect this will work well and without too much boilerplate code. I have to recheck what I encode in these unicode prefix characters (either line number or position). If I always encode the position there you can easily convert back. Alternatively - what about adding these occur-target markers directly into the 'line results offered by consult? Would that make sense? This way we don't introduce our own hacky unicode prefix intermediate protocol and consult just follows the occur protocol by itself.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

You encode the line number in the prefix. I know, because I added an action to Embark to save the line number to the kill ring. 😄

Adding the occur-target markers in consult would be great for me, but more work for you. I guess it might be easier to do it in Consult, since I believe you already compute the markers: you make an alist of strings and markers, right?

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Another argument for adding the markers in Consult: I can do it in Embark by assuming the lines refer to the embark target buffer, and this would work for consult-line and consult-mark, but I don't see any way of handling the consult-global-mark case in Embark.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

You encode the line number in the prefix.

Yes, but not always. consult-mark encodes the position since there could be multiple marks on a single line. Maybe I have to unify this such that there is no consistency problem.

Adding the occur-target markers in consult would be great for me, but more work for you. I guess it might be easier to do it there, since I believe you already compute the markers: you make an alist of strings and markers, right?

Yes, it will be easier and we avoid the intermediate decoding step. I am just not sure if these marker properties on candidates could have bad side effects? What happens if you serialize a string with a marker inside? This is going to create problems. Maybe an integer position would work too, but using a marker is better if lines are moving around. So I would prefer the marker, but I am just worried about the serialization issue.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

Another argument for adding the markers in Consult: I can do it in Embark by assuming the lines refer to the embark target buffer, and this would work for consult-line and consult-mark, but I don't see any way of handling the consult-global-mark case in Embark.

consult-global-mark already uses the grep location formatting. But I am using the 'line category. Why is that? There seems to be something wrong here?

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Yes, but not always. consult-mark encodes the position since there could be multiple marks on a single line. Maybe I have to unify this such that there is no consistency problem.

Ha! My embark-save-line-number action is broken for consult-mark and I never noticed.

consult-global-mark already uses the grep location formatting. But I am using the 'line category. Why is that? There seems to be something wrong here?

Sound like consult-global-mark should report the grep category instead, then. I confess I haven't used that function.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

Ha! My embark-save-line-number action is broken for consult-mark and never noticed.

Yes, I think I had it in mind when I saw that you added this action but forgot about it. Is this really a useful action btw? I would maybe remove it.

Sound like consult-global-mark should report the grep category instead, then. I confess I haven't used that function.

Yes, xref-location. I have to check if there was some reason why I didn't do that yet.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

consult-global-mark: Ouch, there are conflicting issues here - see minad/consult#107. I originally had 'line, then when we introduced 'xref-location, I also changed it there and then changed it back to 'line in minad/consult#107.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Is this really a useful action btw? I would maybe remove it.

Probably not. I have only ever used it for testing. I think ti just seemed like i should do something with the prefix besides strip it! 😆

@minad
Copy link
Contributor

minad commented Jan 9, 2021

I think ti just seemed like i should do something with the prefix besides strip it! laughing

Yes, I knew that you wouldn't like to throw away this precious information 🙈

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

consult-global-mark: Ouch, there are conflicting issues here - see minad/consult#107. I originally had 'line, then when we introduced 'xref-location, I also changed it there and then changed it back to 'line in minad/consult#107.

Oh. I see. Mmh. We'll have to think a bit about this.

What does consult-global-mark use for marks in non-file buffers? Does it use the buffer name?

@minad
Copy link
Contributor

minad commented Jan 9, 2021

I think my fix to #107 was wrong. I think in that case @hmelman should just use embark-collect instead of embark-export! Then the problem should not be there, right? It is this export/collect distinction again. The export is just not what you want in that case, I guess! This means if I change back to xref-location we are good!

What does consult-global-mark use for marks in non-file buffers? Does it use the buffer name?

Yes, it is always only the buffer name.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Mmh. But then it would mean the xref-location exporter is wrong: it doesn't always produce a valid grep buffer.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

Hmm, but I could change it such that it uses the file name relative to the current directory as an alternative if the buffer has a file backing? And then still use the xref-lcoation.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Ah, maybe that works, at least for line from buffers visiting files. I think probably lines from non-file buffer don't have much chance of working in a grep mode buffer.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

My decision to use the xref-location is due to the fact that the format is like the grep buffer, but maybe it is not sufficiently so. We can also just keep the line category. But this will break the embark special handling of 'line, right?

@minad
Copy link
Contributor

minad commented Jan 9, 2021

I have one more question regarding the default-directory - is this somehow inherited by the Embark buffers or the grep buffer via embark-export, such that this works? I am not eager to use absolute paths or abbreviated paths. But with default-directory relative paths it will still look very ugly :(

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

My decision to use the xref-location is due to the fact that the format is like the grep buffer, but maybe it is not sufficiently so. We can also just keep the line category. But this will break the embark special handling of 'line, right?

Mmh. Well, embark does need to do much with lines, it keeps them intact in the collect buffer so the default action works, and it strips the unique prefix for the insert and save actions. Using line for consult-global-mark would only have the side-effect that the line inserted or saved in the kill ring would still have the grep format. That doesn't seem too bad.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

I have one more question regarding the default-directory - is this somehow inherited by the Embark buffers or the grep buffer via embark-export, such that this works? I am not eager to use absolute paths or abbreviated paths. But with default-directory relative paths it will still look very ugly :(

Embark collect buffers have the default-directory of the target buffer, the one current prior to opening the minibuffer. I'm not sure what happens for the exported grep-mode buffers, I'd have to check.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

Mmh. Well, embark does need to do much with lines, it keeps them intact in the collect buffer so the default action works, and it strips the unique prefix for the insert and save actions. Using line for consult-global-mark would only have the side-effect that the line inserted or saved in the kill ring would still have the grep format. That doesn't seem too bad.

Okay then we can keep it as is. I will add a comment to consult-global-mark though to explain this. Otherwise I am sure I will forget it again.

Now I have to check where this occur-target property should be put, maybe we can put it such that it also works with consult-global-mark? I assume it is applied to the string of the line without the prefixes? Or I wait until you have the occur export ready?

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Embark collect buffers have the default-directory of the target buffer, the one current prior to opening the minibuffer.

Sorry, the default-directory for collect buffers is a little fancier than what I said: in case you collected files from the minibuffer, it uses the directory part of the minibuffer input. Otherwise it is the default-directory of the buffer you opened the minibuffer from.

minad added a commit to minad/consult that referenced this issue Jan 9, 2021
@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Now I have to check where this occur-target property should be put, maybe we can put it such that it also works with consult-global-mark? I assume it is applied to the string of the line without the prefixes?

Yes, that would work. Occur also adds a line number, and there are also some other properties that occurs put on the text, but these are constant (independent of the marker) and I can handle all those extras in the embark exporter.

Oh, the line number I should compute from the marker, right? Because the prefix doesn't encode the line number, in the consult-mark case.

I will remove the embark-save-line-number action too.

Or I wait until you have the occur export ready?

I think this can proceed in parallel, maybe.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

Sorry, the default-directory for collect buffers is a little fancier than what I said: in case you collected files from the minibuffer, it uses the directory part of the minibuffer input. Otherwise it is the default-directory of the buffer you opened the minibuffer from.

Okay, makes sense!

I will remove the embark-save-line-number action too.

👍

Yes, that would work. Occur also adds a line number, and there are also some other properties that occurs put on the text, but these are constant (independent of the marker) and I can handle all those extras in the embark exporter.

I wonder - what kind of transformation do you have to do then? I thought you can just insert the candidates directly if I add the occur-target properties? Or are the occur mode lines also required to have a specific format?

Oh, the line number I should compute from the marker, right? Because the prefix doesn't encode the line number, in the consult-mark case.

I would recommend against doing that. If you really need to compute the line numbers at some places I should rather encode it via some different scheme into the prefix. Obtaining the line number from the marker position is really expensive, you have to open the buffer and then you have to call line-number-at-pos which parses all the lines. This is really bad.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

I tried to add these properties to the line candidates - I am not sure if 'consult-line-number and 'consult-marker are really sufficient. Don't you also need the line string without the prefixes in order to normalize the format? For example if exporting from consult-global-mark. Then you could just reformat the whole line. Alternatively I can mark the actual line substring with a property.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

I had forgotten about consult-global-mark! Yes, I do also need the line of text without the prefix.

Maybe instead of stripping unicode characters, you could make the prefix with a text property and I could strip that? (I think you proposed this before.) That would cover both the unicode prefix and the buffer name and colon in consult-global-mark.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Wait, didn't we say that text properties get lost somewhere along the way in #83? I'm suddenly worried about this whole text property approach.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

@oantolin No in this case it is no problem. It is only a problem for the return value of completing-read which cannot be trusted. But for the candidate export I don't see an issue. You should get the unmodified candidate list or does the completion system modify these?

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

I think you are right and it is only the return value of completing-read which is problematic.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

Well, for example selectrum applies its refinement functions. But as long as those do not mess up our properties we are good. About the other completion systems, I have no idea. But I guess default completion and icomplete are also not problematic.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Several completion styles including orderless apply faces, but I think other than that the text properties aren't modified, typically.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

OK, I just ran some tests and for collect/export the properties are present.

But not for actions. I wonder if I'm stripping the properties when I inject the candidate. Can't remember.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

I'll figure out the action parts, but that's irrelevant here, the point is that for collect and export, we can rely on text properties.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

The culprit in the case of actions is embark-target-top-minibuffer-completion. I figured this out by testing with selectrum that use a different target finder, and in that case text properties are preserved.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

Are we okay with this, can you fix it? For actions it does not matter right, there it is good if the actions are stripped before injection? Then I will push the patch which adds the properties.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Yes, the text property approach is fine for export (and for collect) too.

For actions the only place it would make a difference, I think, is that if you use the insert or save actions from consult-global-mark, I'll only strip the unicode prefix (as is done now), and not also strip the buffer name, etc.

I'll try to make the target finder for default minibuffer completion preserve text properties, because now I'm jealous that the target finder for selectrum does preserve them. 😛 But whether I am able to fix that or not, we can still do the occur-mode export.

@minad
Copy link
Contributor

minad commented Jan 9, 2021

Okay, here you go:

I hope I didn't mess up too many things during the refactoring.

@oantolin oantolin reopened this Jan 9, 2021
@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

I have an initial version of the exporter and it works perfectly for the single-buffer case of consult-line and consult-mark, and occur-edit-mode works well too!

For consult-global-mark the only thing missing actually is the little headers that tell you which buffer each line comes from. It currently says they all come from the first one. But each line does take you to the correct spot and even occur-edit-mode works!

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

And that's it, I added the header line at each buffer change, so that the export works well with consult-global-mark too now.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Okay @wd! So, @minad and I had a very long discussion that you don't need to follow, the end result is that with the latest versions of Embark and Consult, you can use embark-export from consult-line, vonsukt-outline, consult-mark or consult-global-mark and you get an honest occur-mode buffer, where you can press e to enter the built-in occur-edit-mode!

Please test and reopen if you have any trouble with it.

I have to say, I think we must have the infrastructure right if fancy additions like this are so easy!

@hmelman
Copy link

hmelman commented Jan 9, 2021

I silently followed it all and I'm very impressed. Well done.

@oantolin
Copy link
Owner

oantolin commented Jan 9, 2021

Thank you, @hmelman!

@minad
Copy link
Contributor

minad commented Jan 9, 2021

This is really great, I did not expect we could unlock this kind of functionality with only 37 added lines in Embark (and 4 lines in Consult)!

@wd
Copy link
Author

wd commented Jan 10, 2021

It's really impressive that your guys added this feature so fast.

I usually use multiple-cursors to edit results in the export buffer with ivy-occur-grep-mode. I can do the same thing in the export results of swiper and cousel-projectile-ag.

I can do the same thing with consult and embark now. Thanks.

@oantolin
Copy link
Owner

consult-outline followed by embark-export, makes for a pretty good table of contents.

@minad
Copy link
Contributor

minad commented Jan 10, 2021

*editable table of contents which stays intact even when lines are moved!

@oantolin
Copy link
Owner

And now navigable with {next,previous}-error. 😄

@wakamenod
Copy link

wakamenod commented Jun 5, 2021

Like @wd commented above, I also used to use counsel-rg together with ivy-occur for editing multiple files (like fixing typos in multiple files).
Thanks to this comment, I know realized that switching to ivy-occur-grep-mode still makes this possible even thought I've already switched from ivy/counsel to embark/consult.
Now I wonder I need to keep ivy for this purpose only, or are there any alternatives?

@oantolin
Copy link
Owner

oantolin commented Jun 5, 2021

@wakamenod You could try using consult-rigrep followed by embark-export which will produce a grep-mode buffer. If you also install the wgrep package, you can use it to edit the files from the buffer of search results.

@wakamenod
Copy link

@oantolin
I really appreciate your quick comment.
I tried wgrep package and it works as I expected.

Thanks so much!

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

5 participants