Support going to specific positions in file

[xJonathanLEI]

Resolves #5259.

Now you can pipe clippy output into a file, open the file in Helix, and easily navigate to the exact locations of the warnings/errors.

[kirawi]

This is too specific to Rust, and so we probably want to use a more generic way of detecting this information. Ideally, I'd imagine that you could do something like cargo clippy | regex_query | hx.

[xJonathanLEI]

I think ideally we should support all common formats so that Helix always works out of the box.

In fact, I'd argue PATH:ROW:COL and PATH:ROW are not very Rust specific. A lot of compilers print paths in this way. For example, webpack prints in this exact format as well.

Also, one more argument for not relying on terminal piping has been specified in the linked issue (#5259): this feature would be very useful once integrated terminal is implemented, where you can't do any text transformation at all, so this workaround doesn't apply anymore.

[xJonathanLEI]

Actually, now that I think about it, we should support the absence of :COL as well. PATH:ROW:COL and PATH:ROW are very commonly used so I think also support it in this PR makes sense.

I just checked and clang also prints in this format. I guess most compilers just use this format.

[xJonathanLEI]

Added support for just PATH:ROW too (without :COL). Also updated the comment itself to be not Rust-specific.

[kirawi]

It can also be turned into a lazy static.

[archseer]

helix/helix-term/src/args.rs

Lines 90 to 100 in df1830e

	pub(crate) fn parse_file(s: &str) -> (PathBuf, Position) {
	let def = || (PathBuf::from(s), Position::default());
	if Path::new(s).exists() {
	return def();
	}
	split_path_row_col(s)
	.or_else(|| split_path_row(s))
	.unwrap_or_else(def)
	}
	/// Split file.rs:10:2 into [`PathBuf`], row and col.

[xJonathanLEI]

@archseer Ah I was actually referring to the file path detector used in vscode pointed out by @pascalkuthe:

https://github.com/microsoft/vscode/blob/1f00f5a6a62b5c860333be498b9f314b93f8afcb/src/vs/workbench/contrib/terminal/browser/links/terminalLocalLinkDetector.ts#L57

The simple case of PATH:ROW:COL already works pretty well in this PR with no additional dependency needed. Thefancy-regex crate is needed to port the whole vscode detector over (which allows us to have to same file linking capability as vscode, which works pretty awesome imo).

[xJonathanLEI]

Sorry. I studied the vscode detector regexs deeper and it turns out that those are not meant to be backreferences. \0 there actually intends to mean the unicode char \u0000 if I'm not mistaken. In that case, we actually don't need fancy-regex. The existing regex crate should work just fine.

Looks like regex101 is wrong. It says in the JavaScript regex engine \0 means backreference, but it seems to actually mean the \u0000 char based on my test in node.

[pascalkuthe]

Yeah I don't think you need to backtrack for the so.ole cases. I wanted to take a look at the regexes to confirm but didn't get the chance. I don't think we want to lull in fancy regex

[xJonathanLEI]

@pascalkuthe Yeah I was also wondering why they even needed backreferences at all. I guess I just trusted regex101 too much :/

[xJonathanLEI]

The reason I coded my own char index calculation is that the LSP impl is not forgiving enough: I want to allow "clamping" the designated position into the actual doc range in a predictable and reasonable way, whereas such a mismatch in the context of LSP would be considered a bug.

Actually, maybe I should refactor this into an option when calling editor.open? This would make adding a CLI option for opening a file at a specific position easier. Such a functionality would be useful with tools like zellij, which relies on a text editor to browser through the buffer. Right now if you use Helix for that, there's another gegl to press just to place the cursor at the end of the buffer as expected.

[the-mikedavis]

This would make adding a CLI option for opening a file at a specific position easier

You can provide line/cols already with CLI args: hx README.md:19:6. Does Zellij use a different format for this?

Also see

helix/helix-term/src/args.rs

Lines 90 to 98 in 8347139

	pub(crate) fn parse_file(s: &str) -> (PathBuf, Position) {
	let def = || (PathBuf::from(s), Position::default());
	if Path::new(s).exists() {
	return def();
	}
	split_path_row_col(s)
	.or_else(|| split_path_row(s))
	.unwrap_or_else(def)
	}

as archseer linked: can't we re-use or extend this function and use it for goto_file_impl?

[pascalkuthe]

This would make adding a CLI option for opening a file at a specific position easier

You can provide line/cols already with CLI args: hx README.md:19:6. Does Zellij use a different format for this?

Also see

helix/helix-term/src/args.rs

Lines 90 to 98 in 8347139

	pub(crate) fn parse_file(s: &str) -> (PathBuf, Position) {
	let def = || (PathBuf::from(s), Position::default());
	if Path::new(s).exists() {
	return def();
	}
	split_path_row_col(s)
	.or_else(|| split_path_row(s))
	.unwrap_or_else(def)
	}

as archseer linked: can't we re-use or extend this function and use it for goto_file_impl?

I also ran into that function and taught the same thing.
However as discussed above there are multiple different formats to specify file columns/rows.
The formats VSCode supports can be found here:

microsoft/vscode@1f00f5a/src/vs/workbench/contrib/terminal/browser/links/terminalLocalLinkDetector.ts#L57

I was under the impression that @xJonathanLEI implemented support for those captures but looking at the PR it does look like currently only <FILE>:<Line>:<COL> is supported.

Regardless the current implementation can not just use parse_file because this command used to use textobect_word to select text (which could be passed to parse_file) but textobject_word does not select colons so just passing the selection to parse_file would not actually work.

I think the best solution here would be to have a regex set of all possible file regexes (taken from vscode) and select the first regex capture that contains the current cursor. We could factor that regex out into a function tough and reuse it in parse_file if we want to support those extra path styles in the CLI aswell.

On a sidenote: Currently goto_file supports opening the current selection instead of using the word motions. I think this was implemented because the current word doesn't allow colons or slash. That requirement would be obsolete if a regex is used. Other goto commands (like goto_definition) only respect the cursor and ignore the selection so it would seem more consistent to do the same here (see #5260 (comment), #5260 (comment)).

[the-mikedavis]

I think the best solution here would be to have a regex set of all possible file regexes (taken from vscode) and select the first regex capture that contains the current cursor

This sounds like a good way to improve the smart detection in this block:

helix/helix-term/src/commands.rs

Lines 1037 to 1058 in 2b58ff4

	// Checks whether there is only one selection with a width of 1
	if selections.len() == 1 && primary.len() == 1 {
	let count = cx.count();
	let text_slice = text.slice(..);
	// In this case it selects the WORD under the cursor
	let current_word = textobject::textobject_word(
	text_slice,
	primary,
	textobject::TextObject::Inside,
	count,
	true,
	);
	// Trims some surrounding chars so that the actual file is opened.
	let surrounding_chars: &[_] = &['\'', '"', '(', ')'];
	paths.clear();
	paths.push(
	current_word
	.fragment(text_slice)
	.trim_matches(surrounding_chars)
	.to_string(),
	);
	}

But the automatic detection of whether surrounding text is a file/row/column should be separate from the parsing of the contents of the selection: when there is a selection (i.e. >1 width) goto-file should just parse the contents of the selection without trying to be smart about the surrounding text.

On a sidenote: Currently goto_file supports opening the current selection instead of using the word motions. I think this was implemented because the current word doesn't allow colons or slash.

I believe it's the other way around: the automatic detection was implemented on top of the parsing of the current selections. Kakoune only parses the contents of the selections since that fits the select-then-act paradigm (also see the original discussion in #1102 (comment)). The smart detection is a sort of separate feature that should only kick in if there's a single 1-width cursor.

[pascalkuthe]

I think the best solution here would be to have a regex set of all possible file regexes (taken from vscode) and select the first regex capture that contains the current cursor

This sounds like a good way to improve the smart detection in this block:

helix/helix-term/src/commands.rs

Lines 1037 to 1058 in 2b58ff4

	// Checks whether there is only one selection with a width of 1
	if selections.len() == 1 && primary.len() == 1 {
	let count = cx.count();
	let text_slice = text.slice(..);
	// In this case it selects the WORD under the cursor
	let current_word = textobject::textobject_word(
	text_slice,
	primary,
	textobject::TextObject::Inside,
	count,
	true,
	);
	// Trims some surrounding chars so that the actual file is opened.
	let surrounding_chars: &[_] = &['\'', '"', '(', ')'];
	paths.clear();
	paths.push(
	current_word
	.fragment(text_slice)
	.trim_matches(surrounding_chars)
	.to_string(),
	);
	}

But the automatic detection of whether surrounding text is a file/row/column should be separate from the parsing of the contents of the selection: when there is a selection (i.e. >1 width) goto-file should just parse the contents of the selection without trying to be smart about the surrounding text.

On a sidenote: Currently goto_file supports opening the current selection instead of using the word motions. I think this was implemented because the current word doesn't allow colons or slash.

I believe it's the other way around: the automatic detection was implemented on top of the parsing of the current selections. Kakoune only parses the contents of the selections since that fits the select-then-act paradigm (also see the original discussion in #1102 (comment)). The smart detection is a sort of separate feature that should only kick in if there's a single 1-width cursor.

Thanks for the additional context.

I was sort of coming from the standpoint of looking at goto_defintion , goto_reference, ... which all only use the cursor so it always seemed kind of odd to me that goto_file behaves differently and just assumed that was the reason. I imagine it would be a bit odd if I have part of the text select by accident and gf stopped working. I guess there are cases tough where you might want to select a postfix of a file (or exclude part of the selection with K) to produce a new path. I personally never taught about using gf like that but if that behavior was intentional then it makes sense to keep that.

In that case it probably works best to separate as a a OnceCell static so it can be reused for both the parse_file and the smart detection case.

[xJonathanLEI]

I was under the impression that @xJonathanLEI implemented support for those captures but looking at the PR it does look like currently only :: is supported.

Yep I was actually going to suggest replacing the "waiting-on-review" tag with "waiting-on-author" since I haven't implemented the vscode detector.

Regarding the behavior when a selection is used, here's my 2 cents after thinking about it more. The way I think about it is: do we lose support for any use case if we implement it one way over another? Since you can always collapse the selection into a single-char one with ; I think we should go for the "ignore surrounding text on non-single-char selections" approach as @the-mikedavis suggested.

Here's one missing use case if goto_file is overly "smart" and always reach for the surrounding text.

Imagine you generated the console logs containing the relative file path from the parent folder of CWD. Say you're in /a/b/c, but the command was run on /a/b, so it shows a path to your file as ./c/d/your_file. You know it's from a different folder, so you select the d/your_file part and do gf.

Oops, the editor is too smart, it finds out the "complete" path is ./c/d/your_file and tries to open that instead. That doesn't work unfortunately.

This is something vscode doesn't have to even think about since it's mouse-oriented.

[xJonathanLEI]

Update: I shifted focus to Git related stuff as I personally don't need the full-fledged detector and am happy with the current implementation here. Anyone interested in taking this PR across the finish line please feel free to do so :)

[mmlb]

vim-fetch also has some patterns that I think aren't in vscode's matcher: https://github.com/wsdjeg/vim-fetch/blob/master/autoload/fetch.vim#L12-L75