Add support for path completion #2608

Philipp-M · 2022-05-29T22:05:45Z

This PR adds support for path completion.

It supports the following:

Autocompletion is triggered with /.
Documentation preview (file permissions, canonicalized path).
shell expansion (e.g. ~/path, $HOME/path, ${HOME}/path)
Async (via spawn_blocking instead of tokios file accessor functions, as they IMHO make the code less readable and are quite a bit slower than just spawning a "thread")
Configurable with editor.path-completion (default true), per-language overrideable path-completion support
Refactors goto_file with a new more robust path-detection/resolution mechanism and also adds shell-expansion.

A baby-step towards #1015

kirawi · 2022-05-30T02:18:51Z

I think it would be better to split off the changes to the LSP into a separate PR. Unless completion is dependent upon them?

Philipp-M · 2022-05-30T07:04:30Z

You mean all the commits that are also in #2507?

In case, I will rebase this as soon as the relevant commits this PR is dependent on are on master, currently only the last commit is relevant for this PR or different to #2507.

nrdxp · 2022-06-29T19:41:40Z

just fyi, tried to rebase this on master for my own use and even though there was only a minor merge conflict in the languages.toml, it doesn't actually build after the changes made in #2738

If trying to use your new macro where the old language_server! was called I get a type error:

error[E0277]: `(dyn for<'r, 's, 't0> FnOnce(&'r mut compositor::Compositor, &'s mut compositor::Context<'t0>) + 'static)` cannot be sent between threads safely
   --> helix-term/src/commands/lsp.rs:848:8
    |
