Add support for path completion for unix paths #2608

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

This PR adds support for path completion, currently only unix paths supported.

It supports the following:

Autocompletion is triggered with /.
Documentation preview (file permissions, canonicalized 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

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 🙏

ddogfoodd · 2024-04-16T16:13:14Z

@Philipp-M thanks for the work. I built https://github.com/Philipp-M/helix/tree/path-completion/ on MacOS 14.4.1 and path completion worked for me as expected.

Let me know if you want specific things tested.

nrdxp · 2024-07-15T21:23:09Z

I don't think we have proper visibility on this PR, and poor Phillip has been force pushing updates for a few years now. I've been using this patch from the close to the beginning and haven't had any issues with it. Sorry if the ping is inappropriate but @archseer or @the-mikedavis do you guys have an idea of what is missing or why this hasn't been merged in so long?

Or has it just been overlooked?

lukeflo · 2024-08-10T21:53:10Z

Thanks for the great work. I just forked the main Helix repo and rebased your PR as well as another PR (#11164) on my forks master. Just built it from there and works great. Was one of the major things which hindered me switching completely to Helix.

Hope it will find its way into the main repo soon.

One thing I encountered is that expanding Tilde (~) for home dir works fine, but the environment variable $HOME isn't expanded (means recognized as e.g. /home/user) at all. Is there a reason for that or am I missing something concerning the concrete use-case?

eliliam

Looks great to me, who are the main people we need to review this to get this finally merged in?

helix-term/src/handlers/completion.rs

book/src/generated/typable-cmd.md

helix-term/src/handlers/completion.rs

the-mikedavis · 2024-08-16T18:39:18Z

helix-term/src/handlers/completion.rs

+                let text_edit = Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
+                    range: range_to_lsp_range(
+                        &text,
+                        Range::new(cursor - edit_diff, cursor),
+                        OffsetEncoding::default(),
+                    ),
+                    new_text: file_name.clone(),
+                }));


I'd like to try to find a way to avoid digging ourselves into lsp-types here. It makes for a pretty small+focused change but I think we should be introducing core types like helix_core::Diagnostic where possible or at least wrappers/replacements like #11486. I suspect it's a potentially large refactor but I think this is the right feature to introduce it with since the actual searching/completion part of this is quite small. (Compared to completing words from the buffer for example)

I haven't looked into this much but it might be pretty simple to update the CompletionItem struct to have an enum between the lsp::CompletionItem and a helix_core::CompletionItem struct that has a changeset or transaction instead of text_edits

Actually a previous iteration of this PR (e.g. this) had an enum CompletionItem with separating variants LSP and Path .
I wasn't sure having a separate mechanism additional to lsp::CompletionItem would make sense, since there was already the variants lsp::CompletionItemKind::{FILE, FOLDER}.

But I guess when thinking about more completion sources it may make sense, especially when the move to more core types resembling those in lsp is planned anyways.

So when I'm getting you right, you mean a CompletionItem at a lower-level than in ui::completion (which I think was the previous iteration). I wonder if it makes sense to generally abstract over lsp::CompletionItem and wrap it in helix_core::CompletionItem. I should probably investigate this, as I think you're right in that this would need a larger refactor (maybe worth it).

I think a Transaction would make more sense here, being more general (and we have impl From<ChangeSet> for Transaction already)?

Ok, I don't think abstracting over lsp::CompletionItem makes sense with the current architecture. So rather having a "data" container helix_core::CompletionItem similar to helix_core::Diagnostic a little bit independent of lsp::CompletionItem with data like documentation (possibly lazy) and Transaction etc. makes probably more sense for now.

Is it planned btw. to move more logic of this kind from helix-term to e.g. helix-view or even helix-core?
I could imagine helix_term::ui::CompletionItem would make more sense in helix-view?

Yeah generally the more we can move from -term to -view the merrier. Ideally most of what lives in -term right now would be in -view (even commands) but it will take quite a bit of work to decouple the UI elements. So -term would just be the terminal backend and then there could be a -gui crate that would also implement the UI components.

Ok, good to know, I'm actually thinking about starting that refactor in this PR, that would obviously blow up this PR a little bit, and would likely produce merge-conflicts with other PRs (e.g. #9996), which is why I'm not sure about it, as I'd like to avoid resolving merge-conflicts and/or maintain this PR for a longer time (in a state where this is more interconnected with the rest of the editor-logic).

Like putting a lot of logic of helix_term::ui::completion into e.g. helix-view::completion as I don't see many dependencies to helix-term currently. (Same for the handlers I guess).

But maybe keep it more incremental, and leave the CompletionItem (and the path/lsp CompletionItem resolution logic) currently in helix-term, i.e. only helix_term::ui::completion::CompletionItem to an enum roughly like this:

pub enum PathKind { Folder, File, Link } // maybe not worth it... pub enum CompletionItem { Lsp { item: lsp::CompletionItem, id: LanguageServerId, resolved: bool, }, Path { // Not sure if this should be a more generic `Core`, // but this probably allows being more flexible in the future kind: PathKind, item: helix_core::CompletionItem, }, } pub struct helix_core::CompletionItem { pub transaction: Transaction, pub label: String, /// Containing Markdown, this could be turned e.g. into an enum to be more structured pub documentation: String, }

Hmm yeah, working incrementally is probably the right way to go about this. Moving more things to helix-view would mainly help the GUI effort which I don't believe anyone is working on actively anyways. I'm also worried about conflicts with #9801 (though on second look it doesn't look like it would conflict much). An enum like that looks good even if it's only a stepping stone to a bigger refactor later.

Actually I have merged the snipped system with path-completion in my personal branch, I don't remember that this caused any serious merge-conflicts...

Ok, good to know, then I'll go the incremental route (when I find the time).

helix-term/src/handlers/completion.rs

the-mikedavis · 2024-08-16T18:57:59Z

helix-term/src/handlers/completion.rs

+                                #[allow(clippy::unnecessary_cast)]
+                                acc.push(if mode & (*p as u32) > 0 { *s } else { '-' });


Suggested change

#[allow(clippy::unnecessary_cast)]

acc.push(if mode & (*p as u32) > 0 { *s } else { '-' });

acc.push(if mode & p > 0 { *s } else { '-' });

it looks like the compiler derefs this automatically now

Actually I guess you could use into_iter instead of iter here and also drop the deref for s

Yeah into_iter is possible, also added String::with_capacity(9) to avoid unnecessary allocs...

Btw. the cast is necessary for e.g. OSX (added a comment as well) as there is p: u16.

Philipp-M · 2024-08-24T19:19:01Z

Ok, did a bit of refactoring to the CompletionItem::{Lsp, Path} enum. I tried to keep the diff small, but it obviously grew a little bit, since the additional variant needs a little bit different handling.

I've added the basically unchanged

struct helix_core::CompletionItem {
    pub transaction: Transaction,
    pub label: String,
    /// Containing Markdown
    pub documentation: String,
}

I think that could be extended over time when necessary.

One thing I encountered is that expanding Tilde (~) for home dir works fine, but the environment variable $HOME isn't expanded (means recognized as e.g. /home/user) at all. Is there a reason for that or am I missing something concerning the concrete use-case?

I've also added support for this along the way.

* 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

Philipp-M · 2024-08-24T19:31:50Z

Hmm nightly rustc seems to be able to handle a few more borrow-checking cases it seems, something like:

let transaction = match item {
    Arm1 { item, .. } => &item_to_transaction(item),
    Arm2 { item, .. } => &item.transaction
};

doesn't seem to be possible without nightly...

Philipp-M · 2024-08-24T19:37:41Z

Btw. I have also refactored the handling of additional text edits of lsp completion, composing it now as a single Transaction (as it was a simple change, and there was a TODO comment anyway for this).

Philipp-M · 2024-08-29T20:16:12Z

Btw. I have also refactored the handling of additional text edits of lsp completion, composing it now as a single Transaction

Well I just reverted that change again... I thought compose handles different resulting ChangeSet::lens, which is not the case. So that would be more complicated, and should probably be handled in a different PR.

the-mikedavis · 2024-09-01T15:08:14Z

