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

Selection of first match after changes to the input doesn't always work #253

Closed
tsdh opened this Issue Oct 6, 2015 · 4 comments

Comments

Projects
None yet
3 participants
@tsdh
Contributor

tsdh commented Oct 6, 2015

After 126158d, the first match should be selected after the input has changed with the exception of swiper. It works in the case I constructed in #231 but not always. Here's a test case which currently fails:

(should (equal
         (ivy-with '(ivy-read "pattern: " '("aba"
                        "baa"
                        "abc"))
                   "a b <down> <backspace> C-m")
         "aba")) ;; What's actually selected now is baa

Here I'd expect the first item "aba" to be selected because the first matching element should be selected after changes to the input. What's actually selected is "baa" which is neither the first match nor the last match so completely implausible. So that's a bug but I also have two other points.

  1. I agree with @PythonNut's comment in #231 in that one might want to customize that behavior on a per-command basis. Right now swiper and counsel-git-grep are exceptions identified by the last state having the unwind function swiper--cleanup, but testing for some specific unwind function seems to be the wrong thing to do. So I suggest to introduce some defcustom ivy-select-first-after-change-list (I'm not convinced of the name) and then changing the test to (memq this-command ivy-select-first-after-change-list).
  2. I think that once a user switched candidates explicitly with C-n/C-p/<up>/<down>, the selection of the first candidate after changes should be disabled, i.e., the selection should stick to the last one in case it is still valid. If that were implemented, the test case above should expect "abc" instead of "aba".
@PythonNut

This comment has been minimized.

Show comment
Hide comment
@PythonNut

PythonNut Oct 6, 2015

Contributor

I think that once a user switched candidates explicitly with C-n/C-p//, the selection of the first candidate after changes should be disabled, i.e., the selection should stick to the last one in case it is still valid. If that were implemented, the test case above should expect "abc" instead of "aba".

What would you propose doing if the previously selected candidate no longer matched the new input? Something like:

(should (equal
         (ivy-with '(ivy-read "pattern: " '("aba"
                        "baa"
                        "abc"))
                   "a C-n C-n b C-m ")
         "???")) ;; What should this be?
Contributor

PythonNut commented Oct 6, 2015

I think that once a user switched candidates explicitly with C-n/C-p//, the selection of the first candidate after changes should be disabled, i.e., the selection should stick to the last one in case it is still valid. If that were implemented, the test case above should expect "abc" instead of "aba".

What would you propose doing if the previously selected candidate no longer matched the new input? Something like:

(should (equal
         (ivy-with '(ivy-read "pattern: " '("aba"
                        "baa"
                        "abc"))
                   "a C-n C-n b C-m ")
         "???")) ;; What should this be?
@tsdh

This comment has been minimized.

Show comment
Hide comment
@tsdh

tsdh Oct 6, 2015

Contributor

If the last one isn't valid anymore, then select the first one again, I.e., aba.

Contributor

tsdh commented Oct 6, 2015

If the last one isn't valid anymore, then select the first one again, I.e., aba.

abo-abo added a commit that referenced this issue Oct 7, 2015

@abo-abo

This comment has been minimized.

Show comment
Hide comment
@abo-abo

abo-abo Oct 7, 2015

Owner

@tsdh
The test that you mention was failing because of a typo (ivy-index instead of ivy--index).

(memq this-command ivy-select-first-after-change-list)

I'd like try to avoid this approach in the future. It removes the flexibility of user commands calling ivy functions, and also ivy-resume. When the collection is a list of strings (like swiper works currently), it's hard to uniquely identify it. Maybe some wrapper could be added, similar to avy-with, that would let each command pass its own name to ivy-read. What do you think? Is there a better approach for ivy-read to uniquely identify its collection type? So far, this is my best idea:

(ivy-read
 "Swiper: "
 candidates
 :initial-input initial-input
 :keymap swiper-map
 :preselect preselect
 :require-match t
 :update-fn #'swiper--update-input-ivy
 :unwind #'swiper--cleanup
 :re-builder #'swiper--re-builder
 :history 'swiper-history
 :caller 'swiper)

This would mean that each ivy-read would have to be updated with :caller eventually. But then all ...-alist type customizations would dispatch on :caller instead of this-command.

Owner

abo-abo commented Oct 7, 2015

@tsdh
The test that you mention was failing because of a typo (ivy-index instead of ivy--index).

(memq this-command ivy-select-first-after-change-list)

I'd like try to avoid this approach in the future. It removes the flexibility of user commands calling ivy functions, and also ivy-resume. When the collection is a list of strings (like swiper works currently), it's hard to uniquely identify it. Maybe some wrapper could be added, similar to avy-with, that would let each command pass its own name to ivy-read. What do you think? Is there a better approach for ivy-read to uniquely identify its collection type? So far, this is my best idea:

(ivy-read
 "Swiper: "
 candidates
 :initial-input initial-input
 :keymap swiper-map
 :preselect preselect
 :require-match t
 :update-fn #'swiper--update-input-ivy
 :unwind #'swiper--cleanup
 :re-builder #'swiper--re-builder
 :history 'swiper-history
 :caller 'swiper)

This would mean that each ivy-read would have to be updated with :caller eventually. But then all ...-alist type customizations would dispatch on :caller instead of this-command.

@tsdh

This comment has been minimized.

Show comment
Hide comment
@tsdh

tsdh Oct 7, 2015

Contributor

Yes, I've also thought about extending the ivy state with a field for the caller, so we may assume that if two people think the same individually, it can't be too wrong.

BTW right now, the ...-alist type customizations don't dispatch on this-command but on the function being the ivy-state-collection.

I think, the new :caller field should be initialized with this-command in case it is not provided. Then you could have ...-alist entries for, e.g., switch-to-buffer instead of the rather obscure internal-complete-buffer.

However, dispatching on ivy-state-collection is still useful because it allows for having one internal-complete-buffer entry which can act as a default for all commands which read a buffer name instead of having to add entries with the same value for switch-to-buffer, pop-to-buffer, etc. So for all the ...-alist variables, I'd test if there's an entry for the current ivy-state-caller, and if not, check if there's an entry for ivy-state-collection if that's a function.

Contributor

tsdh commented Oct 7, 2015

Yes, I've also thought about extending the ivy state with a field for the caller, so we may assume that if two people think the same individually, it can't be too wrong.

BTW right now, the ...-alist type customizations don't dispatch on this-command but on the function being the ivy-state-collection.

I think, the new :caller field should be initialized with this-command in case it is not provided. Then you could have ...-alist entries for, e.g., switch-to-buffer instead of the rather obscure internal-complete-buffer.

However, dispatching on ivy-state-collection is still useful because it allows for having one internal-complete-buffer entry which can act as a default for all commands which read a buffer name instead of having to add entries with the same value for switch-to-buffer, pop-to-buffer, etc. So for all the ...-alist variables, I'd test if there's an entry for the current ivy-state-caller, and if not, check if there's an entry for ivy-state-collection if that's a function.

@abo-abo abo-abo closed this in f9df75e Oct 8, 2015

abo-abo added a commit that referenced this issue Oct 8, 2015

Move setq ivy--index to ivy--recompute-index
* ivy.el (ivy--recompute-index): Move the setq statement here, so that
  the customization functions have less internal variables to deal with.
(ivy-recompute-index-swiper): Update.
(ivy-recompute-index-zero): Update.

Re #253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment