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

Add environment variable completion to counsel-find-file #1932

Merged
merged 1 commit into from Feb 14, 2019

Conversation

@CeleritasCelery
Copy link
Contributor

@CeleritasCelery CeleritasCelery commented Feb 12, 2019

Closes #776. In that issue @Henry created some code to add environment variable completion to counsel-find-file. @abo-abo asked if he or @TauPan could make a PR. Since it has been several months, I took the opportunity to make a PR. I also simplified the code and made it more performant.

counsel.el Outdated Show resolved Hide resolved
ivy.el Outdated Show resolved Hide resolved
@CeleritasCelery
Copy link
Contributor Author

@CeleritasCelery CeleritasCelery commented Feb 12, 2019

I moved back to the original implementation after applying feedback. If @abo-abo or someone knows how to make the quitting recursive minibuffers work in the post-command-hook we can move to the ivy--exhibit solution instead. The code is at CeleritasCelery/swiper@956fcfd

@CeleritasCelery
Copy link
Contributor Author

@CeleritasCelery CeleritasCelery commented Feb 14, 2019

I realized that this implementation does not work with read-file-name-internal. While we could add this to the that file map, I feel like the magic solution would work best if we could get it working correctly.

@abo-abo abo-abo merged commit ef78fb6 into abo-abo:master Feb 14, 2019
1 check passed
abo-abo added a commit that referenced this issue Feb 14, 2019
Suppose an (ivy-read "test: " '("1" "2" "3")) is done when another
`ivy-read' is in progress. In that case, after the first `ivy-read'
returns a string, we should restore the state of the first `ivy-read'.

Re #1932
abo-abo added a commit that referenced this issue Feb 14, 2019
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Feb 14, 2019

@CeleritasCelery Thanks, please test.

I've re-written a portion of your code to simplify it. I think it now falls into the 15 lines limit that's exempt from an Emacs CA. But you'll need a CA for future (hopefully) contributions.

@CeleritasCelery
Copy link
Contributor Author

@CeleritasCelery CeleritasCelery commented Feb 14, 2019

Thanks @abo-abo. I have already applied for a CA. Hear it takes about a month.

I am impressed how much you were able to simplify what I had added.

I only see two issues:

  1. if navigating to a directory via env variable, it will only navigate to parent directory and select the variable. Most environment variables don't have a trailing slash. That is why I added this code
(when (file-accessible-directory-p path)
    (setq path (file-name-as-directory path))
  1. This environment variable completion does not work in the read-file-name handler. I looked into how to make this work but was not able to figure it out. I added this to the magic handlers in ivy--exhibit but then you can't abort the env read.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Feb 14, 2019

@CeleritasCelery I've made a change to ivy.el with a recent commit, after that e.g. read-file-name works if I bind $ in ivy-minibuffer-map.

Please add the path expansion check as a new PR.

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
Suppose an (ivy-read "test: " '("1" "2" "3")) is done when another
`ivy-read' is in progress. In that case, after the first `ivy-read'
returns a string, we should restore the state of the first `ivy-read'.

Re abo-abo#1932
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants