-
-
Notifications
You must be signed in to change notification settings - Fork 816
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
Add hyperlink support to fd #1571
Conversation
Fixes: sharkdp#1295 Fixes: sharkdp#1563
Thank you! Should we maybe make this an option instead of a flag, to avoid future breaking changes in case we see the need for e.g. |
Something wrong happens if a filename contains non-latin characters. |
The problem, is I based the code on the implementation in ripgrep. But while ripgrep is writing directly to the stream, I am using a Formatter, which means I have to write characters, not raw bytes. Thus we need to percent encode all non-ascii bytes (or we could switch to writing bytes directly, but that would be more complicated, and I think percent encoding is safer anyway).
@tmccombs This latest commit seems to fix the bug with non-latin chars, thanks :) |
The value can be auto, always, or never. | ||
|
||
Currently, the default is "never", and if the option is used without an argument "auto" is | ||
used. In the future this may be changed to "auto" and "always". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I love OSC8 as well (thank you for the PR ❤️ ), I'm not sure they're so mainstream that we should go in that direction :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is how do terminals that don't support it handle the sequence? If most terminals silently ignore unrecognized OSC sequences, then it probably doesn't hurt to emit it even if it isn't supported. We'll need to test that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weston-terminal at least simply ignores the OSC 8 sequence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, LGTM!
The print separator change is quite nice as well, I hope it won't make too noise for the PR to be unchecked though :/ (please add links!)
|
||
impl fmt::Display for PathUrl { | ||
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { | ||
write!(f, "file://{}", host())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the hostname? Does file:///home/user/...
not work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. As long as fd is running on the same host as the terminal. Including the hostname is useful if you run the command over ssh or in a container or VM.
See https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda#file-uris-and-the-hostname
src/hyperlink.rs
Outdated
#[cfg(windows)] | ||
b'\\' => f.write_char('/'), | ||
_ => { | ||
write!(f, "%{:X}", byte) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RFC 3986 says that the byte 1
should become %01
not %1
. But to avoid having to know that, maybe we could just use https://crates.io/crates/urlencoding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortanetely, I don't think that urlencoding
would work, because it would percent encode "/", which isn't what we want.
When the first digit is 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much. Looks great.
Hi, this is a great development, but I think that it would be very valuable to allow more customization of the format of the hyperlink. For example, in my case I'd like to be able to specify the protocol (e.g. Here's the current full text of the entry for `rg --help` output--hyperlink-format=FORMAT Set the format of hyperlinks to use when printing results. Hyperlinks make certain elements of ripgrep's output, such as file paths, clickable. This generally only works in terminal emulators that support OSC-8 hyperlinks. For example, the format file://{host}{path} will emit an RFC 8089 hyperlink. To see the format that ripgrep is using, pass the --debug flag. |
It's sort of possible to do that today using the
I'm not completely opposed to implement multiple formats, but I didn't want to add that level of complexity in the initial implementation. |
Fixes: #1295
Fixes: #1563