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

@@ -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 TextFaceArgsAndOptions {
-    let mut style = TextStyling::empty();
+        mut wopt_index,
-    let mut print = false;
+        bgcolor,
-
+        style,
-    let mut w = WGetopter::new(SHORT_OPTIONS, LONG_OPTIONS, argv);
+        print_color_mode,
-    while let Some(c) = w.next_opt() {
+    } = match parse_text_face_and_options(argv, /*is_builtin=*/ true) {
-        match c {
+        TextFaceArgsAndOptionsResult::Ok(parsed_face) => parsed_face,
-            'b' => {
+        TextFaceArgsAndOptionsResult::PrintHelp => {
-                assert!(w.woptarg.is_some(), "Arg should have been set");
+            builtin_print_help(parser, streams, argv[0]);
-                bgcolor = w.woptarg;
+            return Ok(SUCCESS);
-            }
-            '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"),
         }
-    }
+        TextFaceArgsAndOptionsResult::InvalidArgs => {
-    // We want to reclaim argv so grab wopt_index now.
+            // We don't error here because "-b" is the only option that requires an argument,
-    let mut wopt_index = w.wopt_index;
+            // 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
+    // #1323: We may have multiple foreground colors. Choose the best one.
-    // color, we'll get none(); if we have at least one we expect not-none.
+    let fg = best_color(fgcolors.into_iter(), get_color_support());
-    let fg = best_color(&fgcolors, get_color_support());
-    assert!(fgcolors.is_empty() || !fg.is_none());
 
     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 */) {

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);
+        let (color, style) = parse_text_face(var.as_list(), is_background);
-        if color.is_none() {
+        let color = color.unwrap_or(Color::Normal);
-            color = Color::Normal;
-        }
         (color, style)
     };
     let (fg, fg_style) = parse_var(fg_var, false);

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 {
+pub fn best_color(candidates: impl Iterator<Item = Color>, support: ColorSupport) -> Option<Color> {
-    if candidates.is_empty() {
+    let mut first = None;
-        return Color::None;
+    let mut first_rgb = None;
-    }
+    let mut first_named = None;
-
-    let mut first_rgb = Color::None;
-    let mut first_named = Color::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() {
+    (if (first_rgb.is_some() && has_term256) || first_named.is_none() {
-        result = first_rgb;
+        first_rgb
     } else {
-        result = first_named;
+        first_named
-    }
+    })
-    if result.is_none() {
+    .or(first)
-        result = candidates[0];
-    }
-    result
 }
 
 /// The [`Term`] singleton. Initialized via a call to [`setup()`] and surfaced to the outside world via [`term()`].

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) {
+pub fn parse_text_face(arguments: &[WString], is_background: bool) -> (Option<Color>, TextStyling) {
-    let mut style = TextStyling::empty();
+    let mut argv: Vec<&wstr> = Some(L!(""))
-    let mut candidates: Vec<Color> = Vec::new();
+        .into_iter()
-
+        .chain(arguments.iter().map(|s| s.as_utfstr()))
-    let prefix = L!("--background=");
+        .collect();
-
+    let TextFaceArgsAndOptions {
-    let mut next_is_background = false;
+        wopt_index,
-    for arg in arguments {
+        bgcolor,
-        let mut color_name = None;
+        mut style,
-        #[allow(clippy::collapsible_else_if)]
+        print_color_mode,
-        if is_background {
+    } = match parse_text_face_and_options(&mut argv, /*is_builtin=*/ false) {
-            if next_is_background {
+        TextFaceArgsAndOptionsResult::Ok(parsed_text_faces) => parsed_text_faces,
-                color_name = Some(arg.as_utfstr());
+        TextFaceArgsAndOptionsResult::PrintHelp
-                next_is_background = false;
+        | TextFaceArgsAndOptionsResult::InvalidArgs
-            } else if arg.starts_with(prefix) {
+        | TextFaceArgsAndOptionsResult::UnknownOption(_) => unreachable!(),
-                // Look for something like "--background=red".
+    };
-                color_name = Some(arg.slice_from(prefix.char_count()));
+    assert!(!print_color_mode);
-            } else if arg == "--background" || arg == "-b" {
+    let color;
-                // Without argument attached the next token is the color
+    if is_background {
-                // - if it's another option it's an error.
+        color = bgcolor.and_then(Color::from_wstr);
-                next_is_background = true;
+        // We ignore styles for dedicated background roles but reverse should be meaningful in
-            } else if arg == "--reverse" || arg == "-r" {
+        // either context
-                // Reverse should be meaningful in either context
+        style &= TextStyling::REVERSE;
-                style |= TextStyling::REVERSE;
+    } else {
-            } else if arg.starts_with("-b") {
+        color = best_color(
-                // Look for something like "-bred".
+            argv[wopt_index..]
-                // Yes, that length is hardcoded.
+                .iter()
-                color_name = Some(arg.slice_from(2));
+                .filter_map(|&color| Color::from_wstr(color)),
-            }
+            get_color_support(),
-        } 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);
-        }
     }
-
+    assert!(color.map_or(true, |color| !color.is_none()));
-    let color = best_color(&candidates, get_color_support());
     (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,
+    })
+}