-
Notifications
You must be signed in to change notification settings - Fork 21
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 support for hyperlinks and other OSC codes #43
Conversation
Thanks for this. I like your approach better. We want to stay away from breaking changes as much as possible. I'm wondering if FTCS should be added too? (maybe in a later PR) |
0299ca9
to
8eca162
Compare
I think it can be implemented in the same way. I suppose whether it's a separate PR or not is a matter of taste. At what point do you want to break it up into multiple commits? Multiple PRs? For example, I could break up this single commit into one per enum added (first commit for the hyperlink, next commit introduces the window title, etc.) and submit them all in this PR. (happy to do that) My sense is at some point separate PRs will be necessary though -- there are just too many OSC sequences to implement in one commit and/or PR. I chose these because they're a widely-supported subset of OSC escape codes to get started with and two of them (hyperlinks and window title) are of interest to me in another project. I didn't know about FTCS and OSC 133 before you mentioned it so I did some digging and thought I'd put it here for future reference: FTCS (which I guess stands for FinalTerm Control Sequence) uses OSC 133 (among others) to delineate portions of terminal output as either part of the prompt, part of the command exectued, or the command's output. External references:
|
I'm not picky. I prefer to let you decide because it depends on how long it takes. I wouldn't want a PR sitting waiting several weeks for something to get done when we can have several things added easily/quickly like this PR already does. But, if you'd prefer to put it all in one PR, I'm fine with that too. In the end, we squash and merge. Yes, you found the right research links for FTCS, sorry I wasn't more verbose. We use FTCS (among others) in nushell. I was just kind of thinking that it may make it easier if nu-ansi-term had that built in. I'm not sure it'll make any big difference though. You can see in the nushell repo, if you search for OSC, you'll find OCS 8 (links) and OSC 7 (title) as well as FTCS A, B, C, D (you'll have to search for 133; because I didn't label them with OSC). |
713b9b5
to
1db7223
Compare
Changes to the PR:
I think the PR is ready for review. |
These tests seem pretty stubborn, the ones that keep failing. :) |
That's odd. On my machine that assert is passing and one of the fixes I mentioned required changing the expected output from "...[4;..." to "...[04;...". At the moment I'm not seeing the cause of the problem but I'll keep looking. |
Ok, found it more quickly than expected. It's the gnu_legacy feature that's different between my test run versus that CI run. I'll fix that in the new testcases and re-push soon. |
1db7223
to
9fb239a
Compare
OK, I believe I managed to address the issue. In the src/display.rs test module I converted the feature flag to a string fed into assert_eq!(..., format!(...[{L}4;...)); #[cfg(feature = "gnu_legacy")] This seemed better than copy-pasting the expected strings and manually adjusting the expected output. It's a bit different than how I saw this solved elsewhere so I thought it'd be worth mentioning. Happy to change it if you prefer the other way. |
Looks like it failed again. We like to keep the code formatted with
I'd prefer to have two asserts, one with gnu_legacy and one without it. For me, tests are not really about optimal coding, it's about understanding what is being tested and knowing what to do when it fails. So, I just think it would be easier to have something along the lines of // Assemble with link first
let joined = AnsiStrings(&[link.clone(), after.clone()]).to_string();
#[cfg(feature = "gnu_legacy")]
assert_eq!(joined, format!("\x1B[04;34m\x1B]8;;https://example.com\x1B\\Link to example.com.\x1B]8;;\x1B\\\x1B[0m\x1B[32m After link.\x1B[0m"));
#[cfg(not(feature = "gnu_legacy"))]
assert_eq!(joined, format!("\x1B[4;34m\x1B]8;;https://example.com\x1B\\Link to example.com.\x1B]8;;\x1B\\\x1B[0m\x1B[32m After link.\x1B[0m")); |
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.
I agree with @fdncred that your proposed API is nicer from a compatibility/stability perspective than ogham#61.
With #5 we removed the transparent Deref
that lets the styled types act transparently like a base &str
but had some rather confusing behavior. So handling the OSC codes by directly writing into the string seems fine to me.
If you can resolve the tests, we are very happy to have this feature!
9fb239a
to
33265d0
Compare
Add support for producing colorized/stylized hyperlinks, among a selection of other OS Control (OSC) codes such as setting the window title, application/window icon, and notifying the terminal about the current working directory. There has already been some discussion and a change proposed for handling hyperlinks in the dormant rust-ansi-term repo: (See: ogham#61) The above proposed change breaks the Copy trait for Style and would require changing downstream projects that rely on it. These features aren't really about styling text so much as adding more information for the terminal emulator to present to the user outside of the typical area for rendered terminal output. So this change takes a different approach than taken in the referenced pull request. An enum describing the supported OSC codes, which is not exposed outside the crate, is used to indicate that a Style has additional terminal prefix and suffix output control codes to take care of for hyperlinks, titles, etc. These let us keep the prefix/suffix handling consistent. However rather than library users using these enums directly or calling externally visible functions on Style or Color struct, AnsiGenericString uses them to implement its hyperlink(), title(), etc. functions. These store the hyperlink "src" string, title, etc. within the AnsiGenericString rather than in the Style. Style remains Copy-able, and, since it already stores strings, AnsiGenericString traits are consistent with this choice. The locations of the functions better reflect what's happening because the supplied strings are not meant to be rendered inline with the ANSI-styled output. The OSControl enum also nicely describes the subset of OSC codes the package currently supports.
33265d0
to
715ab28
Compare
OK, I converted the asserts, dealt with all the clippy suggestions, and applied cargo fmt. Unless I'm misreading the example run output I think it should be good to go on my end at least. |
Thanks for this. Looks good! |
// The style's os control type, if it has one. | ||
// Used by corresponding public API functions in | ||
// AnsiGenericString. This allows us to keep the | ||
// prefix and suffix bits in the Style definition. | ||
pub(crate) oscontrol: Option<OSControl>, | ||
|
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.
This reverts commit c32266c.
Move OSControl out of Style PR nushell#43 created a regression where code that manually instantiated a Style could no longer be created without using the update operator and Default trait on Style. Rather than place non-public functions and data in Style move it all into AnsiGenericString. This has the added benefit of greatly simplifying the code. Also encloses the control sequences in \x01 .. \x02 to mark those portions which should be considered zero-width when displayed in the terminal. Fixes nushell#46
PR nushell#43 created a regression where code that manually instantiated a Style could no longer be created without using the update operator and Default trait on Style. Rather than place non-public functions and data in Style move it all into AnsiGenericString. This has the added benefit of greatly simplifying the code. Also encloses the control sequences in \x01 .. \x02 to mark those portions which should be considered zero-width when displayed in the terminal. Closes nushell#46
PR nushell#43 introduced a pub(crate) field in Style which broke the intended API (See: Issue nushell#46). Introduce a new test which will fail in those cases since it won't be able to initialize pub(crate) fields. Inspired by: nushell#46 (comment)
PR nushell#43 introduced a pub(crate) field in Style which broke the intended API (See: Issue nushell#46). Introduce a new test which will fail in those cases since it won't be able to initialize pub(crate) fields. Inspired by: nushell#46 (comment)
Move OSControl out of Style PR nushell#43 created a regression where code that manually instantiated a Style could no longer be created without using the update operator and Default trait on Style. Rather than place non-public functions and data in Style move it all into AnsiGenericString. This has the added benefit of greatly simplifying the code. Also encloses the control sequences in \x01 .. \x02 to mark those portions which should be considered zero-width when displayed in the terminal. Fixes nushell#46
PR nushell#43 introduced a pub(crate) field in Style which broke the intended API (See: Issue nushell#46). Introduce a new test which will fail in those cases since it won't be able to initialize pub(crate) fields. Inspired by: nushell#46 (comment)
Move OSControl out of Style PR nushell#43 created a regression where code that manually instantiated a Style could no longer be created without using the update operator and Default trait on Style. Rather than place non-public functions and data in Style move it all into AnsiGenericString. This has the added benefit of greatly simplifying the code. Fixes nushell#46
PR nushell#43 introduced a pub(crate) field in Style which broke the intended API (See: Issue nushell#46). Introduce a new test which will fail in those cases since it won't be able to initialize pub(crate) fields. Inspired by: nushell#46 (comment)
Move OSControl out of Style PR nushell#43 created a regression where code that manually instantiated a Style could no longer be created without using the update operator and Default trait on Style. Rather than place non-public functions and data in Style move it all into AnsiGenericString. This has the added benefit of greatly simplifying the code. Fixes nushell#46
PR nushell#43 introduced a pub(crate) field in Style which broke the intended API (See: Issue nushell#46). Introduce a new test which will fail in those cases since it won't be able to initialize pub(crate) fields. Inspired by: nushell#46 (comment)
Move OSControl out of Style PR nushell#43 created a regression where code that manually instantiated a Style could no longer be created without using the update operator and Default trait on Style. Rather than place non-public functions and data in Style move it all into AnsiGenericString. This has the added benefit of greatly simplifying the code. Fixes nushell#46
PR nushell#43 introduced a pub(crate) field in Style which broke the intended API (See: Issue nushell#46). Introduce a new test which will fail in those cases since it won't be able to initialize pub(crate) fields. Inspired by: nushell#46 (comment)
* Fix Style breakage Move OSControl out of Style PR #43 created a regression where code that manually instantiated a Style could no longer be created without using the update operator and Default trait on Style. Rather than place non-public functions and data in Style move it all into AnsiGenericString. This has the added benefit of greatly simplifying the code. Fixes #46 * testing: Add manual instance test for Style PR #43 introduced a pub(crate) field in Style which broke the intended API (See: Issue #46). Introduce a new test which will fail in those cases since it won't be able to initialize pub(crate) fields. Inspired by: #46 (comment) * Add examples of OSC usage * CI: Run OSC examples --------- Co-authored-by: Matt Helsley <matt.helsley+oss@gmail.com>
Add support for producing colorized/stylized hyperlinks, among a selection of other OS Control (OSC) codes such as setting the window title, application/window icon, and notifying the terminal about the current working directory. The main goal is to satisfy #6 while also supporting a few other very common OSC codes.
There has already been some discussion and a change proposed for handling hyperlinks in the dormant rust-ansi-term repo: (See: ogham#61) The above proposed change breaks the Copy trait for Style and would require changing downstream projects that rely on it.
These features aren't really about styling text so much as adding more information for the terminal emulator to present to the user either outside of the typical area for rendered terminal output or somewhat out-of-band with it.
So this change takes a different approach than taken in the referenced pull request.
An enum describing the supported OSC codes, which is not exposed outside the crate, is used to indicate that a Style has additional terminal prefix and suffix output control codes to take care of for hyperlinks, titles, etc. These let us keep the prefix/suffix handling consistent.
However rather than library users using these enums directly or calling externally visible functions on Style or Color struct, AnsiGenericString uses them to implement its hyperlink(), title(), etc. functions. These store the hyperlink "src" string, title, etc. within the
AnsiGenericString rather than in the Style.
Style remains Copy-able, and, since it already stores strings, AnsiGenericString traits are consistent with this choice. The locations of the functions better reflect what's happening because the supplied strings are not meant to be rendered inline with the ANSI-styled output.
The OSControl enum also nicely describes the subset of OSC codes the package currently supports.