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

Disable vertico-insert for tramp #240

Closed
aheaume opened this issue Jun 9, 2022 · 7 comments
Closed

Disable vertico-insert for tramp #240

aheaume opened this issue Jun 9, 2022 · 7 comments

Comments

@aheaume
Copy link

aheaume commented Jun 9, 2022

Hi! 👋🏻

I'm using vertico+orderless, which is an awesome combo unless trying to autocomplete tramp paths. I don't have a full understanding of the interactions between vertico, orderless and tramp, so I might be missing something obvious 😅

Tramp hostname completion isn't sufficient in my case because hitting TAB still picks the "selected"/first candidate which is not helpful when you can't narrow down the list to something manageable.

Instead of waiting for tramp to support more completion styles, I'm looking at the problem the other way: I need a way to tweak vertico-insert to preempt it from doing it's job, but only when dealing with tramp paths.

I've got this naive implementation bound to TAB in vertico-map

  (defun my-vertico-insert-unless-tramp ()
    "Insert current candidate in minibuffer, except for tramp."
    (interactive)
    ;; build a regexp that matches tramp methods (/ssh:\\|/sftp:\\|...)
    (let ((methods (string-join (mapcar (lambda (x) (concat "/" (car x) ":")) tramp-methods) "\\|")))
      ;; if looking at a tramp-ish minibuffer content, bail out and call minibuffer-complete
      (if (string-match methods (vertico--candidate))
	(minibuffer-complete)
      (vertico-insert))))

and it works OK, if hackish. I could not find any other way to disable vertico-insert temporarily, is there a better way to do this? I haven't found knobs to configure the behavior of vertico-insert other than advising it or defining a new function. I'm especially interested in improvements along the lines of

  • having this custom behavior only active when hitting TAB during file completion, not commands (although there aren't many commands named hello-/ssh:-world out there, hopefully)
  • not calling vertico--candidate if it is an internal function that may change
  • maybe having a hook that activates before vertico-insert does it's job?
  • a more robust way to detect "interesting" minibuffer contents instead of blindly running regexp on the input

This might very well be outside of the scope of vertico to support such customizations, if so then anybody reading this feel free to steal this dumb function for your own config

Have a nice day 😄

@minad
Copy link
Owner

minad commented Jun 9, 2022

There is no simpler way than replacing/rebinding the command in the vertico-map, but there is also nothing wrong with that - it is already sufficiently simple. Keymaps are there to be modified, keys are there to be rebound.

Instead of building the regexp, you could use vertico--remote-p to detect remote paths. As a simpler approach you could always bind M-TAB to minibuffer-complete, see https://github.com/minad/vertico/#completion-styles-and-tab-completion and use that in the Tramp context.

Having Tramp-specific insertion functionality in Vertico out of the box is out of scope. In principle vertico-insert should work well also with Tramp but maybe it hurts too much when the server is very slow.

Please feel free to add your command and configuration to the Vertico wiki!

@minad minad closed this as completed Jun 9, 2022
@aheaume
Copy link
Author

aheaume commented Jun 9, 2022

Thanks 🙏🏻 that makes sense!

I'll be using

  (defun my-vertico-insert-unless-tramp ()
    "Insert current candidate in minibuffer, except for tramp."
    (interactive)
    (if (vertico--remote-p (vertico--candidate))
	(minibuffer-complete)
      (vertico-insert)))

for a few days and if it works I'll try to remember to add it to the wiki.

As a simpler approach you could always bind M-TAB to minibuffer-complete, see https://github.com/minad/vertico/#completion-styles-and-tab-completion and use that in the Tramp context.

I don't really want to use M-TAB for this instead of TAB because I'll only ever think of using it after I tried to TAB complete and got annoyed that tramp opened the connection to the wrong machine, but that might work for other people.

Having Tramp-specific insertion functionality in Vertico out of the box is out of scope. In principle vertico-insert should work well also with Tramp but maybe it hurts too much when the server is very slow.

Yes I don't think there is anything wrong with vertico itself, nor that it should have anything tramp-specific. I was just wondering if overriding the binding or advising the function is the way to go, which you confirm. I think that ultimately the fix would be to make tramp support other completion styles, which would make orderless work. I don't have the courage to fix this unfortunately 😅 so I was looking for a way to disable vertico temporarily as a workaround.

@minad
Copy link
Owner

minad commented Jun 9, 2022

Yes I don't think there is anything wrong with vertico itself, nor that it should have anything tramp-specific. I was just wondering if overriding the binding or advising the function is the way to go, which you confirm. I think that ultimately the fix would be to make tramp support other completion styles, which would make orderless work. I don't have the courage to fix this unfortunately sweat_smile so I was looking for a way to disable vertico temporarily as a workaround.

Tramp should support all completion styles. The file completion backend should be agnostic to the completion styles in use. For example when I enter /sudo::/, completion works reasonably well with Orderless. I don't understand your exact problem. Probably it is just that the server is too slow?

For example, /sudo::/ completion works but is still much slower than normal file completion for me. When I collect a profile it shows that a lot of time (or rather all the time) is spent inside the tramp file handlers. Tramp overall seems to be a bit inefficient. I must admit that I am critical of Tramp, since it is in some sense to intrusive - it hooks into the file handling infrastructure and affects many operations badly and makes them unpredictably slow. Tramp basically makes a mine field out of operating with files and file names - potential Tramp issues are lurking everywhere. The situation becomes particularly bad with aggressively updating incremental completion systems like Vertico, Corfu, Ivy, Helm, ... Anyway Tramp is the best we've got 🤷

@aheaume
Copy link
Author

aheaume commented Jun 9, 2022

We are probably talking past each other then. Do note that the function above is good enough for me so feel free to just drop the discussion 😉 I'll try to explain in detail the problem I was having.

I'm trying to have completion for host names, not remote files, so "Probably it is just that the server is too slow?" does not apply here; the annoying behavior happens before I even hit the server, when completing the host names that tramps gathers from my machine. Which brings us to...

Tramp should support all completion styles.

This is not true in all contexts unfortunately (emphasis mine):

TRAMP can complete the following TRAMP file name components: method names, user names, host names, and file names located on remote hosts. User name and host name completion is activated only, if file name completion has one of the styles basic, emacs21, or emacs22.

from https://www.gnu.org/software/emacs/manual/html_node/tramp/File-name-completion.html for tramp 2.5.2.28.1

This is acknowledged in the orderless readme

The basic completion style is specified as fallback in addition to orderless in order to ensure that completion commands which rely on dynamic completion tables, e.g., completion-table-dynamic or completion-table-in-turn, work correctly. Furthermore the basic completion style needs to be tried first (not as a fallback) for TRAMP hostname completion to work. In order to achieve that, we add an entry for the file completion category in the completion-category-overrides variable.

Here's an example, I want to access some-random-machine-2 and I've got these hosts defined in ~/.ssh/config

- machine-0
- some-random-machine-1
- some-random-machine-2
- yet-another-machine

If I call find-file and type /ssh:so I'm offered the list of completions just fine

> /ssh:so
some-random-machine-1
some-random-machine-2

but I can't to narrow it down so that some-random-machine-2 is the first choice so I can TAB complete it. I can't use orderless here to narrow down by typing /ssh:so 2 because the completion is falling back to basic as described by the orderless readme. Instead this happens

> /ssh:so 2
/nc:
/adb:
/ssh:
...

presumably because tramp supports providing several paths to open multiple files in one go. Which is fine! I don't think vertico or orderless need to add tramp-specific support, the problem comes from tramp only supporting basic in that context, and they probably have their reason too.

If I were to just type /ssh:so<TAB>, vertico-insert kicks in and correctly connects to some-random-machine-1, which is correct since it is the first match... except "correct" in this context is not what I want anymore :)

