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.

Sign up for GitHub

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
Open

Some fixes to my pull request for other underlines #904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3395312
Derived copy and clone for SetCursorStyle.
AhoyISki Jan 20, 2023
2d167d7
Merge branch 'crossterm-rs:master' into master
AhoyISki Jul 31, 2024
4f8ac70
Minor fixes to missing things from other underlines.
AhoyISki Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/style/stylize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ pub trait Stylize: Sized {
stylize_method!(reset Attribute::Reset);
stylize_method!(bold Attribute::Bold);
stylize_method!(underlined Attribute::Underlined);
stylize_method!(double_underlined Attribute::DoubleUnderlined);
stylize_method!(undercurled Attribute::Undercurled);
stylize_method!(underdotted Attribute::Underdotted);
stylize_method!(underdashed Attribute::Underdashed);
stylize_method!(reverse Attribute::Reverse);
stylize_method!(dim Attribute::Dim);
stylize_method!(italic Attribute::Italic);
Expand Down
22 changes: 11 additions & 11 deletions src/style/types/attribute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,16 +101,6 @@ Attribute! {
/// Underlines the text.
Underlined = 4,

// Other types of underlining
/// Double underlines the text.
DoubleUnderlined = 2,
/// Undercurls the text.
Undercurled = 3,
/// Underdots the text.
Underdotted = 4,
/// Underdashes the text.
Underdashed = 5,

/// Makes the text blinking (< 150 per minute).
SlowBlink = 5,
/// Makes the text blinking (>= 150 per minute).
Expand Down Expand Up @@ -151,6 +141,16 @@ Attribute! {
NotFramedOrEncircled = 54,
/// Turns off the `OverLined` attribute.
NotOverLined = 55,
// Other types of underlining

/// Double underlines the text.
DoubleUnderlined = 2,
/// Undercurls the text.
Undercurled = 3,
/// Underdots the text.
Underdotted = 4,
/// Underdashes the text.
Underdashed = 5,
}

impl Display for Attribute {
Expand All @@ -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 •
edited
Loading

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 •
edited
Loading

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).

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]);

}
SGR[self as usize].to_string()
Expand Down
Loading