search

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`: support alists #2715

Open nbfalcon opened 3 years ago

nbfalcon commented 3 years ago

ivy-update-candidates currently errors if an alist is passed to it. This breaks my lsp-ivy refactor.

The problem lies in ivy--format-minibuffer-line, which calls copy-sequence on its argument, which is an element of the CANDS list passed to ivy-update-candidates. copy-sequence expects a string, not a cons, so errors out. The fix is trivial, a single line at the start of ivy--format-minibuffer-line that checks if the element is a cons or a string:

(str (if (consp str) (car str) str))

However, I am unsure whether this is the right way to go about this, because it is a bit ad-hoc: ivy-update-candidates would not behave consistently with ivy-read, as its CANDS field cannot be a hash-table, .... Also, it has no docstring, meaning it is unclear what format its CANDS field should have. I'd like to not have to resort to text-properties for this.

I have provided a PR with my proposal. Copyright shouldn't be an issue, as a single line (my first contribution to this project) is < 15 lines.

basil-conto commented 3 years ago

I'm not really familiar with these murky depths of Ivy, so I could be very wrong, but I was under the impression that ivy--all-candidates is meant to be a flat list of strings. If this is the case, then the problem lies with ivy-update-candidates: it's either not intended to accept an alist, or it doesn't currently support alists because it just sets ivy--all-candidates to its argument CANDS.

nbfalcon commented 3 years ago

Either solutions sound non-satisfying: we need ivy-update-candidates to support alists, and to keep the cdr so that it can be used to get a complex result. Using assoc or abusing text-properties is something I wanted to avoid. With my patch, my modifed version of lsp-ivy works without issues.

nbfalcon commented 3 years ago

Here is an example to reproduce:

(ivy-read
 "Test: "
 (lambda (&rest _)
   (run-with-idle-timer
    0.2 nil (lambda () (ivy-update-candidates '(("1" . 0) ("2" . 5)))))
   nil))
nbfalcon commented 3 years ago

It works with my PR, but not without it.

basil-conto commented 3 years ago

Either solutions sound non-satisfying: we need ivy-update-candidates to support alists

Then I'm afraid we'll have to wait for @abo-abo to pitch in, since I have no idea what the intended design and use of the undocumented ivy-update-candidates was.