-
Notifications
You must be signed in to change notification settings - Fork 281
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
base: master
Are you sure you want to change the base?
Conversation
@@ -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 { |
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.
can you elaborate on this check change? Before there was a range check of 5..9 and now everything above 23?
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 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.
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.
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
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.
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.
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 { |
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.
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.
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(); |
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.
return "4:".to_string() + SGR[self as usize].to_string().as_str(); | |
return format!("{}:{}", Self::Underlined as usize, SGR[self as usize]); |
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 thedouble_underlined
,undercurled
,underdotted
andunderdashed
methods.This commit also makes the printing of these underlines slightly faster by changing the check for
"4:{}"
to be just oneif
statement instead of two.It also implements
Clone
andCopy
forSetCursorStyle
, I did that a while ago though... so I don't really remember why.