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

ivy-update-candidates: support alists #2715

Open
nbfalcon opened this issue Nov 7, 2020 · 5 comments · May be fixed by #2716
Open

ivy-update-candidates: support alists #2715

nbfalcon opened this issue Nov 7, 2020 · 5 comments · May be fixed by #2716

Comments

@nbfalcon
Copy link
Contributor

nbfalcon commented Nov 7, 2020

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.

nbfalcon added a commit to nbfalcon/swiper that referenced this issue Nov 7, 2020
Elements of CANDS are eventually passed trough to `ivy--format-minibuffer-line',
which errors out as it expects a string, not conses. Modify it to accept conses,
rendering their `car'.

Fixes abo-abo#2715.
@nbfalcon nbfalcon linked a pull request Nov 7, 2020 that will close this issue
@basil-conto
Copy link
Collaborator

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
Copy link
Contributor Author

nbfalcon commented Nov 8, 2020

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
Copy link
Contributor Author

nbfalcon commented Nov 8, 2020

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
Copy link
Contributor Author

nbfalcon commented Nov 8, 2020

It works with my PR, but not without it.

@basil-conto
Copy link
Collaborator

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.

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 a pull request may close this issue.

2 participants