abo-abo / swiper

Ivy - a generic completion frontend for Emacs, Swiper - isearch with an overview, and more. Oh, man!
https://oremacs.com/swiper/
2.29k stars 338 forks source link

`ivy-update-candidates` does not work in `:action` #2775

Open nbfalcon opened 3 years ago

nbfalcon commented 3 years ago

This issue came up when I was trying to implement a counsel-kill-buffer command. Calling the action using ivy-call should remove it from the buffer list, since killing a buffer twice is impossible. However, for some strange reason, the old candidates were restored after my action was called. To reproduce, evaluate the following in a *scratch* buffer:

(ivy-read "> " '("1" "2" "q'")
          :action (lambda (a)
                    (message "M")
                    (ivy-update-candidates '("q" "5")))
          :require-match t
          :caller 'q)

Then call ivy-call. Afterwards, the candidate list should be "q" "5", but it was still "1 2 3".

Debugging this further (C-u C-M-x ivy-call), I noticed that the candidates were indeed updated after my action was called and stayed that way until the minibuffer was selected again, after which ivy--exhibit probably reset them again. Adding (setf (ivy-state-collection ivy-last) cands) at the start of ivy-update-candidates didn't help, either.

Even though internal-complete-buffer is handled specially in ivy--update-minibuffer, this bug also shows up if the former is used as CANDIDATES.

I suspect that somewhere around ivy--exhibit the list of candidates is saved and restored somehow, so that ivy--all-candidates and (ivy-state-collection ivy-last) get out of sync.

basil-conto commented 3 years ago

Would doing something like what ivy--kill-buffer-action and counsel-yank-pop-action-remove do work in your case?

The crux being that, like in #2715, ivy-update-candidates seems very alpha quality. :)

nbfalcon commented 3 years ago

Thank you, those functions (especially ivy--kill-current-candidate) are adequate for my use case. Should I close this issue? My problem was solved, but ivy-update-candidates should perhaps work in :action at some point.

basil-conto commented 3 years ago

Should I close this issue?

It's up to you.

My problem was solved, but ivy-update-candidates should perhaps work in :action at some point.

That depends on what ivy-update-candidates is intended to be used for. As I mention in #2715, it's a completely undocumented function that seems to have been spun up for some ad-hoc use case. Without further work on or documentation for it, I have no idea whom or what it's for. As far as I'm concerned, it's therefore some weird internal function that may or may not be changed in the future. But that's just me. ;)

zbelial commented 3 years ago

Hi @nbfalcon @basil-conto I'm trying to implement ivy support in consult (the issue is here), and I had the same problem. I'm not familiar with ivy's internal, but it seems that the problem is in ivy--filter, where it checks whether to use cache or not, as following

    (if (and
         ivy--old-re
         ivy--old-cands
         (equal re ivy--old-re))
        ;; quick caching for "C-n", "C-p" etc.
        ivy--old-cands

As you can see, if cache is usable, ivy--filter will just return ivy--old-cands, which is not synced with ivy--all-candidates set in ivy-update-candidates.

@nbfalcon If you make a small change to you code as follows, you'll get the expected result.

(ivy-read "> " '("1" "2" "q'")
          :action (lambda (a)
                    (message "M")
                    (setq ivy--old-cands nil)  ;; this makes ivy--filter not return cached candidates
                    (ivy-update-candidates '("q" "5")))
          :require-match t
          :caller 'q)

So, IMHO in ivy-update-candidates, we can do something to make ivy--old-cands invalid.

basil-conto commented 3 years ago

So, IMHO in ivy-update-candidates, we can do something to make ivy--old-cands invalid.

Thanks, sounds reasonable. But then like I said I'm not familiar with the intention/design behind ivy-update-candidates :).

zbelial commented 3 years ago

Thanks, sounds reasonable. But then like I said I'm not familiar with the intention/design behind ivy-update-candidates :). I see. But like you said, it seems that it's for some ad-hoc use case(it's only used in counsel-search), so maybe this is a good chance to make it better. :)