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.el (ivy--reset-state): Filter sorted alist #1706
Conversation
Could you please resolve the merge conflict? |
Done, thanks. |
(if (and sort (setq sort-fn (ivy--sort-function caller))) | ||
(progn | ||
(setq sort nil) | ||
(sort (copy-sequence collection) sort-fn)) |
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.
Please be careful about removing copy-sequence
. In all likelihood, there's a purpose for them:
;; oops:
(let ((coll '(("b" . "1") ("a" . "2"))))
(ivy-with '(ivy-read "test:" coll
:sort t)
"C-m")
coll)
;; => ((#("b" 0 1 (idx 1)) . "1"))
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.
Please be careful about removing
copy-sequence
.
I was, but clearly not enough. I was expecting cl-remove-if-not
to allocate a new sequence, but I see now that it only does that on demand, which I don't think makes sense in the presence of cl-delete-if-not
.
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.
Wow, cl-remove
behaves totally differently to remove
:
(let* ((seq (list 1 2 3))
(sub (cl-remove 1 seq)))
(setcar sub 5)
seq) ; => (1 5 3)
I wonder if this is intentional or a bug.
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.
In any case, I've re-added the copy-sequence
.
Thanks. |
Strip idx property from entire candidate returned ivy.el (ivy-read): Strip idx property from entire candidate returned following commit 4ca8786 "ivy.el: Avoid modifying alist collection". ivy-test.el (ivy-read): Test idx property removal. Re: abo-abo#1706 Fixes abo-abo#1724
Fixes #1705
I've also simplified the surrounding code a bit in the subsequent two commits.