From 4e41d142fd73df7de967e12684ba9ff5fcb24300 Mon Sep 17 00:00:00 2001 From: Nahor Date: Tue, 3 Mar 2026 14:09:18 -0800 Subject: [PATCH] set_color: allow resetting the underline style Part 2/3 of #12495 Part of #12507 --- CHANGELOG.rst | 2 +- doc_src/cmds/set_color.rst | 2 +- src/highlight/highlight.rs | 13 ++++--- src/terminal.rs | 71 +++++++++++++++++++++---------------- src/text_face.rs | 60 +++++++++++++++---------------- tests/checks/set_color.fish | 2 ++ 6 files changed, 81 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index bcf90436d..ac3768261 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -4,7 +4,7 @@ fish ?.?.? (released ???) Notable improvements and fixes ------------------------------ - New Spanish translations (:issue:`12489`). -- ``set_color`` is able to turn off italics, reverse mode and strikethrough individually (e.g. ``--italics=off``). +- ``set_color`` is able to turn off italics, reverse mode, strikethrough and underline individually (e.g. ``--italics=off``). For distributors and developers ------------------------------- diff --git a/doc_src/cmds/set_color.rst b/doc_src/cmds/set_color.rst index fd23d1d54..93703eef0 100644 --- a/doc_src/cmds/set_color.rst +++ b/doc_src/cmds/set_color.rst @@ -58,7 +58,7 @@ The following options are available: Sets strikethrough mode. The state can be **on** / **true** (default), or **off** / **false** **-u** or **--underline**, or **-uSTYLE** or **--underline=STYLE** - Set the underline mode; supported styles are **single** (default), **double**, **curly**, **dotted** and **dashed**. + Set the underline mode; supported styles are **single** (default), **double**, **curly**, **dotted**, **dashed** and **off**. **--theme=THEME** Ignored. diff --git a/src/highlight/highlight.rs b/src/highlight/highlight.rs index 1e4360ba3..f053ce2e8 100644 --- a/src/highlight/highlight.rs +++ b/src/highlight/highlight.rs @@ -181,7 +181,7 @@ fn resolve_spec_uncached(highlight: &HighlightSpec, vars: &dyn Environment) -> T face.bg = bg_face.bg; // In case the background role is different from the foreground one, we ignore its style // except for reverse mode. - if face.style.reverse != ResettableStyle::On { + if face.style.reverse != ResettableStyle::On(()) { face.style.reverse = bg_face.style.reverse; } } @@ -202,7 +202,8 @@ fn resolve_spec_uncached(highlight: &HighlightSpec, vars: &dyn Environment) -> T } if highlight.force_underline { - face.style.inject_underline(UnderlineStyle::Single); + face.style + .inject_underline(ResettableStyle::On(UnderlineStyle::Single)); } face @@ -1318,7 +1319,7 @@ mod tests { use crate::operation_context::{EXPANSION_LIMIT_BACKGROUND, OperationContext}; use crate::prelude::*; use crate::tests::prelude::*; - use crate::text_face::UnderlineStyle; + use crate::text_face::{ResettableStyle, UnderlineStyle}; use libc::PATH_MAX; // Helper to return a string whose length greatly exceeds PATH_MAX. @@ -1854,7 +1855,7 @@ fn test_trailing_spaces_after_command() { let face = resolver.resolve_spec(&colors[i], vars); assert_eq!( face.style.underline_style(), - Some(UnderlineStyle::Single), + ResettableStyle::On(UnderlineStyle::Single), "Character at position {} of 'echo' should be underlined", i ); @@ -1865,7 +1866,7 @@ fn test_trailing_spaces_after_command() { let face = resolver.resolve_spec(&colors[i], vars); assert_eq!( face.style.underline_style(), - None, + ResettableStyle::Off, "Trailing space at position {} should NOT be underlined", i ); @@ -1922,5 +1923,7 @@ fn test_parse_text_face_for_highlight_fully_specified() { assert_all_set(vec![L!("--reverse=off").into()]); assert_all_set(vec![L!("--strikethrough").into()]); assert_all_set(vec![L!("--strikethrough=off").into()]); + assert_all_set(vec![L!("--underline").into()]); + assert_all_set(vec![L!("--underline=off").into()]); } } diff --git a/src/terminal.rs b/src/terminal.rs index 491fe6826..444788065 100644 --- a/src/terminal.rs +++ b/src/terminal.rs @@ -390,7 +390,7 @@ fn set_text_face_internal( // Removes all styles that are individually resettable. let non_resettable = |mut style: TextStyling| { style.italics = ResettableStyle::Unchanged; - style.underline_style = None; + style.underline_style = ResettableStyle::Unchanged; style.reverse = ResettableStyle::Unchanged; style.strikethrough = ResettableStyle::Unchanged; style @@ -448,14 +448,16 @@ fn set_text_face_internal( if style.underline_style != style_writer.last().style.underline_style { match style.underline_style { - None => { + ResettableStyle::Unchanged => {} + ResettableStyle::Off => { if style_writer.write_command(ExitUnderlineMode) { - style_writer.last().style.underline_style = None; + style_writer.last().style.underline_style = ResettableStyle::Off; } } - Some(underline_style) => { + ResettableStyle::On(underline_style) => { if style_writer.write_command(EnterUnderlineMode(underline_style)) { - style_writer.last().style.underline_style = Some(underline_style); + style_writer.last().style.underline_style = + ResettableStyle::On(underline_style); } } } @@ -481,7 +483,7 @@ fn set_text_face_internal( ResettableStyle::Unchanged => { return; } - ResettableStyle::On => enter_cmd, + ResettableStyle::On(()) => enter_cmd, ResettableStyle::Off => exit_cmd, }; if style_writer.write_command(cmd) { @@ -790,7 +792,7 @@ fn drop(&mut self) { mod tests { use fish_color::{Color, Color24}; - use crate::text_face::{ResettableStyle, TextFace, TextStyling}; + use crate::text_face::{TextFace, TextStyling, UnderlineStyle}; use super::{ Outputter, @@ -889,45 +891,54 @@ fn sgr_max_length() { #[test] fn resettable_style_attribute() { - use ResettableStyle::{Off, On, Unchanged}; + type RS = crate::text_face::ResettableStyle<()>; + type RU = crate::text_face::ResettableStyle; let mut outp = Outputter::new_buffering_no_assume_normal(); - let mut set_attr = - |italics: ResettableStyle, reverse: ResettableStyle, strikethrough: ResettableStyle| { - let mut style = TextStyling::unknown_style(); - style.italics = italics; - style.reverse = reverse; - style.strikethrough = strikethrough; + let mut set_attr = |italics: RS, reverse: RS, strikethrough: RS, underline: RU| { + let mut style = TextStyling::unknown(); + style.italics = italics; + style.reverse = reverse; + style.strikethrough = strikethrough; + style.underline_style = underline; - let face = TextFace::new(Color::None, Color::None, Color::None, style); - outp.set_text_face(face); - }; + let face = TextFace::new(Color::None, Color::None, Color::None, style); + outp.set_text_face(face); + }; - // `#[cfg_attr(...)]` because `#[rustfmt::skip]` triggers `error[E0658]: attributes on expressions are experimental` + // TODO: feature(stmt_expr_attributes): use #[rustfmt::skip] #[cfg_attr(any(), rustfmt::skip)] { - set_attr(On, Unchanged, Off); - set_attr(On, On, Unchanged); - set_attr(Unchanged, On, Unchanged); - set_attr(Unchanged, Unchanged, On); - set_attr(Off, Unchanged, On); - set_attr(Off, Off, Unchanged); - set_attr(Unchanged, Off, Unchanged); - set_attr(Unchanged, Unchanged, Off); + // There is no particular order between the different attributes. + // The main test is that for a given attribute, setting the same + // value twice (e.g. On->On) shouldn't create a new escape sequence, + // except for Unchanged which should never create a new sequence + // (which also means testing Unchanged->Unchanged is not required) + // Cherry on top: by changing what value is used first for each + // attribute, we also further exercise SGR combining, including + // an empty result. + set_attr(RS::On(()), RS::Unchanged, RS::Off, RU::On(UnderlineStyle::Curly)); + set_attr(RS::On(()), RS::On(()), RS::Unchanged, RU::On(UnderlineStyle::Curly)); + set_attr(RS::Unchanged, RS::On(()), RS::Unchanged, RU::Unchanged); + set_attr(RS::Unchanged, RS::Unchanged, RS::On(()), RU::Off); + set_attr(RS::Off, RS::Unchanged, RS::On(()), RU::Off); + set_attr(RS::Off, RS::Off, RS::Unchanged, RU::Unchanged); + set_attr(RS::Unchanged, RS::Off, RS::Unchanged, RU::On(UnderlineStyle::Dashed)); + set_attr(RS::Unchanged, RS::Unchanged, RS::Off, RU::On(UnderlineStyle::Dotted)); } assert_eq!( String::from_utf8_lossy(outp.contents()), concat!( - "\u{1b}[3;29m", + "\u{1b}[4:3;3;29m", "\u{1b}[7m", "", - "\u{1b}[9m", + "\u{1b}[24;9m", "\u{1b}[23m", "\u{1b}[27m", - "", - "\u{1b}[29m", + "\u{1b}[4:5m", + "\u{1b}[4:4;29m", ) ); } diff --git a/src/text_face.rs b/src/text_face.rs index afef6df47..a0967108b 100644 --- a/src/text_face.rs +++ b/src/text_face.rs @@ -9,14 +9,20 @@ pub(crate) trait StyleSet { } #[derive(Default, Debug, Copy, Clone, PartialEq, Eq)] -pub(crate) enum ResettableStyle { +pub(crate) enum ResettableStyle +where + T: Copy + std::fmt::Debug + Eq, +{ #[default] Unchanged, Off, - On, + On(T), } -impl StyleSet for ResettableStyle { +impl StyleSet for ResettableStyle +where + T: Copy + std::fmt::Debug + Eq, +{ fn union_prefer_right(self, other: Self) -> Self { if other == Self::Unchanged { self @@ -43,23 +49,10 @@ pub(crate) enum UnderlineStyle { Dashed, } -impl StyleSet for Option { - fn union_prefer_right(self, other: Self) -> Self { - other.or(self) - } - - fn difference_prefer_empty(self, other: Self) -> Self { - if other.is_some() { - return None; - } - self - } -} - #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub(crate) struct TextStyling { pub(crate) bold: bool, - pub(crate) underline_style: Option, + pub(crate) underline_style: ResettableStyle, pub(crate) italics: ResettableStyle, pub(crate) dim: bool, pub(crate) reverse: ResettableStyle, @@ -70,7 +63,7 @@ impl TextStyling { pub(crate) const fn terminal_default() -> Self { Self { bold: false, - underline_style: None, + underline_style: ResettableStyle::Off, italics: ResettableStyle::Off, dim: false, reverse: ResettableStyle::Off, @@ -80,7 +73,7 @@ pub(crate) const fn terminal_default() -> Self { pub(crate) const fn unknown() -> Self { Self { bold: false, - underline_style: None, + underline_style: ResettableStyle::Unchanged, italics: ResettableStyle::Unchanged, dim: false, reverse: ResettableStyle::Unchanged, @@ -94,7 +87,8 @@ pub(crate) fn is_empty(&self) -> bool { #[cfg(test)] pub(crate) fn all_set(&self) -> bool { - (self.italics != ResettableStyle::Unchanged) + (self.underline_style != ResettableStyle::Unchanged) + && (self.italics != ResettableStyle::Unchanged) && (self.reverse != ResettableStyle::Unchanged) && (self.strikethrough != ResettableStyle::Unchanged) } @@ -132,13 +126,13 @@ pub const fn is_bold(self) -> bool { } #[cfg(test)] - pub const fn underline_style(self) -> Option { + pub const fn underline_style(self) -> ResettableStyle { self.underline_style } /// Set the given underline style. - pub fn inject_underline(&mut self, underline: UnderlineStyle) { - self.underline_style = Some(underline); + pub fn inject_underline(&mut self, underline: ResettableStyle) { + self.underline_style = underline; } /// Returns whether the text face is dim. @@ -248,12 +242,12 @@ pub(crate) enum ParseError<'args> { fn parse_resettable_style<'a>(w: &WGetopter<'_, 'a, '_>) -> Result { let Some(arg) = w.woptarg else { - return Ok(ResettableStyle::On); + return Ok(ResettableStyle::On(())); }; if (arg == "off") || (arg == "false") { Ok(ResettableStyle::Off) } else if (arg == "on") || (arg == "true") { - Ok(ResettableStyle::On) + Ok(ResettableStyle::On(())) } else { Err(arg) } @@ -334,19 +328,21 @@ pub(crate) fn parse_text_face_and_options<'argarray, 'args>( } 'u' => { let arg = w.woptarg.unwrap_or(L!("single")); - style.underline_style = Some(if arg == "single" { - UnderlineStyle::Single + style.underline_style = if arg == "single" { + ResettableStyle::On(UnderlineStyle::Single) } else if arg == "double" { - UnderlineStyle::Double + ResettableStyle::On(UnderlineStyle::Double) } else if arg == "curly" { - UnderlineStyle::Curly + ResettableStyle::On(UnderlineStyle::Curly) } else if arg == "dotted" { - UnderlineStyle::Dotted + ResettableStyle::On(UnderlineStyle::Dotted) } else if arg == "dashed" { - UnderlineStyle::Dashed + ResettableStyle::On(UnderlineStyle::Dashed) + } else if arg == "off" { + ResettableStyle::Off } else { return Err(UnknownUnderlineStyle(arg)); - }); + }; } 'c' => { assert!(is_builtin); diff --git a/tests/checks/set_color.fish b/tests/checks/set_color.fish index 403b105df..396ef4b9e 100644 --- a/tests/checks/set_color.fish +++ b/tests/checks/set_color.fish @@ -136,6 +136,8 @@ string escape (set_color --underline=dotted) # CHECK: \e\[4:4m string escape (set_color --underline=dashed) # CHECK: \e\[4:5m +string escape (set_color --underline=off) +# CHECK: \e\[24m string escape (set_color f00 --background=00f --underline-color=0f0 --bold --dim --italics --reverse --strikethrough --underline=curly) # CHECK: \e\[38\;2\;255\;0\;0\;48\;2\;0\;0\;255\;58:2::0:255:0\;1\;4:3\;2\;3\;7m\e\[9m