embark icon indicating copy to clipboard operation
embark copied to clipboard

Export location based consult results to grep buffer.

Open artemkovalyov opened this issue 2 years ago • 43 comments

I prefer wgrep over occur-edit-mode. I created an alternative exporter for consult-lines to use grep mode buffer. The benefits are:

  • using wgrep and its multi-line results editing capabilities
  • uniformity of results editing user experience for grep-like buffers.

I prefer a grep buffer for consult-lines because I use it a lot for narrowing down searches in an open buffer as opposed to grep-based searching tools from consult that constantly search over a directory. In-place editing is richer with wgrep and using one tool makes you ponder less about key combinations and how you enable in-place editing.

Related issues:

  • #47
  • #554

artemkovalyov avatar Nov 21 '22 11:11 artemkovalyov

@oantolin , what would be your recommendation for a more granular configuration to keep other tools like consult-outline, etc. , still using occur. With the current setup, other consult-location based tools will also use grep buffer for exporting. I don't mind that at all but would like to consult about your vision for this.

artemkovalyov avatar Nov 21 '22 11:11 artemkovalyov

Thank you for this. It is probably a good idea to provide alternative exporters.

However I have two questions or counter points I'd like to see discussed or sorted out first:

  1. Are the technical reasons strong enough to justify the addition?

Isearch also exports to occur, we cannot argue the grep/occur distinction away. Embark/Consult and the other packages are somewhat opionated. We try to avoid needless duplications and stay minimal. Therefore I wonder what specific aspects of grep are preferrable for you. Are there strong technical reasons? You can probably configure both occur and grep to behave similarly, if this is only about key bindings. I am kind of sceptical regarding arguments based on taste in contrast to technical reasons. Also note that the grep buffer is weaker out of the box than the occur buffer, if wgrep doesn't exist. Otherwise my suggestion would be to add this to the wiki.

  1. Is it reliable given that buffers and files are not the same and not necessarily in sync?

Using grep/wgrep on open buffers worries me a bit and may even lead to inconsistencies between the file and buffer. Are you sure that this behaves correctly? What happens if you use consult-line on a modified buffer and export? What happens then if you wgrep? What happens for buffers which are not backed by a file?

minad avatar Nov 21 '22 12:11 minad

Thanks for pointing out great questions and detailed write-up, @minad.

I totally support a minimalistic approach and focused nature of the Consult and Embark.

  1. Keybninding is not my main concern, but a side-effect. I knew from tools like rg.el how to use wgrep and after trying to use embark with consult it worked in a very similar and highly more interactive fashion with consult-grep. But I use consult-line for a single buffer search most of the time as it's faster and I have to filter through less content. I had to dig into google search and docs to figure out inline editing with occur. I saw I was not alone in asking about it. Then I tried to do a multiline edit in occur and failed. I think it's not supported., is it?
  2. The workaround clearly exists - I can use consult-ripgrep and then wgrep for multiline editing when needed. It's just that I like consult-line and use it automatically. I have to redo my searches with grep sometimes when I figure out occur wouldn't do the job for me on the current search-replace occasion. That's a matter of building a habit probably but I decided to write an exporter to consult-line because I didn't see obvious reasons why not.
  3. On the edge cases after shallow testing:
  • modified buffer - didn't see any side effects. Wgrep worked well, multiline edits were supported. After g you can update your search and re-export. All the changes by wgrep a factored in. Further wgrep updates on those changes are properly applied to the buffer.
  • buffer not backed by a file - editing and navigating don't work before you save the buffer. When navigating Emacs prompts for a file. This is not so frequent if an ever existing case for me because for the search-replace situation a buffer almost always exists. I can add a check if a file exists and prompt to save a buffer before running export in that case, as Magit does.
  • grep buffer is weaker - for search-replace it's not, same for exporting the data for further processing. It's weaker if you use it to navigate over a buffer, but for that, I'd never use exported occur buffer, I have Consult for that! Do you know cases when people rely on the export to navigate the file? Searching the web I saw that exporting to a buffer is mostly used for search-replace or capturing the data for further processing.

All in all, I can easily survive with grep and occur as opinionated choices, but Emacs is a bit about fine-tuning and this is a good tuning for me. I would prefer to have the option to configure where I want to export to accommodate my working pipeline. I think occur is a great default option. I'm happy to discuss how to make a reasonable overriding mechanism if a grep buffer is preferred. Other location-based Consult tools worked also worked fine with the exporter. See the consult-outline example. image

I think adding a check about saved buffer, in that case, is a good idea in a similar fashion Magit does it.

I'm happy to polish the PR and add more docs if you and @oantolin will consider the exporter useful. Otherwise, I'll maintain it in my configs as my own little hack and pray for no bigger API changes:)

Offtopic: What do you use for search-replace?

artemkovalyov avatar Nov 21 '22 15:11 artemkovalyov

I'll read through this conversation carefully later, but my first reaction is that it doesn't really make sense to have consult-line export to a grep-mode buffer because consult-line is meant to search through a buffer whether or not that buffer is visiting a file (and I, for one, do use it very often on non-file buffers!); grep-mode on the other hand is meant to list results only in files. So my first instinct is to reject this PR on semantic grounds: it seems actually incorrect to me to do this.

Of course, if you do not want the capability of exporting consult-line results from non-file buffers, you are free to reject this capability in your personal configuration, and since others may also want to forgo this feature in exchange for grep-mode it would be nice to have this documented in the wiki.

oantolin avatar Nov 21 '22 15:11 oantolin

On second thought, if we keep occur as the default and document the limitations of exporting to grep, I don't see why we couldn't keep both exporters.

oantolin avatar Nov 21 '22 16:11 oantolin

OK, I'm reading the conversation here now (which is not as long as I thought it was, there is a large screenshot which made me overestimate the length :)).

what would be your recommendation for a more granular configuration to keep other tools like consult-outline, etc. , still using occur

I'd suggest using marginalia to have consult-line report a different category, say consult-line-location, and configure the exporter only for that.

I see @minad brought up the same concern I have.

I'm also curious about what is better about wgrep compared to occur-edit-mode. I've always thought of wgrep as implementing occur-edit-mode but for grep. You mention "multiline edits", but I'm not sure what you mean by that, I can think of two meanings but both work fine here:

  1. Maybe you meant editing an occur buffer containing multiline results (as obtained by calling occur with a numeric prefix parameter specifying the number of context lines). That works here just fine, and it is probably not what you meant since there is no way to get embark export to give you context lines.

  2. Maybe you meant editing several different single-line results in an occur-mode buffer. That also works perfectly well here and I would say is probably the main use case for occur-edit-mode. It would be weird if that didn't work.

So either something is a little off in your configuration (and we can certainly try to figure out what), or you meant a third thing by "multiline edit".

Continuing, you say "I had to dig into google search and docs to figure out inline editing with occur", which is a perfectly valid way of finding out about it, but I want to point out that: (1) that would be the same for wgrep, people need to find out about it somehow in order to use it; (2) for wgrep finding out necessarily involves the internet, but occur-edit-mode is built-in and well-documented inside Emacs, so no googling is required to find it.

I'm glad you looked into @minad's questions, and your answers would form the backbone of documenting the pitfalls of using grep-mode export for consult-line. I do have one minor correction, you say "grep buffer is weaker - for search-replace it's not". But it definitely is weaker at least in the sense that editing requires the third-party wgrep package, while occur-edit-mode is built-in.

I think that on balance, the first thing to figure out is why whatever you meant by "multiline edits" are not working for you with occur-edit-mode. If we fix that you may not even feel a need for grep export anymore.

oantolin avatar Nov 21 '22 16:11 oantolin

My initial reaction was also to reject because of the separation grep/occur and file/buffer.

However if grep+wgrep work well and the grep export can be seen as a generalization of the occur export somehow. Then why not? However I see it as a requirement that we don't introduce bugs by adding this (handling non file backed buffers, buffer/file out of sync, ...). The exporter must be sufficiently robust in most (or all) scenarios.

Not sure if we had this discussion before but maybe we also need a way to select between multiple exporters.

minad avatar Nov 21 '22 16:11 minad

@oantolin Iirc there was a feature request for export with context lines to occur.

minad avatar Nov 21 '22 16:11 minad

Do you know cases when people rely on the export to navigate the file?

Forgot to answer this: I do that fairly often. I use it when I need to make a bunch of changes everywhere some string appears, for example, if I'm programming and changed a function signature and need to consider how best to update each call-site, or if I'm writing some math and decided to change the name of a concept I'm defining. What I like about using occur for these situations is that the occur buffer works like a temporary todo list containing all the spots where I need to change something; and I can navigate among the spots with next-error and previous-error, I don't even need to switch to the occur mode buffer.

