-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
base: master
Are you sure you want to change the base?
Conversation
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)) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
(let ((proj-and-cmd (counsel--git-grep-cmd-and-proj cmd)) | ||
(enable-recursive-minibuffers t) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
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 thegit-grep
command.