848 |     cx.callback(
    |        ^^^^^^^^ `(dyn for<'r, 's, 't0> FnOnce(&'r mut compositor::Compositor, &'s mut compositor::Context<'t0>) + 'static)` cannot be sent between threads safely

Philipp-M · 2022-06-29T20:26:19Z

Rebased to current master and fixed the issue (the acquire for the language-server inside the closure was unnecessary as only the offset-encoding is required there)

nrdxp · 2022-07-09T20:18:36Z

What is left to move this out of draft status? I've been using this for over a week now and it seems to work fine.

Philipp-M · 2022-07-09T20:54:11Z

Well for one (biggest blocker): This is currently dependent on a few commits (particularly the extension/merging of the completion menu) of #2507, and I'm unsure how to progress as I'm awaiting some feedback there.

Also support for windows paths should be added, this should be simple (by extending the path regex), but I would like to first resolve the first issue.

Another (smaller, maybe not worth to fix) issue is, that currently only one path per line is possible, this could probably be tackled with a different path-regex. But I've read that every character but \0 is allowed in a unix filepath, so I think it's difficult to find a good regex that doesn't have (realworld) limitations.

I've also experimented with async io (via tokio::fs), but IMHO the performance-drop and probably race-conditions (in the completion menu, if writing to fast/slow IO) wasn't worth it to continue that path (yet).

cole-h · 2022-11-17T02:48:26Z

helix-term/src/commands.rs

+                let items = ui::PATH_REGEX
+                    .find(&line_until_cursor)
+                    .and_then(|matched_path| {
+                        let matched_path = matched_path.as_str();


Would it be possible to also add a denylist setting? I run NixOS, so attempting to index /nix/store would be extremely slow with its (currently) ~265000 files / directories.

Maybe we can just use the .(git)ignore for that?

Actually I've used nix/store for stress testing/general feeling, and I thought it was acceptable given the amount of files (but I've got a fast CPU and NVME drive for /nix ...))

Would someone actually edit something directly in /nix/store or in a directory with that much files/folders?

I'm not sure of a daily usecase currently where this would really be a problem, but I'm open for different solutions (my suggestion would be a global .ignore)

I don't edit things in the Nix store, no, but I do use Helix to read files in there very frequently. I'd rather not use a .ignore for that because so many other tools read that, and I'd like to set this only in Helix.

Right now it's less of a blocker (literally :)) for typing, as it's now async and results will be discarded when not relevant anymore (i.e. the user has typed something that doesn't match the completion item).

On a fast computer it's still acceptable IMHO, especially if the folder was indexed once (i.e. is likely cached in memory when accessing again)).

If this is still relevant, can you/someone maybe think/prototype how the UX/configuration for this look like (I'm happy to implement it, but I'm not sure how the UI for this should look like).

But the (completion-)menu is quite slow with > ~100k entries that are sorted synchronously every keystroke.
I think a more sophisticated lazier/async/"streamable" menu/picker implementation would be probably help in general (I still think that the behavior is so similar (also in the future), that the implementations should be shared, but that's a different topic...).

nrdxp · 2023-01-26T20:04:35Z

Did something change? After rebasing on the last push path completion seems to have just stopped working all together. Do I need to configure something differently?

Philipp-M · 2023-01-26T20:36:42Z

Have you tried using the HEAD of this branch?

My personal fork (rebased on this branch) is still working.

It's a little bit out of sync with master, I will try to find time soon to rebase this and the multiple-language-servers PR onto master.

nrdxp · 2023-01-27T03:42:06Z

Oops, I figured it out. It was my own fault, I forgot to push my latest changes of my forked branch (based on this branch) to the remote so my system never pulled my rebase. Apologies 🙏

Philipp-M · 2024-10-03T18:58:55Z

Thanks for the review, I think I handled all the feedback.

helix-term/src/handlers/completion.rs

book/src/editor.md

helix-term/src/handlers/completion.rs

pascalkuthe · 2024-10-03T21:12:19Z

helix-term/src/handlers/completion.rs

+            .filter_map(Result::ok)
+            .filter_map(|dir_entry| dir_entry.metadata().ok().map(|md| (dir_entry.path(), md)))
+            .map(|(path, md)| {
+                let file_name = path


this is a bit ugly, what dir_entry.path() does is just dir_path.join(dir_entry.file_name()) so going trough all this to extractd the file name again doesn't seem ideal. Instead just extract the file name and use that here (and just do dir_path.join(file_name) below)

Should be handled in the recent push.

helix-view/src/document.rs

pascalkuthe · 2024-10-03T21:46:16Z

helix-term/src/ui/completion.rs

+    Path(PathCompletionItem),
+}
+
+impl PartialEq<CompletionItem> for LspCompletionItem {


I don't think we necessarily need these

See #2608 (comment)

Hmm I was hoping to avoid the operator overload by just doing CompletionItem::Lsp(..) but I guess that doesn't work because we use a reference there... Feels a bit overkill but I guess it's fine if moved into a separate file.

pascalkuthe · 2024-10-03T21:47:01Z

helix-term/src/ui/completion.rs

+#[derive(Debug, PartialEq, Clone)]
+pub enum CompletionItem {
+    Lsp(LspCompletionItem),
+    Path(PathCompletionItem),


I don't think we need a seperate PathCompletionItem type. Instead we can just have a kind: Cow<str> on helix_core::CompletionItem. At that point we could also just call that variant Other (so it's clear there will only ever be an lsp variant and one for everything else going trough the helix-core type).

I am also not really sure about this abstraction in general. I think first starters this should be much better encapsulated. This file is certainly the wrong place for it (I don't want the handler/resolve code to depend on the UI at all). So probably at least a submodule in handlers::completion. I would also like to have as much information extraction as possible within methods (like preselect). It's ok to keep `lsp_item_to_transaction in the UI code for now since that is a larger change (altough it also doesn't belong there)

I don't think we need a seperate PathCompletionItem type. Instead we can just have a kind: Cow<str> on helix_core::CompletionItem. At that point we could also just call that variant Other (so it's clear there will only ever be an lsp variant and one for everything else going trough the helix-core type).

I just fear that there may be more information relevant for a completion item, but that fear may well be unjustified.

This file is certainly the wrong place for it

See recent discussion for it, I'm aware of it, but I don't want to start an even bigger refactor, when PRs are open for so long without any kind of review to avoid resolving merge-conflicts. It shouldn't be too hard to move this somewhere else, when that kind of refactor is done.

I just fear that there may be more information relevant for a completion item, but that fear may well be unjustified.

To me if there is more relevant information is should be added to the generic helix_core::CompletionItem. I don't want to couple the UI code to the completion sources. Basically helix_core::CompletionItem shouldn't be modeled after whatever data completion sources can or can't provide but instead by the data that the UI can actually support (and I would want everything to go trough that generic interface except LSP). The only reason that lsp needs to be separate is because creating the transaction is complicated and needs to be done lazily.

See #2608 (comment) for it, I'm aware of it, but I don't want to start an even bigger refactor, when PRs are open for so long without any kind of review to avoid resolving merge-conflicts. It shouldn't be too hard to move this somewhere else, when that kind of refactor is done.

I agree that we shouldn't do a large refactor in this PR but moving this into another file inside helix term (like in the handler) is a pretty easy cosmetic change and reduces spaghetti code by a lot. I don't think that would even conflict with the snippet system PR

I think this should be handled.

pascalkuthe · 2024-10-06T19:58:19Z

pushed to this branch by accident, sorry about that I reverted the change (not clear in github UI)

Adds support for path completion for unix paths * Autocompletion is triggered with `/`. * Documentation preview (file type, file permissions, canonicalized full path). * Home-path resolution (`~/path`, `$HOME/path`, `${HOME}/path`) * Link resolution (makes sense for preview, since the LSP specification (`CompletionItemKind`) only supports files and folders but not symlinks) * Async (via `spawn_blocking` instead of tokios file accessor functions, as they IMHO make the code less readable and are quite a bit slower than just spawning a "thread") * Configurable with `editor.path-completion` (default `true`), per-language overrideable path-completion support Handle windows CI Micro-optimization and revert of composing additional text edits Use `PartialEq` for `LspCompletionItem` and don't prefix-filter path completion items Apply suggestions from code review Co-authored-by: Michael Davis <[email protected]> Apply suggestions from code review Fix cargo fmt Fix byte offsets in `matched_path`

Philipp-M · 2024-10-14T16:24:43Z

AFAICS path detection + shell-expansion should now be quite a bit more robust with @pascalkuthe s changes, and should also support windows.
I have no (easy) way currently test this though, so real-world testing would be appreciated.

pascalkuthe

This is starting to look pretty good, thanks for addressing my comments! I found a couple smaller things but hopefully should be ready soon

pascalkuthe · 2024-10-19T07:30:10Z

helix-term/src/handlers/completion.rs

@@ -196,6 +202,7 @@ fn request_completion(
    // necessary from our side too.
    trigger.pos = cursor;
    let trigger_text = text.slice(..cursor);
+    let cancel_completion = Arc::new(AtomicBool::new(false));


I don't want to build custom synchronization code here around atomic. What you did is reasonable but I want it to be part of a single cancelation token implementation. I thought our cancelation token was already clonable when I made that comment but seems I misremembered.

I already have a specific implementation in mind (a simplified variant if the tokio-utils cancelation token). I am currently on vacation but I will send a PR for that to your branch once I get back (unless you want to /get around to implementing that first)

I can wait for your PR, thanks.

I put up a PR, ended up a bit more complex than I thaught but I am very happy with it. After looking at it I also did a lot of "quick hacks" like this that weren't really wrong but are better centralized in dedicated shared code

Yeah, probably better to have this a little bit more encapsulated.

helix-term/src/handlers/completion.rs

helix-term/src/handlers/completion/path.rs

Philipp-M force-pushed the path-completion branch from e325640 to d4dfb7d Compare June 4, 2022 11:15

Philipp-M force-pushed the path-completion branch from d4dfb7d to bc62cb1 Compare June 21, 2022 20:19

the-mikedavis linked an issue Jun 25, 2022 that may be closed by this pull request

autocompleting file paths #2874

Open

Philipp-M force-pushed the path-completion branch from bc62cb1 to 0865642 Compare June 29, 2022 20:23

Philipp-M force-pushed the path-completion branch from 0865642 to f5ae055 Compare July 7, 2022 11:21

Philipp-M force-pushed the path-completion branch from f5ae055 to 344f3d4 Compare July 27, 2022 10:55

Philipp-M force-pushed the path-completion branch from 344f3d4 to 24dff03 Compare August 11, 2022 19:36

Philipp-M force-pushed the path-completion branch from 24dff03 to 354ac33 Compare September 7, 2022 12:42

kirawi added A-helix-term Area: Helix term improvements S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Sep 13, 2022

Philipp-M force-pushed the path-completion branch 2 times, most recently from e224b0d to 1dc2381 Compare September 26, 2022 21:32

Philipp-M force-pushed the path-completion branch from 1dc2381 to 3721d63 Compare October 16, 2022 15:19

Philipp-M force-pushed the path-completion branch from 3721d63 to 9193aa4 Compare October 27, 2022 23:11

Philipp-M force-pushed the path-completion branch from 9193aa4 to 0af6cfe Compare November 5, 2022 23:48

cole-h reviewed Nov 17, 2022

View reviewed changes

Philipp-M force-pushed the path-completion branch 3 times, most recently from 7435f08 to b594f49 Compare November 25, 2022 00:16

Philipp-M force-pushed the path-completion branch from b594f49 to 12c0889 Compare December 8, 2022 22:45

Philipp-M mentioned this pull request Feb 14, 2023

Refactor picker/menu logic into an options manager which can be fed with multiple different item sources (async, sync, refetch) #5991

Closed

the-mikedavis reviewed Oct 3, 2024

View reviewed changes

helix-term/src/handlers/completion.rs Outdated Show resolved Hide resolved

Fix byte offsets in matched_path

59b2cf5

the-mikedavis previously approved these changes Oct 3, 2024

View reviewed changes

the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2024

Philipp-M commented Oct 3, 2024

View reviewed changes

book/src/editor.md Show resolved Hide resolved

pascalkuthe requested changes Oct 3, 2024

View reviewed changes

the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Oct 3, 2024

pascalkuthe dismissed the-mikedavis’s stale review via 9aeedc2 October 6, 2024 19:38

pascalkuthe force-pushed the path-completion branch from 9aeedc2 to 59b2cf5 Compare October 6, 2024 19:47

pascalkuthe and others added 7 commits October 14, 2024 17:45

implement robust path detection and env expansion

b08e6b6

use path::expand in path completions

a3e12ac

use new path detection and expansions in goto_file

1334606

Fix some typos.

101bcc7

Apply review suggestions, and other smaller fixes.

8af7c67

Modularize path completion into separate functions

57e34aa

cargo fmt

d4a873d

Philipp-M changed the title ~~Add support for path completion for unix paths~~ Add support for path completion Oct 14, 2024

cargo clippy fix

ce885df

the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 14, 2024

pascalkuthe reviewed Oct 19, 2024

View reviewed changes

Philipp-M and others added 3 commits October 20, 2024 00:59

Address review feedback

3817321

improve completion cancelation

d73f158

Fix typos, and other cosmetic stuff

3dbcc4e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for path completion #2608

Add support for path completion #2608

Philipp-M commented May 29, 2022 •

edited

Loading

kirawi commented May 30, 2022

Philipp-M commented May 30, 2022

nrdxp commented Jun 29, 2022

Philipp-M commented Jun 29, 2022

nrdxp commented Jul 9, 2022

Philipp-M commented Jul 9, 2022

cole-h Nov 17, 2022 •

edited

Loading

Philipp-M Nov 25, 2022

cole-h Nov 25, 2022

Philipp-M Jun 12, 2023

nrdxp commented Jan 26, 2023

Philipp-M commented Jan 26, 2023

nrdxp commented Jan 27, 2023

Philipp-M commented Oct 3, 2024

pascalkuthe Oct 3, 2024

Philipp-M Oct 14, 2024

pascalkuthe Oct 3, 2024

Philipp-M Oct 4, 2024

pascalkuthe Oct 4, 2024

pascalkuthe Oct 3, 2024

Philipp-M Oct 4, 2024 •

edited

Loading

pascalkuthe Oct 4, 2024

pascalkuthe Oct 4, 2024

Philipp-M Oct 14, 2024

pascalkuthe commented Oct 6, 2024

Philipp-M commented Oct 14, 2024

pascalkuthe left a comment

pascalkuthe Oct 19, 2024

Philipp-M Oct 19, 2024 •

edited

Loading

pascalkuthe Oct 21, 2024

Philipp-M Oct 22, 2024

Add support for path completion #2608

Are you sure you want to change the base?

Add support for path completion #2608

Conversation

Philipp-M commented May 29, 2022 • edited Loading

kirawi commented May 30, 2022

Philipp-M commented May 30, 2022

nrdxp commented Jun 29, 2022

Philipp-M commented Jun 29, 2022

nrdxp commented Jul 9, 2022

Philipp-M commented Jul 9, 2022

cole-h Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrdxp commented Jan 26, 2023

Philipp-M commented Jan 26, 2023

nrdxp commented Jan 27, 2023

Philipp-M commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philipp-M Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pascalkuthe commented Oct 6, 2024

Philipp-M commented Oct 14, 2024

pascalkuthe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philipp-M Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philipp-M commented May 29, 2022 •

edited

Loading

cole-h Nov 17, 2022 •

edited

Loading

Philipp-M Oct 4, 2024 •

edited

Loading

Philipp-M Oct 19, 2024 •

edited

Loading