Skip to content
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

Some fixes to my pull request for other underlines #904

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AhoyISki
Copy link
Contributor

A while ago, I sent a pull request to add undercurls and other types of underline to crossterm.
I just realized that I forgot to include methods for doing that to styles.

Now, Stylize will include the double_underlined, undercurled, underdotted and underdashed methods.

This commit also makes the printing of these underlines slightly faster by changing the check for "4:{}" to be just one if statement instead of two.

It also implements Clone and Copy for SetCursorStyle, I did that a while ago though... so I don't really remember why.

@AhoyISki AhoyISki requested a review from TimonPost as a code owner July 31, 2024 23:47
@@ -175,7 +175,7 @@ impl Attribute {
///
/// See <https://en.wikipedia.org/wiki/ANSI_escape_code#SGR_parameters>
pub fn sgr(self) -> String {
if (self as usize) > 4 && (self as usize) < 9 {
if (self as usize) > 23 {
Copy link
Member

@TimonPost TimonPost Aug 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you elaborate on this check change? Before there was a range check of 5..9 and now everything above 23?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was changed to reduce the number of checks.

Previously, the special underlines were in the middle of the enum, so you had to check for a lower and an upper bound whenever they came up.

Now, I moved them to the end, so you only have to check for a lower bound.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, just asking real quick, but are there plans for merging this PR? Any other concerns with its contents? I'm asking because I have a crate that depends on my github version, and it is not possible to depend on a github version when publishing to crates.io

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

23 makes it difficult to work out how this code works and makes the code fragile to changes. Would this work better? It still assumes that the underline types are sequential, but makes it easier to understand what bounds are in play here.

Suggested change
if (self as usize) > 23 {
if (self as usize >= Self::DoubleUnderlined as usize) && (self as usize <= Self::Underdashed as usize) {

Copy link
Contributor Author

@AhoyISki AhoyISki Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is more clear, but you can remove the second check since the maximum value is known to be <= (Self::Underdashed as usize).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's safe unless you add some other variant at the end, or you move the options back somewhere within the enum like they were originally. Putting upper and lower bounds on the check makes it clear that the rationale is that these options are contiguous in the enums variants. I'd guess that having this check is unlikely to be on the hot path (and is probably even compiled out in a release build).

@@ -175,7 +175,7 @@ impl Attribute {
///
/// See <https://en.wikipedia.org/wiki/ANSI_escape_code#SGR_parameters>
pub fn sgr(self) -> String {
if (self as usize) > 4 && (self as usize) < 9 {
if (self as usize) > 23 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

23 makes it difficult to work out how this code works and makes the code fragile to changes. Would this work better? It still assumes that the underline types are sequential, but makes it easier to understand what bounds are in play here.

Suggested change
if (self as usize) > 23 {
if (self as usize >= Self::DoubleUnderlined as usize) && (self as usize <= Self::Underdashed as usize) {

@@ -175,7 +175,7 @@ impl Attribute {
///
/// See <https://en.wikipedia.org/wiki/ANSI_escape_code#SGR_parameters>
pub fn sgr(self) -> String {
if (self as usize) > 4 && (self as usize) < 9 {
if (self as usize) > 23 {
return "4:".to_string() + SGR[self as usize].to_string().as_str();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return "4:".to_string() + SGR[self as usize].to_string().as_str();
return format!("{}:{}", Self::Underlined as usize, SGR[self as usize]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants