From 037c1896d45ca174dbaef0ba2297f24dc4bad9f5 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sun, 13 Apr 2025 12:16:16 +0200 Subject: [PATCH] Reuse wgetopt parsing for set_color for internal colors Not sure if this is 100% a good idea but it does remove some duplication. While at it, change parse_color_maybe_none to actually return Option::None and not Color::None which means something else (see a following commit). --- src/builtins/set_color.rs | 93 ++++++++-------------- src/highlight/highlight.rs | 8 +- src/terminal.rs | 32 ++++---- src/text_face.rs | 158 +++++++++++++++++++++++++------------ 4 files changed, 158 insertions(+), 133 deletions(-) diff --git a/src/builtins/set_color.rs b/src/builtins/set_color.rs index 5bca2448c..37fdacd58 100644 --- a/src/builtins/set_color.rs +++ b/src/builtins/set_color.rs @@ -5,7 +5,10 @@ use crate::common::str2wcstring; use crate::terminal::TerminalCommand::ExitAttributeMode; use crate::terminal::{best_color, get_color_support, Output, Outputter}; -use crate::text_face::{TextFace, TextStyling}; +use crate::text_face::{ + parse_text_face_and_options, TextFace, TextFaceArgsAndOptions, TextFaceArgsAndOptionsResult, + TextStyling, +}; fn print_colors(streams: &mut IoStreams, args: &[&wstr], style: TextStyling, bg: Color) { let outp = &mut Outputter::new_buffering(); @@ -37,18 +40,6 @@ fn print_colors(streams: &mut IoStreams, args: &[&wstr], style: TextStyling, bg: streams.out.append(str2wcstring(contents)); } -const SHORT_OPTIONS: &wstr = L!(":b:hoidrcu"); -const LONG_OPTIONS: &[WOption] = &[ - wopt(L!("background"), ArgType::RequiredArgument, 'b'), - wopt(L!("help"), ArgType::NoArgument, 'h'), - wopt(L!("bold"), ArgType::NoArgument, 'o'), - wopt(L!("underline"), ArgType::NoArgument, 'u'), - wopt(L!("italics"), ArgType::NoArgument, 'i'), - wopt(L!("dim"), ArgType::NoArgument, 'd'), - wopt(L!("reverse"), ArgType::NoArgument, 'r'), - wopt(L!("print-colors"), ArgType::NoArgument, 'c'), -]; - /// set_color builtin. pub fn set_color(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> BuiltinResult { // Variables used for parsing the argument list. @@ -60,47 +51,33 @@ pub fn set_color(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) - return Err(STATUS_CMD_ERROR); } - let mut bgcolor = None; - let mut style = TextStyling::empty(); - let mut print = false; - - let mut w = WGetopter::new(SHORT_OPTIONS, LONG_OPTIONS, argv); - while let Some(c) = w.next_opt() { - match c { - 'b' => { - assert!(w.woptarg.is_some(), "Arg should have been set"); - bgcolor = w.woptarg; - } - 'h' => { - builtin_print_help(parser, streams, argv[0]); - return Ok(SUCCESS); - } - 'o' => style |= TextStyling::BOLD, - 'i' => style |= TextStyling::ITALICS, - 'd' => style |= TextStyling::DIM, - 'r' => style |= TextStyling::REVERSE, - 'u' => style |= TextStyling::UNDERLINE, - 'c' => print = true, - ':' => { - // We don't error here because "-b" is the only option that requires an argument, - // and we don't error for missing colors. - return Err(STATUS_INVALID_ARGS); - } - '?' => { - builtin_unknown_option( - parser, - streams, - L!("set_color"), - argv[w.wopt_index - 1], - true, /* print_hints */ - ); - return Err(STATUS_INVALID_ARGS); - } - _ => unreachable!("unexpected retval from WGetopter"), + let TextFaceArgsAndOptions { + mut wopt_index, + bgcolor, + style, + print_color_mode, + } = match parse_text_face_and_options(argv, /*is_builtin=*/ true) { + TextFaceArgsAndOptionsResult::Ok(parsed_face) => parsed_face, + TextFaceArgsAndOptionsResult::PrintHelp => { + builtin_print_help(parser, streams, argv[0]); + return Ok(SUCCESS); } - } - // We want to reclaim argv so grab wopt_index now. - let mut wopt_index = w.wopt_index; + TextFaceArgsAndOptionsResult::InvalidArgs => { + // We don't error here because "-b" is the only option that requires an argument, + // and we don't error for missing colors. + return Err(STATUS_INVALID_ARGS); + } + TextFaceArgsAndOptionsResult::UnknownOption(unknown_option_index) => { + builtin_unknown_option( + parser, + streams, + L!("set_color"), + argv[unknown_option_index], + true, /* print_hints */ + ); + return Err(STATUS_INVALID_ARGS); + } + }; let mut parse_color = |color_str| { Color::from_wstr(color_str).ok_or_else(|| { @@ -118,7 +95,7 @@ pub fn set_color(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) - bg = parse_color(bgcolor)?; } - if print { + if print_color_mode { // Hack: Explicitly setting a background of "normal" crashes // for --print-colors. Because it's not interesting in terms of display, // just skip it. @@ -137,10 +114,8 @@ pub fn set_color(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) - wopt_index += 1; } - // #1323: We may have multiple foreground colors. Choose the best one. If we had no foreground - // color, we'll get none(); if we have at least one we expect not-none. - let fg = best_color(&fgcolors, get_color_support()); - assert!(fgcolors.is_empty() || !fg.is_none()); + // #1323: We may have multiple foreground colors. Choose the best one. + let fg = best_color(fgcolors.into_iter(), get_color_support()); let outp = &mut Outputter::new_buffering(); outp.set_text_face(TextFace::new(Color::None, Color::None, style)); @@ -148,7 +123,7 @@ pub fn set_color(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) - outp.write_command(ExitAttributeMode); } - if !fg.is_none() { + if let Some(fg) = fg { if fg.is_normal() || fg.is_reset() { outp.write_command(ExitAttributeMode); } else if !outp.write_color(fg, true /* is_fg */) { diff --git a/src/highlight/highlight.rs b/src/highlight/highlight.rs index fb2944b45..780068718 100644 --- a/src/highlight/highlight.rs +++ b/src/highlight/highlight.rs @@ -185,8 +185,6 @@ pub(crate) fn resolve_spec_uncached( } /// Return the internal color code representing the specified color. -/// TODO: This code should be refactored to enable sharing with builtin_set_color. -/// In particular, the argument parsing still isn't fully capable. pub(crate) fn parse_text_face_for_highlight( fg_var: Option<&EnvVar>, bg_var: Option<&EnvVar>, @@ -195,10 +193,8 @@ pub(crate) fn parse_text_face_for_highlight( let Some(var) = maybe_var else { return (Color::Normal, TextStyling::empty()); }; - let (mut color, style) = parse_text_face(var.as_list(), is_background); - if color.is_none() { - color = Color::Normal; - } + let (color, style) = parse_text_face(var.as_list(), is_background); + let color = color.unwrap_or(Color::Normal); (color, style) }; let (fg, fg_style) = parse_var(fg_var, false); diff --git a/src/terminal.rs b/src/terminal.rs index 9ebfc035e..3fed60927 100644 --- a/src/terminal.rs +++ b/src/terminal.rs @@ -729,34 +729,30 @@ fn write_bytes(&mut self, buf: &[u8]) { /// Given a list of RgbColor, pick the "best" one, as determined by the color support. Returns /// RgbColor::NONE if empty. -pub fn best_color(candidates: &[Color], support: ColorSupport) -> Color { - if candidates.is_empty() { - return Color::None; - } - - let mut first_rgb = Color::None; - let mut first_named = Color::None; +pub fn best_color(candidates: impl Iterator, support: ColorSupport) -> Option { + let mut first = None; + let mut first_rgb = None; + let mut first_named = None; for color in candidates { + if first.is_none() { + first = Some(color); + } if first_rgb.is_none() && color.is_rgb() { - first_rgb = *color; + first_rgb = Some(color); } if first_named.is_none() && color.is_named() { - first_named = *color; + first_named = Some(color); } } // If we have both RGB and named colors, then prefer rgb if term256 is supported. - let mut result; let has_term256 = support.contains(ColorSupport::TERM_256COLOR); - if (!first_rgb.is_none() && has_term256) || first_named.is_none() { - result = first_rgb; + (if (first_rgb.is_some() && has_term256) || first_named.is_none() { + first_rgb } else { - result = first_named; - } - if result.is_none() { - result = candidates[0]; - } - result + first_named + }) + .or(first) } /// The [`Term`] singleton. Initialized via a call to [`setup()`] and surfaced to the outside world via [`term()`]. diff --git a/src/text_face.rs b/src/text_face.rs index e5a3bf42b..6cae8e28b 100644 --- a/src/text_face.rs +++ b/src/text_face.rs @@ -3,6 +3,7 @@ use crate::color::Color; use crate::terminal::{best_color, get_color_support}; use crate::wchar::prelude::*; +use crate::wgetopt::{wopt, ArgType, WGetopter, WOption}; bitflags! { #[derive(Debug, Default, Copy, Clone, PartialEq, Eq)] @@ -87,56 +88,113 @@ pub fn set_reverse(&mut self, reverse: bool) { } } -pub fn parse_text_face(arguments: &[WString], is_background: bool) -> (Color, TextStyling) { - let mut style = TextStyling::empty(); - let mut candidates: Vec = Vec::new(); - - let prefix = L!("--background="); - - let mut next_is_background = false; - for arg in arguments { - let mut color_name = None; - #[allow(clippy::collapsible_else_if)] - if is_background { - if next_is_background { - color_name = Some(arg.as_utfstr()); - next_is_background = false; - } else if arg.starts_with(prefix) { - // Look for something like "--background=red". - color_name = Some(arg.slice_from(prefix.char_count())); - } else if arg == "--background" || arg == "-b" { - // Without argument attached the next token is the color - // - if it's another option it's an error. - next_is_background = true; - } else if arg == "--reverse" || arg == "-r" { - // Reverse should be meaningful in either context - style |= TextStyling::REVERSE; - } else if arg.starts_with("-b") { - // Look for something like "-bred". - // Yes, that length is hardcoded. - color_name = Some(arg.slice_from(2)); - } - } else { - if arg == "--bold" || arg == "-o" { - style |= TextStyling::BOLD; - } else if arg == "--underline" || arg == "-u" { - style |= TextStyling::UNDERLINE; - } else if arg == "--italics" || arg == "-i" { - style |= TextStyling::ITALICS; - } else if arg == "--dim" || arg == "-d" { - style |= TextStyling::DIM; - } else if arg == "--reverse" || arg == "-r" { - style |= TextStyling::REVERSE; - } else { - color_name = Some(arg.as_utfstr()); - } - } - - if let Some(color) = color_name.and_then(Color::from_wstr) { - candidates.push(color); - } +pub fn parse_text_face(arguments: &[WString], is_background: bool) -> (Option, TextStyling) { + let mut argv: Vec<&wstr> = Some(L!("")) + .into_iter() + .chain(arguments.iter().map(|s| s.as_utfstr())) + .collect(); + let TextFaceArgsAndOptions { + wopt_index, + bgcolor, + mut style, + print_color_mode, + } = match parse_text_face_and_options(&mut argv, /*is_builtin=*/ false) { + TextFaceArgsAndOptionsResult::Ok(parsed_text_faces) => parsed_text_faces, + TextFaceArgsAndOptionsResult::PrintHelp + | TextFaceArgsAndOptionsResult::InvalidArgs + | TextFaceArgsAndOptionsResult::UnknownOption(_) => unreachable!(), + }; + assert!(!print_color_mode); + let color; + if is_background { + color = bgcolor.and_then(Color::from_wstr); + // We ignore styles for dedicated background roles but reverse should be meaningful in + // either context + style &= TextStyling::REVERSE; + } else { + color = best_color( + argv[wopt_index..] + .iter() + .filter_map(|&color| Color::from_wstr(color)), + get_color_support(), + ); } - - let color = best_color(&candidates, get_color_support()); + assert!(color.map_or(true, |color| !color.is_none())); (color, style) } + +pub(crate) struct TextFaceArgsAndOptions<'a> { + pub(crate) wopt_index: usize, + pub(crate) bgcolor: Option<&'a wstr>, + pub(crate) style: TextStyling, + pub(crate) print_color_mode: bool, +} + +pub(crate) enum TextFaceArgsAndOptionsResult<'a> { + Ok(TextFaceArgsAndOptions<'a>), + PrintHelp, + InvalidArgs, + UnknownOption(usize), +} + +pub(crate) fn parse_text_face_and_options<'a>( + argv: &mut [&'a wstr], + is_builtin: bool, +) -> TextFaceArgsAndOptionsResult<'a> { + let builtin_extra_args = if is_builtin { 0 } else { "hc".len() }; + let short_options = L!(":b:oidruch"); + let short_options = &short_options[..short_options.len() - builtin_extra_args]; + let long_options: &[WOption] = &[ + wopt(L!("background"), ArgType::RequiredArgument, 'b'), + wopt(L!("bold"), ArgType::NoArgument, 'o'), + wopt(L!("underline"), ArgType::NoArgument, 'u'), + wopt(L!("italics"), ArgType::NoArgument, 'i'), + wopt(L!("dim"), ArgType::NoArgument, 'd'), + wopt(L!("reverse"), ArgType::NoArgument, 'r'), + wopt(L!("help"), ArgType::NoArgument, 'h'), + wopt(L!("print-colors"), ArgType::NoArgument, 'c'), + ]; + let long_options = &long_options[..long_options.len() - builtin_extra_args]; + + let mut bgcolor = None; + let mut style = TextStyling::empty(); + let mut print_color_mode = false; + + let mut w = WGetopter::new(short_options, long_options, argv); + while let Some(c) = w.next_opt() { + match c { + 'b' => { + assert!(w.woptarg.is_some(), "Arg should have been set"); + bgcolor = w.woptarg; + } + 'h' => { + if is_builtin { + return TextFaceArgsAndOptionsResult::PrintHelp; + } + } + 'o' => style |= TextStyling::BOLD, + 'i' => style |= TextStyling::ITALICS, + 'd' => style |= TextStyling::DIM, + 'r' => style |= TextStyling::REVERSE, + 'u' => style |= TextStyling::UNDERLINE, + 'c' => print_color_mode = true, + ':' => { + if is_builtin { + return TextFaceArgsAndOptionsResult::InvalidArgs; + } + } + '?' => { + if is_builtin { + return TextFaceArgsAndOptionsResult::UnknownOption(w.wopt_index - 1); + } + } + _ => unreachable!("unexpected retval from WGetopter"), + } + } + TextFaceArgsAndOptionsResult::Ok(TextFaceArgsAndOptions { + wopt_index: w.wopt_index, + bgcolor, + style, + print_color_mode, + }) +}