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.el (counsel-git-grep-switch-cmd): allow recursive minibuffer #2723

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

brownts
Copy link
Contributor

@brownts brownts commented Nov 11, 2020

Fixes #2720.

Locally sets enable-recursive-minibuffers so that if this is not set in the user's environment, it won't cause an error when attempting to configure the git-grep command.

counsel.el Outdated
(unless (ivy-state-dynamic-collection ivy-last)
(setq ivy--all-candidates
(all-completions "" 'counsel-git-grep-function))))
(let ((enable-recursive-minibuffers t))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why here and not in counsel-git-grep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@basil-conto, I was attempting to follow precedent of how this was applied in the past. For example, enable-recursive-minibuffers is also set in counsel-cd, counsel-git-grep-query-replace, etc. To your point, those technically didn't have to be set there and instead could have been set higher at the counsel-ag and counsel-git-grep level. I'm guessing the reason it was done this way in the past is that the setting was performed local to where the need was. In other words, enable-recursive-minibuffers is not required to be set at the counsel-ag or counsel-git-grep level for basic functionality, as it typically works fine there. It's only when something is invoked from their key maps which might need to use the minibuffer again (such as counsel-cd or in this case counsel-git-grep-switch-cmd).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how you arrived at this precedent, since there are several top-level Counsel commands that bind the user option. My impression is that the addition of all of these bindings has been ad-hoc and not systematic until now. Ideally Ivy would solve this problem once and for all, rather than requiring us to exhaustively list all places that might need recursive completion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the precedent was just ad-hoc then, as you say...it was more of a guess/observation. I don't have a problem moving this up to the counsel-git-grep level. Although, that brings up the bigger question of whether this should be applied at counsel-ag as well and the settings removed from counsel-cd and counsel-git-grep-query-replace. Looking in counsel.el, there are 7 other usages of this binding in the following locations:

  • counsel-describe-variable
  • counsel-describe-function
  • counsel-describe-symbol
  • counsel-info-lookup-symbol
  • counsel-git-grep-query-replace
  • counsel-cd
  • counsel-minibuffer-history

In your opinion, of the above list, is it just counsel-cd and counsel-git-grep-query-replace that are incorrectly placed? I can remove them from these locations and instead add the setting to both counsel-ag and counsel-git-grep.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Until a system is put in place, I'd say you can leave existing bindings where they are, especially since they're relied on by different callers.

counsel-git-grep-switch-cmd, on the other hand, is more closely tied to counsel-git-grep, so the choice of what to do with the binding is more straightforward. My vote is for putting it in counsel-git-grep, for the benefit of all functions it ends up invoking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, thanks for the feedback @basil-conto. I'll move the change to counsel-git-grep until a better system is put in place.

counsel.el Outdated
Comment on lines 1511 to 1512
(let ((proj-and-cmd (counsel--git-grep-cmd-and-proj cmd))
(enable-recursive-minibuffers t)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think counsel--git-grep-cmd-and-proj can use the minibuffer, so maybe enable-recursive-minibuffers should come first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll move it up. Thanks.

Copy link
Collaborator

@basil-conto basil-conto left a comment

Choose a reason for hiding this comment

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

Thanks!

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

Successfully merging this pull request may close these issues.

counsel-git-grep-switch-cmd does not set enable-recursive-minibuffers
2 participants