Skip to content

Commit

Permalink
ivy.el (ivy--occur-insert-lines): Fix for counsel-ag
Browse files Browse the repository at this point in the history 
The results of `ag' don't have "./" prepended like `rg' has.

Fixes #1841
  • Loading branch information
@abo-abo
abo-abo committed Dec 6, 2018
1 parent f6d71b9 commit be98b75
Showing 1 changed file with 4 additions and 1 deletion.
5 changes: 4 additions & 1 deletion ivy.el
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4123,7 +4123,10 @@ When `ivy-calling' isn't nil, call `ivy-occur-press'."
highlight
help-echo "mouse-1: call ivy-action")
str)
(insert (if (string-match-p "\\`.[/\\]" str) "" " ")
(insert (if (or (string-match-p "\\`.[/\\]" str)
(eq (ivy-state-caller ivy-last) 'counsel-ag))
""
" ")
str ?\n))
(goto-char (point-min))

This comment has been minimized.

Sign in to view

Copy link
@mookid

mookid Dec 7, 2018

Contributor

This breaks the navigation in ivy-occur buffers from ag: the ./ prefix hack allows :
https://github.com/abo-abo/swiper/blob/master/ivy.el#L4280

Plus, we create a coupling with the different tool behaviour: what if rg changes the way results are displayed? sould we support multiple executable versions?

I think that the proper fix is to prepend ./ if not already there; we get

  • decoupling with the way tools present results
  • simpler and more robust code: ivy-occur-press expects ./ prefixed candidates, and clients in counsel are in charge of putting it if necessary

This comment has been minimized.

Sign in to view

Copy link
@m-renaud

m-renaud Dec 7, 2018

Agreed, it is kinda strange for ivy to depend on specific callers of it.

Another option which doesn't involve changing the candidate strings (which is currently an indirect and somewhat brittle way to signal the desired behaviour) is to add another (optional) parameter to this function, say format, which is used to determine how the occur buffer is created. format could be one of wgrep | pretty | ... which would be well documented, and the callers could specify how they wanted the buffer to be created.

This comment has been minimized.

Sign in to view

Copy link
@m-renaud

m-renaud Dec 10, 2018

@mookid @abo-abo

Thoughts on the optional format parameter idea? If this sounds reasonable I could put together a PR implementing it if you don't have the time right now :)

This comment has been minimized.

Sign in to view

Copy link
@mookid

mookid Dec 10, 2018 •
edited
Loading

Contributor

Do you suggest to keep a customization variable (say, ivy-grep-format) to configure the display?

If you try that, note that:

  • format should be kept in sync with ivy-occur-press that drives the navigation: see the line
          (looking-at "\\(?:./\\|    \\)\\(.*\\)$"))
  • you need to keep wgrep working

I made a previous attempt for something similar: #1783 , this could be a start. No custom format there, but I did not try to make that work with wgrep.
(forward-line 4)
Expand Down

1 comment on commit be98b75

@m-renaud
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abo-abo Any chance this or could be extended to include swiper, swiper-many, and swiper-all? I think something like the following would work:

(or (string-match-p "\\`.[/\\]" str)
                    (eq (ivy-state-caller ivy-last) 'counsel-ag)
                    (eq (ivy-state-caller ivy-last) 'swiper)
                    (eq (ivy-state-caller ivy-last) 'swiper-many)
                    (eq (ivy-state-caller ivy-last) 'swiper-all))

Even better would be to provide a customization option where you could specify which source minibuffers you want to generate a wgrep compatible occur buffer for (for me it would be all of them).

Please sign in to comment.