Of course, in these use cases I'm editing an actual file so grep would do just as well. But I do sometimes use consult-line in the list-packages buffer and export to an occur-mode buffer which I treat as a todo list of packages I wish to check out (I do this when I want to search both package names and one-line descriptions, for just packages names I prefer to use describe-package). But this use case could easily be replaced by consult-focus (which I also sometimes use for the same purpose, I'm inconsistent that way).

oantolin avatar Nov 21 '22 16:11 oantolin

@minad said:

Not sure if we had this discussion before but maybe we also need a way to select between multiple exporters.

You mean selecting at export time? That could be a nice to have. A first idea on the UI would be that if (alist-get embark-exporters-alist type) is a list (whose car is not lambda or closure... 🙄), then you get prompted for which exporter you want. But it may be premature to do that, we seem to have managed with a single "best" exporter for each type of targets so far. In this particular case, I'm still not sure I understand why occur-edit-mode isn't good enough for @artemkovalyov.

Iirc there was a feature request for export with context lines to occur.

Yes, there was, and I do think that's a good idea, I just forgot about it. I guess a first step would be to pass the prefix argument to the exporter function somehow, probably just as the value of prefix-arg (which I think currently gets reset to nil before the exporter even runs).

oantolin avatar Nov 21 '22 16:11 oantolin

@oantolin Yes, I'd also like to understand the real value of this addition better and why @artemkovalyov isn't happy with occur and occur-edit-mode. I don't think you can "run away" from occur given that it is also baked into isearch. Just adding a grep export because we can and because of some consistency details doesn't seem justified (we have the wiki for that), considering that we introduce new problems given non-file backed buffers and given that wgrep is needed to reach parity with occur.

minad avatar Nov 21 '22 16:11 minad

@oantolin, I think you didn't get what multi-line edit means. It's about the ability to add lines in an exported buffer that are committed to the original buffer (file).

Here are examples with Wgrep and occure-edit

  • In Wgrep new lines are handled perfectly well and that's a very useful feature (at least to me) image

  • In occur-edit I can only edit the exported line, I can't remove it or add new lines image

artemkovalyov avatar Nov 21 '22 17:11 artemkovalyov

@artemkovalyov That's indeed an interesting feature! I wasn't aware of that. I think occur-edit-mode should be improved such that this is also supported.

minad avatar Nov 21 '22 17:11 minad

You're absolutely right that I hadn't understood what you meant, @artemkovalyov! (I suspected as much.) That is a cool feature I did not know wgrep had! Like @minad, I think the right approach is to enhance occur-edit-mode to support this, but in the meantime, I do think that for that feature it is worth having the grep exporter even if it only works properly for file-backed buffers.

oantolin avatar Nov 21 '22 17:11 oantolin

On 11/22/22 18:10, Omar Antolín Camarena wrote:

@.**** commented on this pull request.


