Improve completing-read-multiple (also fixes #80)

[clemera]

This PR implements an alternate replacement for completing-read-multiple which mimics the default interface more closely and makes it more convenient for selecting multiple candidates via completion.

I requote oantolins critique of the current behaviour here for easier reference:

- Selecting the first n-1 candidates with M-RET and then the last one with RET
  feels a bit asymmetric. I think I instictively expected that I would select
  all n candidates with M-RET and then RET would confirm the selection without
  adding an extra (n+1)-st candidate.

- After pressing M-RET the contents of the minibuffer don't change. This has
  positive and negative aspects:

- 1. On the positive side, the list of candidates doesn't suddenly change, so if
  you wanted to M-RET a couple of other nearby candidates you can easily get to
  them with the arrow keys; on the

- 2. On the negative side, it makes it pretty inconvenient to use completion! If
  you want to now match some other candidate you often have to delete everything
  you wrote in the minibuffer to do so.

- You are often flying blind: usually most of your selections aren't visible!
  (Naturally so, if you scroll around the list they can scroll off, if you
  type text to match your next selection your previous ones are unlikely to
  still match.)

Contrast that last point with the built-in method, where all your selections are
clearly visible in the minibuffer, separated by commas. It is also very
convenient to use completion in the built-in method: to add an item, type a
comma and you can start completing again from scratch!

All in all it feels like the completing-read-multiple selectrum interface is
designed for the case of a tiny list of candidates you can scroll around in
full, not for completion.

I have left the previous multi selection feature untouched because I'm not sure how you want to proceed with this.

To test the new version call describe-face then insert candidates with TAB and press , to proceed and add more. Finally submit input with RET. This way it works exactly like the stock Emacs UI only that it's much better ;)

This new method also has the additional benefits that history for completing-read-multiple looks the same as without selectrum-mode and you can select history items of previous multi selections.

[clemera]

Currently this works with recursive minibuffer, too but only by accident! crm-completion-table is used for checking internally if completing-read-multiple is used. But let binding it around the selectrum-read call keeps it bound for recursive calls, so the proper way would be to add another argument to selectrum-read but I will wait for feedback and your opinion how to proceed before I continue changing anything related to that.

[raxod502]

This is clearly much better than the proof-of-concept that I hacked together previously. Thank you for working on an improvement. You can get rid of all the multiple-selection stuff from before in favor of this approach.

so the proper way would be to add another argument to selectrum-read

Yes, I agree. My previous approach already has an extra argument and a state variable for tracking whether multiple selection is enabled. Perhaps it would be best to just use that, rather than checking crm-completion-table? (But of course we can still set crm-completion-table, for API compliance.)

[raxod502]

Would cl-remove-if do the same thing faster?

[clemera]

The problem is when I use:

(cl-remove-if
 (lambda (i)
   (member i inputs))
 coll)

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 with TAB , now look at the number of candidates again: they shrinked in size more than one.

[raxod502]

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 plus copy-sequence) should work properly and should be the right thing to do. Let me know.

[clemera]

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

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 if may-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.

[raxod502]

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.

the candidates returned by the function above can be modified and you can't just reuse the value inside the closure

If we don't copy the candidates returned from a function collection when MAY-MODIFY-CANDIDATES is nil, that's a bug.

[clemera]

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.

Ah, yes thanks for pointing that out, totally forgot about gc!

If we don't copy the candidates returned from a function collection when MAY-MODIFY-CANDIDATES is nil, that's a bug.

I will open an issue for that later.

[clemera]

Should I replace the current description of selectrum-completing-read-multiple in CHANGELOG or add another entry describing that the previous version has changed?

[clemera]

Okay I removed all the code previously used for the old approach to multiple candidate selection and updated changelog. Should all be good now.

[clemera]

I've written a guide for changelog authorship that covers this and other notes.

Thanks this is helpful! The "why 79" reference made me laugh, I had to test it to see it's true :D

[raxod502]

This is splendid, a great improvement. We should also update the README.

[clemera]

We should also update the README.

Done

[clemera]

I noticed :multiple shouldn't be a keyword of selectrum-read because this feature only works for selectrum-completing-read-multiple which passes the dynamic function for multiple selection tailored to crm.

We could later decide to allow this feature for selectrum-read in general (using selectrums own variables without depending on crm-separator and such) but for now I have removed the keyword and instead set selectrum--crm-p buffer locally in the minibuffer invoked by selectrum-completing-read-multiple (this way this also continues to work correctly with recursive minibuffers).

The code for crm support is now mostly bundled into selectrum-completing-read-multiple which is a bit nicer, too.

[clemera]

As far as I am concerned this can be merged. Let me know if you have further considerations.

[raxod502]

Splendid!