builtin set_color: reuse outputter

The differences between color variables and set_color implementation have
gotten somewhat small so make them explicit by getting rid of this code clone.

Outputter::set_text_face() has clever caching logic that is not always needed
by builtin set_color -- in fact, it even needs to explicitly disable the
cache for foreground and background colros -- but this might still be worth it.

src/builtins/set_color.rs

@@ -3,8 +3,8 @@
 use super::prelude::*;
 use crate::color::Color;
 use crate::common::str2wcstring;
-use crate::terminal::TerminalCommand::{DefaultBackgroundColor, DefaultUnderlineColor};
-use crate::terminal::{Output, Outputter, Paintable};
+use crate::screen::is_dumb;
+use crate::terminal::{use_terminfo, Outputter};
 use crate::text_face::{
     self, parse_text_face_and_options, PrintColorsArgs, SpecifiedTextFace, TextFace, TextStyling,
 };
@@ -112,46 +112,27 @@ pub fn set_color(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -
             }
         };
 
-    let mut outp = Outputter::new_buffering();
+    let mut outp = Outputter::new_buffering_no_assume_normal();
 
-    let fg = specified_face.fg;
-    let bg = specified_face.bg;
-    let underline_color = specified_face.underline_color;
-    let style = specified_face.style;
-
-    // Here's some automagic behavior: if either of foreground or background are "normal",
-    // reset all colors/attributes. Same if foreground is "reset" (undocumented).
-    if is_reset || fg.is_some_and(|fg| fg.is_normal()) {
+    // Note that for historical reasons "set_color normal" reset all colors/attributes. So does
+    // "set_color reset" (undocumented).
+    if is_reset {
         outp.reset_text_face();
     }
 
-    // Historically we have not used set_text_face() for colors here.
-    // Doing so would add two magic behaviors:
-    // - if fg and bg are equal, it makes one of them white
-    // - if bg is not normal, it makes the foreground bold
-    // The first one seems fine but the second one not really.
-    outp.set_text_face(TextFace::new(Color::None, Color::None, Color::None, style));
-    if let Some(fg) = fg {
-        if !fg.is_normal() && !outp.write_color(Paintable::Foreground, fg) {
-            // We need to do *something* or the lack of any output messes up
-            // when the cartesian product here would make "foo" disappear:
-            //  $ echo (set_color foo)bar
-            outp.reset_text_face();
-        }
-    }
-    if let Some(bg) = bg {
-        if bg.is_normal() {
-            outp.write_command(DefaultBackgroundColor);
-        } else {
-            outp.write_color(Paintable::Background, bg);
-        }
-    }
-    if let Some(underline_color) = underline_color {
-        if underline_color.is_normal() {
-            outp.write_command(DefaultUnderlineColor);
-        } else {
-            outp.write_color(Paintable::Underline, underline_color);
-        }
+    outp.set_text_face_no_magic(TextFace::new(
+        specified_face.fg.unwrap_or(Color::None),
+        specified_face.bg.unwrap_or(Color::None),
+        specified_face.underline_color.unwrap_or(Color::None),
+        specified_face.style,
+    ));
+
+    if specified_face.fg.is_some() && outp.contents().is_empty() {
+        assert!(is_dumb() || use_terminfo());
+        // We need to do *something* or the lack of any output messes up
+        // when the cartesian product here would make "foo" disappear:
+        //  $ echo (set_color foo)bar
+        outp.reset_text_face();
     }
 
     // Output the collected string.

src/terminal.rs

@@ -444,6 +444,14 @@ pub fn new_buffering() -> Self {
         Self::new_from_fd(-1)
     }
 
+    pub fn new_buffering_no_assume_normal() -> Self {
+        let mut zelf = Self::new_buffering();
+        zelf.last.fg = Color::None;
+        zelf.last.bg = Color::None;
+        assert_eq!(zelf.last.underline_color, Color::None);
+        zelf
+    }
+
     fn maybe_flush(&mut self) {
         if self.fd >= 0 && self.buffer_count == 0 {
             self.flush_to(self.fd);
@@ -494,8 +502,14 @@ pub(crate) fn reset_text_face(&mut self) {
     ///
     /// - Lastly we may need to write set_a_background or set_a_foreground to set the other half of the
     /// color pair to what it should be.
-    #[allow(clippy::if_same_then_else)]
     pub(crate) fn set_text_face(&mut self, face: TextFace) {
+        self.set_text_face_internal(face, true)
+    }
+    pub(crate) fn set_text_face_no_magic(&mut self, face: TextFace) {
+        self.set_text_face_internal(face, false)
+    }
+    #[allow(clippy::if_same_then_else)]
+    fn set_text_face_internal(&mut self, face: TextFace, salvage_unreadable: bool) {
         let mut fg = face.fg;
         let bg = face.bg;
         let underline_color = face.underline_color;
@@ -519,12 +533,14 @@ pub(crate) fn set_text_face(&mut self, face: TextFace) {
             // Only way to exit non-resettable ones is a reset of all attributes.
             self.reset_text_face();
         }
-        if !bg.is_special() && fg == bg {
-            fg = if bg == Color::WHITE {
-                Color::BLACK
-            } else {
-                Color::WHITE
-            };
+        if salvage_unreadable {
+            if !bg.is_special() && fg == bg {
+                fg = if bg == Color::WHITE {
+                    Color::BLACK
+                } else {
+                    Color::WHITE
+                };
+            }
         }
 
         if !fg.is_none() && fg != self.last.fg {

tests/checks/set_color.fish

@@ -14,9 +14,9 @@ string escape (set_color --background=reset)
 # CHECKERR: set_color: Unknown color 'reset'
 
 string escape (set_color --bold red --background=normal)
-# CHECK: \e\[1m\e\[31m\e\[49m
+# CHECK: \e\[31m\e\[49m\e\[1m
 string escape (set_color --bold red --background=blue)
-# CHECK: \e\[1m\e\[31m\e\[44m
+# CHECK: \e\[31m\e\[44m\e\[1m
 
 string escape (set_color --background=f00 --background=green --background=00f)
 # CHECK: \e\[48\;2\;255\;0\;0m

tests/checks/string.fish

@@ -1045,12 +1045,12 @@ end
 string shorten -m6 (set_color blue)s(set_color red)t(set_color --bold brwhite)rin(set_color red)g(set_color yellow)-shorten | string escape
 # Renders like "strin…" in colors
 # Note that red sequence that we still pass on because it's width 0.
-# CHECK: \e\[34ms\e\[31mt\e\[1m\e\[97mrin\e\[31m…
+# CHECK: \e\[34ms\e\[31mt\e\[97m\e\[1mrin\e\[31m…
 
 # See that colors aren't counted in ellipsis
 string shorten -c (set_color blue)s(set_color red)t(set_color --bold brwhite)rin(set_color red)g -m 8 abcdefghijklmno | string escape
 # Renders like "abstring" in colors
-# CHECK: ab\e\[34ms\e\[31mt\e\[1m\e\[97mrin\e\[31mg
+# CHECK: ab\e\[34ms\e\[31mt\e\[97m\e\[1mrin\e\[31mg
 
 set -l str (set_color blue)s(set_color red)t(set_color --bold brwhite)rin(set_color red)g(set_color yellow)-shorten
 for i in (seq 1 (string length -V -- $str))