Skip to content
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

sly-completing-read and Vertico mode #473

Closed
andreyorst opened this issue Nov 12, 2021 · 9 comments
Closed

sly-completing-read and Vertico mode #473

andreyorst opened this issue Nov 12, 2021 · 9 comments

Comments

@andreyorst
Copy link

Hi. This is more a question, rather than an issue, although maybe it should be fixed in Sly, or maybe elsewhere. I'm using the Vertico package, which provides vertical completion UI for everything in Emacs. I've noticed, however, that Sly uses IDO mode when I press , in the REPL buffer. Inspecting the code I see that the sly-completing-read function checks exactly for two conditions - whether completing-read-function equals to 'completing-read-default, and that icomplete-mode is disabled. If both are true, ido-completing-read is used:

sly/lib/sly-messages.el

Lines 95 to 101 in 540a8c5

(let ((completing-read-function
(if (and (eq completing-read-function 'completing-read-default)
(not icomplete-mode))
#'ido-completing-read
completing-read-function)))
(completing-read prompt choices predicate require-match initial-input hist def
inherit-input-method)))

This prevents Vertico from working here, as it doesn't change the completing-read-function, and there's no check for vertico-mode either.

Now, I think it would be not the best approach to include every single completion UI mode into this function, so I'm not proposing adding a check for vertico-mode here. However, I'd like to use Vertico for this menu, for consistency's sake. I can use the advice system to make it work, but it seems kinda hacky for this particular case:

(define-advice sly-completing-read (:around (fn &rest args))
  (let ((icomplete-mode t))
    (apply fn args)))

Any suggestions or thoughts? Feel free to close it if you think that Sly does everything correctly here, and this is a user configuration/Vertico problem.

andreyorst added a commit to andreyorst/dotfiles that referenced this issue Nov 12, 2021
@joaotavora
Copy link
Owner

Yes you are right.

I think as time progresses, more people will become aware of things better than Ido and then we can probably kill that block completely. You can probably just set sly-completing-read to completing-read...

btw if you like vertico-mode (it's nice!) , then give fido-vertical-mode (in emacs 28) a try and tell me what you think.

@andreyorst
Copy link
Author

btw if you like vertico-mode (it's nice!) , then give fido-vertical-mode (in emacs 28) a try and tell me what you think.

I've tried fido-vertical-mode, and its performance is not as good as I'd like it to be. The completion candidate list appears after a slight delay, which also can be noticed after each keypress. This actually happens in fido-mode without the vertical UI, so I guess, it's an unrelated problem, probably caused by matching in any place in the substring. E.g. typing complete in M-x yields:

image

Whereas Verctico only matches at the start of the string:

image

And only shows the rest if I move the cursor to the start of the prompt:

image

I'm not sure if fido-mode does its own matching, but Vertico uses completion styles, and some of the above trickery. There's also icomplete-mode, and the icomplete-vertical-mode is a bit more performant than fido-vertical-mode, but I don't understand icomplete completion semantics.

I think as time progresses, more people will become aware of things better than Ido and then we can probably kill that block completely. You can probably just set sly-completing-read to completing-read...

Yes, seems that doing (fset 'sly-completing-read 'completing-read) works fine

@joaotavora
Copy link
Owner

fido-vertical-modeis doing always the flex completion style, which is a bit slower. vertico-mode seems to use prefix in the beginning, for speed, then change to flex if you move the cursor elsewhere. Not sure I'd like that tbh. But that's probably what causes the speed difference.

@andreyorst
Copy link
Author

I don't have flex in my completion styles: (partial-completion basic), and Vertico only uses completion styles for filtering, so I guess it's using partial completion in the screenshot above.

@joaotavora
Copy link
Owner

joaotavora commented Nov 12, 2021

fido-vertical-mode is overriding your completion styles :-) It's a flex-lover because it's "ido" emulation, and ido used to use flex. You can re-override them if you want.

@andreyorst
Copy link
Author

andreyorst commented Nov 12, 2021

fido-vertical-mode is overriding your completion styles :-) It's a flex-lover because it's "ido" emulation, and ido used flex. You can re-override them if you want.

Ah, I only meant that Vertico doesn't use flex when I move the cursor, as changing completion styles affects the resulting completion list when I move the cursor. For example, here's the behavior with completion-styles set to (emacs21):

image

No flex matching or anything special happening here.


Anyway, this is off-topic of the original problem. I think the issue is fixed for me with (fset 'sly-completing-read 'completing-read), but you've said that you may remove this function in the future, so I'm not sure if I should close this issue, or you'd want to leave it open until the change.

@monnier
Copy link
Collaborator

monnier commented Nov 12, 2021 via email

andreyorst added a commit to andreyorst/dotfiles that referenced this issue Nov 12, 2021
@joaotavora
Copy link
Owner

Or maybe check for ido-mode (i.e. check for positive signs that the
user wants IDO, rather than positive signs that the user wants
something else).

That code was done when people (for people that) weren't even aware of IDO.

This showed that Emacs had IDE-like, visually responsive, capabilities to
lots of old-timer SLIME users/switchers for example.

THe code is obsolete now, it should probably just be deleted.

theothornhill added a commit to theothornhill/sly that referenced this issue Nov 16, 2021
Sly wraps `completing-read' to force ido to provide better UX.  This is
interfering with completion frameworks that don't set the
`completing-read-function' such as `vertico' and `mct'.  Considering that these
packages serve to provide better UX, this functionality shouldn't really be
necessary.

See joaotavora#473

* lib/sly-messages.el (sly-completing-read): Remove wrapper funcition that only
serves to force ido-completion

* contrib/sly-mrepl.el (sly-mrepl-shortcut): Use vanilla completing-read
* sly.el (sly-read-package-name): Use vanilla completing-read
(sly--read-interactive-args): Use vanilla completing-read
(sly-prompt-for-connection): Use vanilla completing-read
(sly-switch-to-most-recent): Use vanilla completing-read
(sly-info): Use vanilla completing-read
(sly-db-invoke-restart-by-name): Use vanilla completing-read
(sly-read-connection): Use vanilla completing-read
(sly-read-inspector-name): Use vanilla completing-read
(sly-contrib--read-contrib-name): Use vanilla completing-read
andreyorst added a commit to andreyorst/dotfiles that referenced this issue Nov 21, 2021
@andreyorst
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants