consult-completing-read-multiple does not handle completion-file-name-table correctly

[Hugo-Heagren]

I recently wanted to write a command which included selecting more than one file name. The standard way to do this is

(completing-read-multiple "Foo: " #'completion-file-name-table)

In vanilla emacs (and with other completion enhancements like vertico or icomplete-vertical) this works exactly as expected: completion is done on filenames, and whenever a full file is completed, it can be somehow delimited (e.g. by typing the crm-separator), and then another one can be completed.

With the recommended advice overriding standard crm with consult's enhanced version, the above code stops working. Removing the advice fixes the problem, so I think that consult-completing-read-multiple is the problem.

I still get a completion prompt with multiple candidates (all the files in the current dir), and can select more than one, but if I type in the name of a directory and press /, the list doesn't update to that directory, as it would in other systems.

Tried searching for other issues but couldn't find any. I found this though, so this is clearly a use-case which other people need.

[minad]

No, as far as I know CRM cannot handle complex completion tables with completion boundaries like file path completion. Are you sure that this works? For this reason consult-crm cannot handle this either.

[Hugo-Heagren]

Certainly works for me. I'm on emacs 29.0.50. I did emacs -Q, and executed the following in a scratch buffer:

(completing-read-multiple "Foo: " #'completion-file-name-table)

Obviously the vanilla crm interface is a bit clunky, so you have to input the separator , manually, but it worked. I completed two file names and got a list back.

Does it not work for you? The last reference I can find to crm in NEWS is in NEWS.24, where the separator became a regexp, so I don't think its likely to be a version issue.

[minad]

Ok, I will take a look. If this is true then I will remove consult-crm entirely from consult since we cannot implement it in the way it is implemented.

[minad]

@Hugo-Heagren Thank you for point this out! You are perfectly right that the default completing-read-multiple implementation handles completion tables with completion boundaries correctly (as do the fully compliant UIs default completion, Vertico and Icomplete). I am not sure how I came to the wrong assumption but it is likely that I was still using Selectrum back then, which doesn't implement completion boundaries. Also the Selectrum CRM implementation has the same problem. These Selectrum issues with dynamic completion tables, completion boundaries, completion style support were part the motivation back then to experiment with a new implementation in the first place, which then turned into Vertico.

Unfortunately there is no way we can salvage the current implementation of consult-completing-read-multiple. The only reasonable way forward is to ultimately drop consult-completing-read-multiple. I value correctness higher than having broken features around, in particular since I advertised this a general drop in for completing-read-multiple, which it is not as you pointed out. We can still try to recreate a new version which will be based on the same idea. The idea is to essentially use a loop of completing-read. But there will be no way to implement the nice check mark prefixes again. Anyway, if such an implementation will enter Consult again at some point it will have to go through another careful review and PR.

[Hugo-Heagren]

Well, I'm very sorry to see it go, I really liked the interface of consult's crm. But I agree, if I can't be properly compliant, it shouldn't be used as if it is.

Could you expand on what the general nature of the problem is? You say in the commit message that "The assumption was that the completion tables passed to completing-read-multiple don't have non-trivial completion boundaries", but I'm not sure what a completion boundary is, or what the difference between trivial and non-trivial versions would be. I ask because if I find the time, I would be glad to have a go at implementing a version of consult crm again?

[minad]

The implementation of consult-crm is unfortunately too simplistic. It takes a list of the candidate strings and modifies them depending on if they are selected or not (check mark). Now if you have completion boundaries, this approach cannot work since completion candidates depend on the part of the string you are completing. For example for files the whole completion result is a full path. The boundaries are determined by the path separator (slashes). The completion fields bounded by the boundaries are the file/directory names which are completed in each step. Does this make sense? You can look into the Emacs manual for programmable completion and the boundaries action of completion tables.

[minad]

I forgot to mention trivial tables - a list of strings or an alist of symbols for example is a trivial table. For those tables completion needs only a single step to produce the final result. For those tables consult-crm worked just fine.

[jsilve24]

I too am very sorry to see this go and hope that this would get added back in the future.

[minad]

@jsilve24 Where did you use it? I am not sure if we find a way to put a version back which works better for the aforementioned tables with boundaries. We are basically backed into a corner by the generality of the completion tables and we have almost no design freedom left. For more restricted (trivial) tables the old approach worked well but it doesn't generalize.

I think the impact of the removal is rather small since completing-read-multiple is not used widely. The main user seems to be citar (also due to its long candidate strings), but it also switched to completing-read a while ago in most cases.

[iyefrat]

I think I've been mostly using consult-crm for requesting reviews in forge, since I usually want to request from multiple people. I don't know how widely used the interface is, but we recently get a Doom issue about it doomemacs/doomemacs#6352, @t-e-r-m can you explain your usecase?

I'm currently working on a vertico module bump, so I'll remove the Doom consult-crm customizations, at least until/if consult-crm gets reimplemented.

[minad]

@iyefrat Thanks for mentioning your use case! I think it is similar to describe-face, where you complete short symbols. In this case the default crm implementation works well. You only have to insert a CRM separator character, e.g., a comma. See also the CRM indicator from the Vertico README: https://github.com/minad/vertico#configuration. It will display [CRM,] in the prompt if comma is the separator. Maybe it is minimally less convenient than before?

The only use case where consult-crm seems to be really needed is Citar, since Citar uses these long candidate strings, such that the interface becomes essentially unusable if you select multiple candidates. I hope that @bdarcus will figure out a better solution soon (essentially calling completing-read in a loop, see emacs-citar/citar#582 and emacs-citar/citar#495). The problem for consult-crm is that when I implemented it initially, I assumed that the completion tables passed to completing-read-multiple are much more limited than they have to be in general. Now consult-crm leads to a serious regression in comparison to the default implementation as was pointed out in this issue. Unfortunately there is no way to salvage the implementation and patch it up to handle the general case. It is an unfixable flaw and the thing has to be reimplemented again from scratch (and I don't know how exactly).

Now given that Citar is the only relevant consumer (if you buy the argument that describe-face etc work well with the default implementation) and given that consult-crm was added mostly because of Citar, then removing consult-crm is the right call. Instead Citar should use its own implementation. If I recall correctly this is something we considered a longer time ago, when @bdarcus initially asked for a better crm interface in Selectrum.

[iyefrat]

@iyefrat Thanks for mentioning your use case! I think it is similar to describe-face, where you complete short symbols. In this case the default crm implementation works well. You only have to insert a CRM separator character, e.g., a comma. See also the CRM indicator from the Vertico README: https://github.com/minad/vertico#configuration. It will display [CRM,] in the prompt if comma is the separator. Maybe it is minimally less convenient than before?

Ah yeah, I've just found that, I'll be adding it to Doom. It is less convenient, since:

1. Selecting a candidate is now [tab] , instead of just [tab] (I changed the keybindings in Doom, see here and here).
2. Deselecting is now harder (for candidates that weren't selected last), since it requires using editing commands on the minibuffer text itself rather than using the vertico interface
3. Having the different candidates on their own lines made them instantly parsable, having them on the same line separated by commas doesn't pop out as much.

These are relatively minor and are not worth keeping around a broken implementation, maybe I'm missing an easier way to do these as well.

There is however the added bonus with the vertico indicator of it being clearer when packages are doing nonstandard stuff with crm, such as magit using it to get completions on ref1..ref2 type inputs. I think at the time you said that this was a bit of an abuse of the api though.

[minad]

It is less convenient, since:

1. You could bind your own command to TAB which also inserts the separator.
2. Definitely true. Removing is less convenient, but for the last selection backward-kill-word should do.
3. Not a big issue for short candidates?

The underlying issue here is that the consult-crm interface is just too different from default completing-read-multiple. As it turns out, it is an interface which is hard to improve upon while retaining its generality. I noted a few times before that I consider crm a problematic API which doesn't fit well into the whole picture (also with respect to Embark). Now I think one should really only use it in narrow niches like describe-face where the downsides you mentioned are acceptable.

There is however the added bonus with the vertico indicator of it being clearer when packages are doing nonstandard stuff with crm, such as magit using it to get completions on ref1..ref2 type inputs. I think at the time you said that this was a bit of an abuse of the api though.

Magit definitely abuses the API by relying too much on the exact internal implementation. But with the removal of consult-crm all such issues should be gone anyway.

[iyefrat]

1. You could bind your own command to TAB which also inserts the separator.

Yeah, but I like to offload my vert&co configuration to Doom when i can help it 😛, and this feels like a too opinionated default.

3. Not a big issue for short candidates?

Yeah Like I said it's all pretty minor, but it's still not as instantly obvious. I wonder how hard it would be to invert the syntax highlighting for the separators for example. I need to look into that.

I do have a thought for the citar long candidate case though, what about folding the previously selected candidate when the cursor isn't on them? @bdarcus does this fit your usecase at all?

[bdarcus]

@iyefrat

what about folding the previously selected candidate when the cursor isn't on them?

By "folding" do you mean like a shortened representation of it?

If yes, I think we'd toyed with that idea. But it retains the more general limitations you note.

edit: I just think CRM is a horrible UI in general, regardless of candidate length.

[precompute]

I liked this feature a lot! Sad to see it go.

Couldn't this just be kept in as an "experimental" function? It wouldn't replace completing-read-multiple by default, but the users who want it could still use it.

[iyefrat]

Just to be clear, it never replaced completing-read-multiple by default, it's just an optional configuration that's on by default in Doom.

[precompute]

@iyefrat Thanks for the clarification!

[minad]

@t-e-r-m

Couldn't this just be kept in as an "experimental" function? It wouldn't replace completing-read-multiple by default, but the users who want it could still use it.

It was an experimental function. And it failed :-P

[jsilve24]

The other two I noticed are embark-export where I get an error

embark-consult--crm-selected: Symbol’s function definition is void: consult--crm-selected

and mu4e-view-save-attachments where I don't get an error but I had enjoyed the consult version of CRM.

[oantolin]

The function embark-consult--crm-selected has already been removed from embark-consult, so after updating you shouldn't get that error anymore, @jsilve24

[jsilve24]

Thank you! Sent From My Mobile Device

…

________________________________ From: Omar Antolín Camarena ***@***.***> Sent: Thursday, May 12, 2022 4:35:48 PM To: minad/consult ***@***.***> Cc: Justin Silverman ***@***.***>; Mention ***@***.***> Subject: Re: [minad/consult] `consult-completing-read-multiple` does not handle `completion-file-name-table` correctly (Issue #567) The function embark-consult--crm-selected has already been removed from embark-consult, so after updating you shouldn't get that error anymore, @jsilve24<https://github.com/jsilve24> — Reply to this email directly, view it on GitHub<#567 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AADOORQFR4D7XOI73Y74B4DVJVTSJANCNFSM5U6J3J5Q>. You are receiving this because you were mentioned.Message ID: ***@***.***>