minad / consult

:mag: consult.el - Consulting completing-read
GNU General Public License v3.0
1.26k stars 106 forks source link

Eshell completion with consult-completion-in-region don't quote filename #810

Closed oaue closed 1 year ago

oaue commented 1 year ago

Hi,

I'm not sure if it's a bug in consult or if it's something missing in my configuration. But when using consult-completion-in-region in eshell, completed filenames aren't quoted.

emacs -Q

(package-initialize)
(require 'consult)
(require 'vertico)
(vertico-mode)

;; substring doesn't work with the completions described below
(setq completion-styles '(basic)) 

(setq completion-in-region-function
      (lambda (&rest args)
        (apply (if vertico-mode
                   #'consult-completion-in-region
                 #'completion--in-region)
               args)))

M-x eshell

$ touch /tmp/file\ with\ spaces
$ ls /tmp/<tab>

Result: image

Expected result:

$ ls /tmp/file\ with\ spaces
minad commented 1 year ago

Thanks for the report. I can reproduce this issue and attempted to fix it in the branch https://github.com/minad/consult/tree/attempted-fix-cir. Unfortunately my attempt to fix this issue failed. I recommend that you use my Corfu package instead, which does not suffer from this problem, since the whole completion process is executed in the shell buffer and quoting works correctly. I am personally not using consult-completion-in-region due to similar problems caused by moving the completion table to the minibuffer. Maybe @oantolin has some insight regarding the quoting?

minad commented 1 year ago

I think I figured it out in https://github.com/minad/consult/commit/3c0f87ebd20b25f03568fb9ef8fd36b5a2a6eb84. I also removed the entire file special casing and just reuse the original completion table as is in all cases. @oantolin Can you please give this a thorough check, since you use this function mainly for in-buffer completion? I hope the refactoring still preserves all the use cases.

oantolin commented 1 year ago

I will test. Without having looked at your changes at all, let me just say that one point of the existing function was to make sure that completion for file names occurred with a category metadatum of file, so packages like Vertico, Marginalia and Embark that have some special handling for files could offer those services.

oantolin commented 1 year ago

So the good news is that with 3c0f87e file names with spaces are indeed quoted correctly, the bad news (for me personally at least) is that I do miss the special handling for files. For example, Marginalia isn't showing the file annotations now. And here's another issue that is kind of hard to explain:

I have a file in ~/Notes/Haskell Monads.org, with a space in the name. Consider having ~/Notes/mo in the buffer and attempting to complete the file name. I'm using orderless with orderless-smart-case = t. Before 3c0f87e, the file I had in mind would match, now it doesn't. Now it seems to want to match the whole path once, so the capital N in ~/Notes/mo makes the matching case sensitive; before, since the usual file name completion table was used (by calling read-file-name), only the last component, namely mo, was being matched (with ~/Notes/ becoming the prefix), so matching was still case insensitive.

Maybe these aren't big deals, but they are changes.

If I recall correctly, we were calling read-file-name because some completion UIs like Selectrum also overrode it and gave a nicer UI for completing file names. This reason might be obsolete if Selectrum was the only one, but I suspect we also had Ivy in mind.

minad commented 1 year ago

The reason for the old approach was that we wanted to support Selectrum which doesn't support the file completion table via completing-read. Selectrum doesn't implement programmable completion tables correctly and neither does Ivy. I don't think there is a point in going back to that approach. The current approach seems technically correct since it reuses the completion table as is with all bells and whistles like quoting.

I think we should fix all the new issues one by one. The file completion category is still preserved, even if the table is a bit quirky.

We should make Marginalia somehow aware of quoting. Note that the default completion UI also works like with quoting. Marginalia probably never worked correctly in e?shell with default completion? The same applies to Embark. It probably also needs to understand quoting.

I am not sure about your matching problem. The path components should still be matched step wise (completion boundaries), not affecting Orderless smart case. This needs more investigation. I suspect that the problem is due to the completion style being executed in the minibuffer while the table itself is wrapped to be executed in the original buffer (necessary for the quoting and tables like LSP mode). If the completion style configuration differs between buffers it may lead to odd effects.

oantolin commented 1 year ago

The reason for the old approach was [...]

Oh, right: not only, as I said, that Selectrum gave better results with read-file-name, but that it actually didn't work with this completion table. Yes, that was the reason. I agree that your new approach is the correct one and that we should fix the resulting issues.

The file completion category is still preserved, even if the table is a bit quirky.

I had not understood this. So the category is file, but the candidates are quoted or something? I'll look into it, I don't actually remember (if I ever knew) what the completion table looks like.

I am not sure about your matching problem.

I'll try to pinpoint the issue more precisely.

If the completion style configuration differs between buffers it may lead to odd effects.

I don't locally change the styles in different buffers, so it's probably not that in my case, unless maybe something is changing my completion styles behind my back (fido-mode does that and possibly it's not the only part of Emacs that does that).

minad commented 1 year ago

I had not understood this. So the category is file, but the candidates are quoted or something? I'll look into it, I don't actually remember (if I ever knew) what the completion table looks like.

completion-table-with-quoting is used to implement this feature. The design is awkward. I don't really understand the reasoning behind the design. The completion table returns quoted candidates, but hopefully there is a way to unquote the candidates for Marginalia. Actually it seems like a bug that the quoted completion table doesn't transform the candidates before passing them to the annotation or affixation function. The wrapping is incomplete and the consequence is that the annotation function sees a candidate it should never see.

oaue commented 1 year ago

Thank you!

I can live without marginalia in eshell completion, but strangely it seems to work only for certain files/dirs: image

(no spaces or special characters in my / directory except lost+found)

minad commented 1 year ago

I added a fix for Marginalia and Embark by setting minibuffer-completing-file-name. This works at least in the common case of filenames without quoting.