abo-abo / swiper

Ivy - a generic completion frontend for Emacs, Swiper - isearch with an overview, and more. Oh, man!
https://oremacs.com/swiper/
2.31k stars 338 forks source link

Ivy adds an extra space after doing completion in M-x shell and M-x eshell #2852

Closed gnuvince closed 3 years ago

gnuvince commented 3 years ago

Summary

In shell and eshell, an extra space is added to a tab-completion after the first time the ivy dialog has popped.

System information

Steps to reproduce

(package-initialize)
(require 'ivy)
(ivy-mode +1)
(eshell)

In the eshell session, do cd <path that will cause a completion dialog>. After selecting your choice with RET you will notice there's a space at the end of the command-line. Now, whenever you do TAB completion in eshell, even if there's only a single choice, an extra space will be added at the end of the command. It's extremely irritating.

In case the explanations above are not entirely clear, I made an asciinema video that, hopefully, illustrates the problem clearly: https://asciinema.org/a/7UvI9mV0JF7fPifLGkSEPDTfI

basil-conto commented 3 years ago

This is a regression caused by #2837. Paging @meadofpoetry.

basil-conto commented 3 years ago

It is caused by the :exit-function of pcomplete-completions-at-point which inserts pcomplete-termination-string, which is a user option:

pcomplete-termination-string is a variable defined in `pcomplete.el'.

Its value is " "

  You can customize this variable.

A string that is inserted after any completion or expansion.
This is usually a space character, useful when completing lists of
words separated by spaces.  However, if your list uses a different
separator character, or if the completion occurs in a word that is
already terminated by a character, this variable should be locally
modified to be an empty string, or the desired separation string.

So setting this user option to the empty string should work around the problem, and I wonder whether Eshell or Pcomplete should manage this instead?

basil-conto commented 3 years ago

Indeed shell locally modifies pcomplete-termination-string according to another user option comint-completion-addsuffix:

comint-completion-addsuffix is a variable defined in `comint.el'.

Its value is t

  You can customize this variable.
  Probably introduced at or before Emacs version 19.23.

If non-nil, add ` ' to file names.
It can either be a string FILESUFFIX or a cons (DIRSUFFIX . FILESUFFIX)
where DIRSUFFIX is ignored and FILESUFFIX is a string added on unambiguous
or exact completion.
This mirrors the optional behavior of tcsh.
basil-conto commented 3 years ago

However it still counts as a regression because in vanilla Emacs completion--done is called with exact rather than finished.

basil-conto commented 3 years ago

I don't know how to distinguish between exact and finished in Ivy without redesigning it.

Is it safe to work around this by binding pcomplete-termination-string around the call to completion--done? Otherwise I think we'll have to revert to passing exact.

basil-conto commented 3 years ago

Thinking about this some more: isn't it the case that finished only applies when there is a single possible completion?

gnuvince commented 3 years ago

Thanks for the investigation @basil-conto. Thanks to the information you posted here, I can work around the problem with this config of comint:


(use-package comint
  :config
  (setq comint-completion-addsuffix nil))
meadofpoetry commented 3 years ago

I don't know how to distinguish between exact and finished in Ivy without redesigning it.

I think that Ivy needs redesign here, yes. Helm and company are good examples of treating this right, and Ivy feels like it was designed around the idea of just selecting an item out of the list, ignoring the fact that Emacs' completion facility's logic is more complex than that.

basil-conto commented 3 years ago

I think that Ivy needs redesign here, yes.

I was hoping you'd suggest a solution instead ;).

AFAICT commit fc89be74f2ff1ae1145436a2ebc3b9e1e4a8d521 improves the pre- #2837 situation whilst fixing the regresion it introduced in shell and eshell completion.

@gnuvince Please check whether this is indeed the case.

@meadofpoetry Does this change still address the scenario in #2837, or did I effectively revert that PR in a more convoluted way? In the latter case, please submit a new issue or PR with a precise recipe, e.g. involving Eglot which I already use.