Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve completing-read-multiple (also fixes #80) #74
Improve completing-read-multiple (also fixes #80) #74
Changes from 2 commits
6684a32
6e8679e
497cdb9
c357bbe
4450ae0
346562e
004a86e
1bfced1
9a25a5a
1148e83
e2d105f
14632bf
24a9d1a
a31b82b
83bd44c
f77f244
9eb3b27
1436236
5484048
bd792ac
e51851b
e7eb34a
834555a
1bb0020
db8d760
246a197
5a50669
9bd00a0
8442c4b
b781611
4fa0c87
5884388
d093187
6eae7a6
1aa4c0f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Would
cl-remove-if
do the same thing faster?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.
The problem is when I use:
and try to input multiple candidates the sequence gets corrupted somehow, I don't understand why this is. To test this use it with the definition above and call
describe-face
. Look at the number of candidates and afterwards add one face withTAB ,
now look at the number of candidates again: they shrinked in size more than one.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.
This is probably because
cl-remove-if
is special-cased to return the same object if there is nothing to remove. Then something downstream destructively modifies the collection.I looked into this and found that the docstring of
selectrum-read
was not really clear on whether the collection might be modified. I fixed this problem. With the latest commit, I think using the code you have right now (cl-delete-if
pluscopy-sequence
) should work properly and should be the right thing to do. Let me know.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.
If the collection is a function (as in this example) the returned candidates can still later be modified by the sorting function (even if
may-modify-candidates
was not provided), so ifmay-modify-candidates
was not provided maybe we should copy the returned values before doing the sort in post-command-hook, too?Another thought I had is that even when
may-modify-candidates
was provided it can be confusing that the candidates returned by the function above can be modified and you can't just reuse the value inside the closure. Maybe we should copy the returned candidates by default,copy-sequence
is pretty fast and I think this probably wouldn't be much of a problem and would avoid that people would need to do it manually in cases like above.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.
The problem is that
copy-sequence
is fast, but it generates a lot of garbage due to duplicating the collection, and garbage collection really kills latency. What's slow in Elisp is really not obvious at first glance.If we don't copy the candidates returned from a function collection when MAY-MODIFY-CANDIDATES is nil, that's 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.
Ah, yes thanks for pointing that out, totally forgot about gc!
I will open an issue for that later.