Breaking change introduced by adding private field to transparent Style struct.

[sholderbach]

See the test failure in https://github.com/sharkdp/lscolors/actions/runs/5187090829/jobs/9349029439

#43 added a pub(crate) field to Style which blocks outside consumers of the crate to manually construct Style.

nu-ansi-term/src/style.rs

Lines 24 to 28 in 98b763f

	// 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>,

Previously this was possible as all fields were marked pub and the types accessible to the outside world.

Three possible courses of actions:

1. Embrace this breaking change and assume that Style shouldn't be built directly (and maybe hide all fields).
2. Accept a basic breaking change that introduces this field. Make OSControl public if necessary.\
3. Figure out a way to achieve the same behavior without modifying Style

I currently favor the latter two options as with the many crates around ANSI styling in the ecosystem e.g. owo-colors etc. being able to transparently convert the base style as another crate is valuable.

As a lesson learned we should probably add a test that constructs and destructures Style manually. This would make a breakage to the API a bit more obvious. (Also worth looking into tools like https://crates.io/crates/cargo-semver-checks)

[fdncred]

Dang, this just stinks. I really don't enjoy breaking everyone. Seems like we should figure out a way around this without breaking Style.

[mhelsley]

I agree -- I was trying to avoid breaking Style in the first place (the alternate implementation broke it by dropping the Copy trait) but clearly did not succeed.

2. Accept a basic breaking change that introduces this field. Make OSControl public if necessary.\
3. Figure out a way to achieve the same behavior without modifying Style

I currently favor the latter two options as with the many crates around ANSI styling in the ecosystem e.g. owo-colors etc. being able to transparently convert the base style as another crate is valuable.

I think option 3 is the best and there might be a way to do it. I need to think about it some more though.

Considering that there's already a commit that breaks external code that manually constructs Style { .. } I wonder if it might be wiser to immediately revert PR #43 for now to minimize pain and buy time so I can try for option 3. I could also add the test case you mentioned.

[alexkunde]

@mhelsley @fdncred what speaks against making the field just pub and adding a sensible default? (None seems to be your go-to for the json-serde test)
That is the way with_reset is implemented, which is released in the same release here.

Reconstruction is allowed and everyone using ..default:default() will not see a breaking change.

[fdncred]

@alexkunde I could probably live with your suggestion. @sholderbach What do you think?

[mhelsley]

@alexkunde
None is indeed a sensible default. Also, it might be good to mention in the docs that using the update syntax (e.g. ..Default::default() ) is recommended to future-proof manual creation of Style structs.

@sholderbach @fdncred

As for motivation for not using pub here:

I'm worried that exposing OSControl as part of the API might increase the likelihood of API users fiddling with it. That could introduce OSC sequences terminal emulators won't recognize or it might blow up the number of combinations nu-ansi-term will have to handle in the code in order to keep compatibility going forward. That's why my feeling is it would be preferable if there was some way to prevent code outside the crate from supplying anything except None in .oscontrol.

[alexkunde]

@mhelsley

In my opinion, the enum OSControl is already a rather safe bet, since we have a pre-defined set of possibilities.
Yes, you can have users requesting more codes, but that might even be valid at a later point in time.

The other way I would see is to create another lower level style object that is not exposed, with private values, and have the current Style fill that one. It would add overhead and some intransparency, but would protect against using OSC codes via Style.

In the end, it is @fdncred @sholderbach decision on how to proceed.

[sholderbach]

I think I am at this point convinced we should go with Option 3 and avoid having those OSC code related behaviors in Style. On top of the great points around terminal compatibility by @mhelsley here's my reasoning:

The beauty of having the transparent Style struct in the original ansi_term API in my view is that you can keep a Style instance around and can through .paint() print many string segments with that particular style instance. This is both convenient and through being directly modifiable may have some reasonably nice runtime performance.

But the OSC code stuff doesn't really fit that bill in my view. The ::title(), ::icon(), and ::cwd() functions don't fit this pattern as they are not characters printed directly in the terminal and instead affect the outside frame of the terminal emulator. As such you also can not really uphold the terminal API requirements for those magic OSCs by wrapping regular text with a particular Style through .paint().

Hyperlinks (and the terminal/shell-intergration markers for prompt and output location to some extent) are slightly different in that they are still text being printed and "styled" but you only want to do those operations once for a particular AnsiGenericString. It probably only makes sense to link one bit of text with a particular hyperlink. Reusing a style for which you once set OSControl::Hyperlink would probably be garbage.

In that sense the OSC operations should probably be methods and functions you can properly validate (potentially with termcap like checks even.)

[mhelsley]

@sholderbach @fdncred @alexkunde

I put up a new RFC branch (add-osc-attempt2) in my fork showing what I think just the API changes would look like. For now it starts with reverting the original commit as a convenience to get this going quickly so folks can have a look. I can quickly turn it into option 3 but this might be easier to review for any design issues before I go further.

It builds but is completely untested. I expect much of the test code will be very similar.

I even modeled state tracking for semantic prompt functionality. Though I'm unsure that I recall the states and transitions correctly it still shows how it might be used to constrain callers to ensure they comply with valid state transitions while giving some idea of what more complex OSC codes might need. The idea is each creating a new instance in a subsequent state requires the instance with the previous state and starting with the "prompt" or None state.

[mhelsley]

@sholderbach

As a lesson learned we should probably add a test that constructs and destructures Style manually. This would make a breakage to the API a bit more obvious.

I've written a test that manually constructs Style and would detect a future breaking change. However, I'm not sure I know what you had in mind for a destructuring test. I'm fairly new to Rust so I'm having a hard time imagining problems a destructuring test might uncover that a manual construction test would not. If you could show me a snippet of the destructuring I could add that test.

@sholderbach

In that sense the OSC operations should probably be methods and functions you can properly validate (potentially with termcap like checks even.)

I don't think I understand your termcap reference in the context of validating things here. Do you have a link to an example of termcap-like checks you're referring to or perhaps some quick pseudocode?

For the OSC sequences, so far, I've just modified the original ::hyperlink(), ::title(), ::icon(), and ::cwd() tests.

[sholderbach]

I've written a test that manually constructs Style and would detect a future breaking change. However, I'm not sure I know what you had in mind for a destructuring test. I'm fairly new to Rust so I'm having a hard time imagining problems a destructuring test might uncover that a manual construction test would not. If you could show me a snippet of the destructuring I could add that test.

Yeah that should suffice for testing. I was thinking of extracting the fields through pattern matching in a let or match but that shouldn't help the test in any additional way.

I don't think I understand your termcap reference in the context of validating things here. Do you have a link to an example of termcap-like checks you're referring to or perhaps some quick pseudocode?

There is on some systems (a) file(s) that could through cryptic codes describe what a terminal can or cannot do and how it is set up at the moment. See man termcap or man terminfo but that doesn't really translate on all platforms. Also it may not be faithfully implemented especially when we are dealing with a terminal where the availability of certain capabilities is in question.

For the OSC sequences, so far, I've just modified the original ::hyperlink(), ::title(), ::icon(), and ::cwd() tests.

Awesome! Thanks

[sylvestre]

would it be possible to have a new release of nu-ansi-term with this change?
(i already made the mistake of uploading lscolors & nu-ansi-term in debian)

[amtoine]

@sylvestre
the next release of Nushell will be on the 25th of july, so i think nu-ansi-term will be released at the same time if there are new feature to include 😊

we switched to a 4-week schedule to buy us more time between releases towards 1.0 😌

[sholderbach]

Sorry @amtoine, but what you are suggesting is simply unworkable as we use nu-ansi-term types to interface with third-party crates like lscolors. We will make nu-ansi-term releases with ample time before the Nushell releases to coordinate with the ecosystem, and only if necessary.

To your question @sylvestre, I will try to check a build of nushell (using lscolors) incorporating #47 and then make the release if nothing problematic pops up.

[fdncred]

Thanks @sholderbach. I agree. It's kind of a pain doing this. It looks to me like we'll need to release it so that lscolors can use the with_reset changes. I didn't look to see if there was a PR on lscolors for that yet. Maybe they're waiting on us?

[sholderbach]

Alright tested nu with lscolors and reedline using the new version. Everything still seems to work. The only breaking change which was expected was the with_reset field added to manual constructions (#40). (are we happy with the name?)
Have not changed the nushell link construction to directly use the new link support, that would be the final test.

[fdncred]

are we happy with the name?

We could be more verbose like starts_with_reset but I'm not sure it's necessary. What do you think?

[sylvestre]

@sholderbach thanks, I appreciate it. This issue is preventing upgrades of the Rust/coreutils in Debian/Ubuntu :)