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

counsel-rg ivy-occur is broken on Windows #1817

Closed
mookid opened this issue Nov 23, 2018 · 7 comments

Comments

@mookid
Copy link
Contributor

commented Nov 23, 2018

At least on windows, jumping to a match does not work anymore on master.

To reproduce:
in the swiper repo, searching for cl-delete yields the following ivy-occur buffer:

-*- mode:grep; default-directory: "~/src/swiper/" -*-


5 candidates:
    .\counsel.el:1610:              (or (cl-delete default-directory (counsel-git-worktree-list)
    .\counsel.el:3533:    (set sym (cl-delete-duplicates
    .\counsel.el:3534:              (cl-delete-if-not counsel-yank-pop-filter (symbol-value sym))
    .\counsel.el:3565:    (set sym (cl-delete s (symbol-value sym)
    .\ivy.el:280:                   (cl-delete-duplicates

As a consequence, pressing RET fails to jump to the match.
A minimal patch to work around the issue is the following:

--- a/ivy.el
+++ b/ivy.el
@@ -4119,7 +4119,7 @@ When `ivy-calling' isn't nil, call `ivy-occur-press'."
        highlight
        help-echo "mouse-1: call ivy-action")
      str)
-    (insert (if (string-prefix-p "./" str) "" "    ")
+    (insert (if (or (string-prefix-p "./" str) (string-prefix-p ".\\" str)) "" "     ")
             str ?\n))
   (goto-char (point-min))
   (forward-line 4)

@mookid mookid changed the title counsel-rg ivy-occur is broken counsel-rg ivy-occur is broken on Windows Nov 23, 2018

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Nov 23, 2018

(insert (if (or (string-prefix-p "./" str) (string-prefix-p ".\\" str)) "" " ")

AKA

(if (string-match-p "\\`\\.[/\\]" str) "" "     ")

But this is looking increasingly brittle and hard-coded (platform-specific file name manipulation can and should be avoided!). Isn't there a more declarative way of achieving a similar effect?

@iquiw

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

Does --path-separator option of ripgrep help?

        --path-separator <SEPARATOR>
            Set the path separator to use when printing file paths. This defaults to your
            platform's path separator, which is / on Unix and \ on Windows. This flag is
            intended for overriding the default when the environment demands it (e.g.,
            cygwin). A path separator is limited to a single byte.
@mookid

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

But this is looking increasingly brittle and hard-coded (platform-specific file name manipulation can and should be avoided!). Isn't there a more declarative way of achieving a similar effect?

I am not advocating for the inclusion of that patch, just saying that this fixes my issue.
For a proper fix, let somebody write a proper PR.

@mookid

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

Does --path-separator option of ripgrep help?

I am not sure that ripgrep is the only backend affected by this.

@abo-abo abo-abo closed this in d5ffd67 Nov 24, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Nov 24, 2018

Thanks, I'm fine with applying the suggested patch.

The whole if statement is a hack anyway. If there's time for a refactor, ivy--occur-insert-lines should know if it's called by a grep-like caller or not and insert appropriately " " or "". Right now, we extract this info from the strings to be inserted.

@iquiw

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

swiper/ivy.el

Line 4126 in 7b6765b

(insert (if (string-prefix-p "\\`.[/\\]" str) "" " ")

Shouldn't it be string-match-p as in #1817 (comment) ?

abo-abo added a commit that referenced this issue Nov 28, 2018
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Nov 28, 2018

@iquiw Thanks, fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.