In embark-consult.el https://github.com/oantolin/embark/pull/560#discussion_r1029619030:

  •         (contents (propertize (embark-consult--strip line)))
    
  •         (this-buf (marker-buffer loc)))
    
  •      (unless (eq this-buf last-buf)
    
  •        (insert (propertize "Exported grep results:\n\n" 'wgrep-header t))
    
  •        (setq last-buf this-buf))
    
  •      (insert (concat (file-name-nondirectory (buffer-file-name this-buf)) ":" lineno contents "\n"))))
    
  •  (goto-char (point-min))
    
  •  (grep-mode)
    
  •  (setq next-error-last-buffer buf)
    
  •  ;; Set up keymap before possible wgrep-setup, so that wgrep
    
  •  ;; restores our binding too when the user finishes editing.
    
  •  (use-local-map (make-composed-keymap
    
  •                  embark-consult-revert-map
    
  •                  (current-local-map)))
    
  •  (setq-local wgrep-header/footer-parser #'ignore)
    
  •  (when (fboundp 'wgrep-setup) (wgrep-setup)))
    

You're right, of course. I just thought it shouldn't be a problem since I'd expect few matching lines compared to the number of lines in the buffer, so the slow step would actually be the initial gathering of candidates in consult-line. I mostly wanted to reuse more code, in particular to reuse the line insertion loop, so maybe the best option is a callback but one that is called per line.

Hmm, I would probably go with the most efficient solution, a single call. I often export many candidates from consult-grep/ripgrep. As an alternative we could also use a macro with a BODY.

minad avatar Nov 22 '22 17:11 minad

Oh, @artemkovalyov have you signed the FSF copyright assignment papers?

oantolin avatar Nov 23 '22 18:11 oantolin

Oh, @artemkovalyov have you signed the FSF copyright assignment papers? I assume there's no GH plugin to do that? Do you have a simple instruction or link? Googling confuse me :)

artemkovalyov avatar Nov 27 '22 22:11 artemkovalyov

@minad, @oantolin, please, take another look at the implementation. I'm not an experienced LISPer but I tried to follow your recommendations. I had to create wrappers for actual exporters to preserve their APIs but split the implementation into a buffer setup and processing of results to make them reusable.

If you have better ideas, please, share. I also renamed an exporter to occur into a location, as the destination buffer can now be different. It seems to play nicely with the naming strategy and everything before that also uses location for the range of location-enabled consult functions.

artemkovalyov avatar Nov 27 '22 22:11 artemkovalyov

@minad, I made sure the buffers exported to grep buffer are visiting a file. Here are a couple of screenshots: image image All the temporary buffers like scratch and others will be excluded from consult-line-multi for example. After we agree on the implementation and details I'll add some WIKI about this exporter. The switch of the behavior is done via defcustom var for now.

artemkovalyov avatar Nov 27 '22 22:11 artemkovalyov

@artemkovalyov Thanks, this looks quite good. I've added a few comments, mostly details.

More importantly, please try to get the FSF assignment, see the description at https://protesilaos.com/emacs/modus-themes#h:111773e2-f26f-4b68-8c4f-9794ca6b9633.

minad avatar Nov 28 '22 11:11 minad

@artemkovalyov What's going on in the second screenshot of https://github.com/oantolin/embark/pull/560#issuecomment-1328360294? Why do we see the results of the embark export buffer? It shouldn't be backed by a file.

minad avatar Nov 28 '22 11:11 minad

@artemkovalyov Thanks, this looks quite good. I've added a few comments, mostly details.

More importantly, please try to get the FSF assignment, see the description at https://protesilaos.com/emacs/modus-themes#h:111773e2-f26f-4b68-8c4f-9794ca6b9633.

On it. I'll update you when it's ready. Take another look at the code changes when you have time.

artemkovalyov avatar Nov 29 '22 18:11 artemkovalyov

This looks pretty good, @artemkovalyov, but I unfortunately I won't be able to merge it until you've signed the FSF copyright assignment, since Embark is now on GNU ELPA.

oantolin avatar Nov 30 '22 02:11 oantolin

This looks pretty good, @artemkovalyov, but I unfortunately I won't be able to merge it until you've signed the FSF copyright assignment, since Embark is now on GNU ELPA.

Roger that, @oantolin, I've sent the form already and hope to get the agreement soon. Should I just share it with you after I receive it and sign? In the meanwhile I'll install it from my fork to catch UX issues if any.

artemkovalyov avatar Nov 30 '22 15:11 artemkovalyov

Should I just share it with you after I receive it and sign?

That's not necessary, just let me know when it's done. I think the FSF copyright assignment is usually either very easy to get or almost impossible, depending on whether your employer has any right to your Emacs code and how they feel about the FSF. 😄

In the meanwhile I'll install it from my fork to catch UX issues if any.

Yes, good idea.

oantolin avatar Nov 30 '22 16:11 oantolin

@oantolin, I have my SFS assignment done, the number is RT: 1896958. I see the branch is in conflict now. I'll see what's changed and hopefully resolve them.

artemkovalyov avatar Dec 23 '22 15:12 artemkovalyov

I'm glad you got the copyright assignment worked out! I don't think too much has changed so the code shouldn't be hard to update.

oantolin avatar Dec 23 '22 16:12 oantolin

@oantolin, I've brushed up my PR and tested both grep and occur based export to work properly for viewing and editing. While resolving merge conflicts I've seen you added highlighting, which is cool. I slightly updated the implementation to use (save-excursion ...) instead of (goto-char ...). I hope you don't mind as it doesn't change the logic and puts the point right after the header when the buffer is ready.

artemkovalyov avatar Apr 13 '23 12:04 artemkovalyov

@artemkovalyov Thanks for the refactoring. From my side this looks good. However I haven't tested this yet. @oantolin can surely give you some additional comments.

minad avatar Apr 13 '23 12:04 minad

@oantolin, here are some test results with both consult-line and consule-multi-line image image image

artemkovalyov avatar Apr 14 '23 13:04 artemkovalyov