Interesting that nightly allows borrowing that value. An alternative could be wrapping it in a Cow::Owned and the path variant uses Cow::Borrowed but passing it to Document::apply_temporary in both branches is good too.

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

helix-term/src/handlers/completion.rs

…completion items

kik4444 · 2024-09-18T17:53:16Z

Not sure if anyone's mentioned this before, but have you considered taking inspiration or logic from the nvim cmp-path plugin? As far as I've noticed it doesn't attempt any complexities like Home-path resolution except the tilde ~.

Philipp-M · 2024-09-18T18:40:27Z

I have not, but since it's implemented already (and it's not really complex IMO), I guess we can keep support for resolution of $HOME and ${HOME}?

kik4444 · 2024-09-18T19:33:33Z

Well if it works it works. Either way I'd just be grateful to have this as part of the main editor. This feature is on the list of things I'd like to have before giving Helix a more serious attempt.

nrdxp · 2024-09-18T19:36:39Z

Well if it works it works. Either way I'd just be grateful to have this as part of the main editor. This feature is on the list of things I'd like to have before giving Helix a more serious attempt.

fwiw I have been using Helix for years now and just carrying this along as a patch pretty much that whole time. It works and I've never had an issue with it. Hope it get's merged soon

RGBCube · 2024-09-25T05:24:34Z

Seems like all reviews are passing and the code works, maybe it's time to finally merge this ~2 year old MR?

jerabaul29 · 2024-09-29T15:58:46Z

Thanks for this excellent PR - I can confirm that it works very well, love it and use it a lot :) .

David-Else · 2024-09-29T17:41:09Z

I am using it and have had no problems, could not live without it now.

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

estin mentioned this pull request Mar 11, 2024

Add paths autocompletion estin/simple-completion-language-server#38

Closed

Philipp-M force-pushed the path-completion branch 2 times, most recently from 749ec12 to 6bcbfdc Compare July 15, 2024 21:22

eliliam approved these changes Aug 10, 2024

View reviewed changes

the-mikedavis reviewed Aug 16, 2024

View reviewed changes

the-mikedavis mentioned this pull request Aug 21, 2024

Allow insertion of file paths with autocomplete #11544

Closed

Philipp-M force-pushed the path-completion branch from 6bcbfdc to a0e10fc Compare August 24, 2024 19:17

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

Philipp-M force-pushed the path-completion branch from a0e10fc to cc19db1 Compare August 24, 2024 19:28

Handle windows CI

aaf7c20

Micro-optimization and revert of composing additional text edits

ee5b319

the-mikedavis reviewed Sep 1, 2024

View reviewed changes

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

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

Use PartialEq for LspCompletionItem and don't prefix-filter path …

be1e616

…completion items

		#[allow(clippy::unnecessary_cast)]
		acc.push(if mode & (p as u32) > 0 { s } else { '-' });

	#[allow(clippy::unnecessary_cast)]
	acc.push(if mode & (p as u32) > 0 { s } else { '-' });
	acc.push(if mode & p > 0 { *s } else { '-' });

Add support for path completion for unix paths #2608

Are you sure you want to change the base?

Add support for path completion for unix paths #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

ddogfoodd commented Apr 16, 2024 • edited Loading

nrdxp commented Jul 15, 2024

lukeflo commented Aug 10, 2024 • edited Loading

eliliam left a 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

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

Choose a reason for hiding this comment

Philipp-M commented Aug 24, 2024

Philipp-M commented Aug 24, 2024

Philipp-M commented Aug 24, 2024

Philipp-M commented Aug 29, 2024

the-mikedavis commented Sep 1, 2024

kik4444 commented Sep 18, 2024

Philipp-M commented Sep 18, 2024

kik4444 commented Sep 18, 2024

nrdxp commented Sep 18, 2024

RGBCube commented Sep 25, 2024

jerabaul29 commented Sep 29, 2024

David-Else commented Sep 29, 2024

Philipp-M commented May 29, 2022 •

edited

Loading

cole-h Nov 17, 2022 •

edited

Loading

ddogfoodd commented Apr 16, 2024 •

edited

Loading

lukeflo commented Aug 10, 2024 •

edited

Loading