> /ssh:so
some-random-machine-1  # not the machine I'm looking for
some-random-machine-2

so instead what I want was a way to revert, temporarily, to the old TAB-calls-minibuffer-complete behavior.

  (defun my-vertico-insert-unless-tramp ()
    "Insert current candidate in minibuffer, except for tramp."
    (interactive)
    (if (vertico--remote-p (vertico--candidate))
	(minibuffer-complete)
      (vertico-insert)))

does just that. But then I wondered if this was the best way to do it, if I was missing something, etc. Hence the question, but I'm satisfied with the answer to just rebind TAB in vertico-map to a custom function.

Hopefully that makes the context clearer 😅

@minad
Copy link
Owner

minad commented Jun 9, 2022

Okay, I am aware that Orderless does not work for all programmable completion tables, in particular not the completion-table-dynamic ones. These tables require the basic, emacs21 or emacs22 completion styles. This is even documented in the Vertico README:

For completion-table-dynamic it is not even a bug,but by design. For the Tramp host completion table we are talking about here, it is a bug however. This table should be reimplemented to take advantage of completion boundaries. Then it would also work flawlessly with Orderless.

I see your point now that for basic completion you don't like the vertico-insert behavior which inserts the selected candidate. For such tables you rather prefer the normal shell-like prefix completion behavior. Your command my-vertico-insert-unless-tramp makes sense to achieve the desired behavior. I'd appreciate if you document that in the wiki. Or we could even add it to the README in the section https://github.com/minad/vertico/#tramp-hostname-completion.

Unfortunately I don't see how we could integrate this command in Vertico out of the box - usually I prefer to have uniform behavior across all contexts. We would need some means to detect if we are in a basic context and then drop to the old-style tabbing. It is not clear to me how this can be done. We could add the command as is, such that it is just a Tramp special case but this is something I would rather avoid.

@minad
Copy link
Owner

minad commented Jun 9, 2022

I just tried to go down the Orderless style dispatcher rabbit hole to improve the host name completion. Unfortunately it turns out that the hostname completion table ignores completion-regexp-list. It is truly a broken completion table, which does not respect the specification. 🤷

Just for reference, this is what I tried:

(defun +orderless-dispatch (word index _total)
  (cond
   ;; Treat first word as prefix completion for remote files.
   ((and (= index 0)
         (string-match-p ":" word)
         (string-match-p "\\`/[^/|:]+:"
                         (substitute-in-file-name
                          (minibuffer-contents-no-properties))))
    ;; Orderless will interpret this regexp as prefix
    ;; and pass it directly to the table. This special treatment
    ;; of ^literal regexps is an optimization and has also been
    ;; added to support tables which require a prefix.
    `(orderless-regexp . ,(concat "^" (regexp-quote word))))
   ...))

If the table would respect the regexp filtering in addition to the prefix filtering, input like /ssh:so 2 would work. This means that the basic-remote configuration from the README is the best we can do, in combination with your command my-vertico-insert-unless-tramp.

(For more background regarding the style dispatcher, see the Orderless README and https://github.com/minad/consult/wiki#minads-orderless-configuration.)

@aheaume
Copy link
Author

aheaume commented Jun 10, 2022

That makes sense, thanks for looking into this! 🙏

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

2 participants