Cleaned up command macros and StyledContent (#369)

CHANGELOG.md

@@ -1,6 +1,11 @@
 # master
-- Added a generic implementation `Command` for `&T: Command`. This allows
-  commands to be queued by reference, as well as by value.
+- Added a generic implementation `Command` for `&T: Command`. This allows commands to be queued by reference, as well as by value.
+- Removed unnecessary trait bounds from StyledContent.
+- Added `StyledContent::style_mut`.
+- `execute!` and `queue!` now correctly handle errors during writing.
+- Fixed minor syntax bug in `execute!` and `queue!`.
+- Cleaned up implementation of `execute!` and `queue!`.
+- **breaking change** Changed `ContentStyle::apply` to take self by value instead of reference, to prevent an unnecessary extra clone.
 
 # Version 0.14.2
 - Fix TIOCGWINSZ for FreeBSD

src/macros.rs

@@ -10,13 +10,10 @@ macro_rules! csi {
 #[macro_export]
 macro_rules! write_ansi_code {
     ($writer:expr, $ansi_code:expr) => {{
-        use std::{
-            error::Error,
-            io::{self, ErrorKind},
-        };
+        use std::io::{self, ErrorKind};
 
         write!($writer, "{}", $ansi_code)
-            .map_err(|e| io::Error::new(ErrorKind::Other, e.description()))
+            .map_err(|e| io::Error::new(ErrorKind::Other, e))
             .map_err($crate::ErrorKind::IoError)
     }};
 }
@@ -33,11 +30,8 @@ macro_rules! handle_command {
         #[cfg(windows)]
         {
             let command = $command;
-            // ansi code is not always a string, however it does implement `Display` and `Write`.
-            // In order to check if the code is supported we have to make it a string.
-            let ansi_code = format!("{}", command.ansi_code());
             if command.is_ansi_code_supported() {
-                write_ansi_code!($writer, &ansi_code)
+                write_ansi_code!($writer, command.ansi_code())
             } else {
                 command.execute_winapi().map_err($crate::ErrorKind::from)
             }
@@ -51,7 +45,8 @@ macro_rules! handle_command {
 
 /// Queues one or more command(s) for further execution.
 ///
-/// Queued commands will be executed in the following cases:
+/// Queued commands must be flushed to the underlying device to be executed.
+/// This generally happens in the following cases:
 ///
 /// * When `flush` is called manually on the given type implementing `io::Write`.
 /// * The terminal will `flush` automatically if the buffer is full.
@@ -101,20 +96,11 @@ macro_rules! handle_command {
 ///
 #[macro_export]
 macro_rules! queue {
-    ($writer:expr, $($command:expr), * $(,)?) => {{
-        // Silent warning when the macro is used inside the `command` module
-        #[allow(unused_imports)]
-        use $crate::{Command, handle_command};
-
-        #[allow(unused_assignments)]
-        let mut error = Ok(());
-
-        $(
-            error = handle_command!($writer, $command);
+    ($writer:expr $(, $command:expr)* $(,)?) => {
+        Ok(()) $(
+            .and_then(|()| $crate::handle_command!($writer, $command))
         )*
-
-        error
-    }}
+    }
 }
 
 /// Executes one or more command(s).
@@ -160,24 +146,12 @@ macro_rules! queue {
 ///     and [queue](macro.queue.html) for those old Windows versions.
 #[macro_export]
 macro_rules! execute {
-    ($write:expr, $($command:expr), * $(,)? ) => {{
-        // Silent warning when the macro is used inside the `command` module
-        #[allow(unused_imports)]
-        use $crate::{handle_command, Command};
-
-        #[allow(unused_assignments)]
-        let mut error = Ok(());
-
-        $(
-            if let Err(e) = handle_command!($write, $command) {
-                error = Err(e);
-            }else {
-                $write.flush().map_err($crate::ErrorKind::IoError).unwrap();
+    ($writer:expr $(, $command:expr)* $(,)? ) => {
+        // Queue each command, then flush
+        $crate::queue!($writer $(, $command)*).and_then(|()| {
+            $writer.flush().map_err($crate::ErrorKind::IoError)
+        })
     }
-        )*
-
-        error
-    }}
 }
 
 #[doc(hidden)]
@@ -206,11 +180,36 @@ macro_rules! impl_from {
 
 #[cfg(test)]
 mod tests {
-    use std::io::{stdout, Write};
+    use std::io;
+    use std::str;
+    // Helper for execute tests to confirm flush
+    #[derive(Default, Debug, Clone)]
+    pub(self) struct FakeWrite {
+        buffer: String,
+        flushed: bool,
+    }
 
+    impl io::Write for FakeWrite {
+        fn write(&mut self, content: &[u8]) -> io::Result<usize> {
+            let content = str::from_utf8(content)
+                .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?;
+            self.buffer.push_str(content);
+            self.flushed = false;
+            Ok(content.len())
+        }
+
+        fn flush(&mut self) -> io::Result<()> {
+            self.flushed = true;
+            Ok(())
+        }
+    }
+
+    #[cfg(not(windows))]
+    mod unix {
+        use std::io::Write;
+
+        use super::FakeWrite;
         use crate::command::Command;
-    #[cfg(windows)]
-    use crate::error::ErrorKind;
 
         pub struct FakeCommand;
 
@@ -218,24 +217,194 @@ mod tests {
             type AnsiType = &'static str;
 
             fn ansi_code(&self) -> Self::AnsiType {
-            ""
+                "cmd"
+            }
+        }
+
+        #[test]
+        fn test_queue_one() {
+            let mut result = FakeWrite::default();
+            queue!(&mut result, FakeCommand).unwrap();
+            assert_eq!(&result.buffer, "cmd");
+            assert!(!result.flushed);
+        }
+
+        #[test]
+        fn test_queue_many() {
+            let mut result = FakeWrite::default();
+            queue!(&mut result, FakeCommand, FakeCommand).unwrap();
+            assert_eq!(&result.buffer, "cmdcmd");
+            assert!(!result.flushed);
+        }
+
+        #[test]
+        fn test_queue_trailing_comma() {
+            let mut result = FakeWrite::default();
+            queue!(&mut result, FakeCommand, FakeCommand,).unwrap();
+            assert_eq!(&result.buffer, "cmdcmd");
+            assert!(!result.flushed);
+        }
+
+        #[test]
+        fn test_execute_one() {
+            let mut result = FakeWrite::default();
+            execute!(&mut result, FakeCommand).unwrap();
+            assert_eq!(&result.buffer, "cmd");
+            assert!(result.flushed);
+        }
+
+        #[test]
+        fn test_execute_many() {
+            let mut result = FakeWrite::default();
+            execute!(&mut result, FakeCommand, FakeCommand).unwrap();
+            assert_eq!(&result.buffer, "cmdcmd");
+            assert!(result.flushed);
+        }
+
+        #[test]
+        fn test_execute_trailing_comma() {
+            let mut result = FakeWrite::default();
+            execute!(&mut result, FakeCommand, FakeCommand,).unwrap();
+            assert_eq!(&result.buffer, "cmdcmd");
+            assert!(result.flushed);
+        }
     }
 
     #[cfg(windows)]
-        fn execute_winapi(&self) -> Result<(), ErrorKind> {
+    mod windows {
+        use std::cell::RefCell;
+        use std::fmt::Debug;
+        use std::io::Write;
+
+        use super::FakeWrite;
+        use crate::command::Command;
+        use crate::error::Result as CrosstermResult;
+
+        // We need to test two different APIs: winapi and the write api. We
+        // don't know until runtime which we're supporting (via
+        // Command::is_ansi_code_supported), so we have to test them both. The
+        // CI environment hopefully includes both versions of windows.
+
+        // WindowsEventStream is a place for execute_winapi to push strings,
+        // when called.
+        type WindowsEventStream = Vec<&'static str>;
+
+        struct FakeCommand<'a> {
+            // Need to use a refcell because we want execute_winapi to be able
+            // push to the vector, but execute_winapi take &self.
+            stream: RefCell<&'a mut WindowsEventStream>,
+            value: &'static str,
+        }
+
+        impl<'a> FakeCommand<'a> {
+            fn new(stream: &'a mut WindowsEventStream, value: &'static str) -> Self {
+                Self {
+                    value,
+                    stream: RefCell::new(stream),
+                }
+            }
+        }
+
+        impl<'a> Command for FakeCommand<'a> {
+            type AnsiType = &'static str;
+
+            fn ansi_code(&self) -> Self::AnsiType {
+                self.value
+            }
+
+            fn execute_winapi(&self) -> CrosstermResult<()> {
+                self.stream.borrow_mut().push(self.value);
                 Ok(())
             }
         }
 
-    #[test]
-    fn test_queue() {
-        assert!(queue!(stdout(), FakeCommand,).is_ok());
-        assert!(queue!(stdout(), FakeCommand).is_ok());
+        // Helper function for running tests against either winapi or an
+        // io::Write.
+        //
+        // This function will execute the `test` function, which should
+        // queue some commands against the given FakeWrite and
+        // WindowsEventStream. It will then test that the correct data sink
+        // was populated. It does not currently check is_ansi_code_supported;
+        // for now it simply checks that one of the two streams was correctly
+        // populated.
+        //
+        // If the stream was populated, it tests that the two arrays are equal.
+        // If the writer was populated, it tests that the contents of the
+        // write buffer are equal to the concatenation of `stream_result`.
+        fn test_harness<E: Debug>(
+            stream_result: &[&'static str],
+            test: impl FnOnce(&mut FakeWrite, &mut WindowsEventStream) -> Result<(), E>,
+        ) {
+            let mut stream = WindowsEventStream::default();
+            let mut writer = FakeWrite::default();
+
+            if let Err(err) = test(&mut writer, &mut stream) {
+                panic!("Error returned from test function: {:?}", err);
+            }
+
+            // We need this for type inference, for whatever reason.
+            const EMPTY_RESULT: [&'static str; 0] = [];
+
+            // TODO: confirm that the correct sink was used, based on
+            // is_ansi_code_supported
+            match (writer.buffer.is_empty(), stream.is_empty()) {
+                (true, true) if stream_result == &EMPTY_RESULT => {}
+                (true, true) => panic!(
+                    "Neither the event stream nor the writer were populated. Expected {:?}",
+                    stream_result
+                ),
+
+                // writer is populated
+                (false, true) => {
+                    // Concat the stream result to find the string result
+                    let result: String = stream_result.iter().copied().collect();
+                    assert_eq!(result, writer.buffer);
+                    assert_eq!(&stream, &EMPTY_RESULT);
+                }
+
+                // stream is populated
+                (true, false) => {
+                    assert_eq!(stream, stream_result);
+                    assert_eq!(writer.buffer, "");
+                }
+
+                // Both are populated
+                (false, false) => panic!(
+                    "Both the writer and the event stream were written to.\n\
+                     Only one should be used, based on is_ansi_code_supported.\n\
+                     stream: {stream:?}\n\
+                     writer: {writer:?}",
+                    stream = stream,
+                    writer = writer,
+                ),
+            }
         }
 
         #[test]
-    fn test_execute() {
-        assert!(execute!(stdout(), FakeCommand,).is_ok());
-        assert!(execute!(stdout(), FakeCommand).is_ok());
+        fn test_queue_one() {
+            test_harness(&["cmd1"], |writer, stream| {
+                queue!(writer, FakeCommand::new(stream, "cmd1"))
+            })
+        }
+
+        #[test]
+        fn test_queue_some() {
+            test_harness(&["cmd1", "cmd2"], |writer, stream| {
+                queue!(
+                    writer,
+                    FakeCommand::new(stream, "cmd1"),
+                    FakeCommand::new(stream, "cmd2"),
+                )
+            })
+        }
+
+        #[test]
+        fn test_many_queues() {
+            test_harness(&["cmd1", "cmd2", "cmd3"], |writer, stream| {
+                queue!(writer, FakeCommand::new(stream, "cmd1"))?;
+                queue!(writer, FakeCommand::new(stream, "cmd2"))?;
+                queue!(writer, FakeCommand::new(stream, "cmd3"))
+            })
+        }
     }
 }

src/style/content_style.rs

@@ -17,30 +17,39 @@ pub struct ContentStyle {
 
 impl ContentStyle {
     /// Creates a `StyledContent` by applying the style to the given `val`.
-    pub fn apply<D: Display + Clone>(&self, val: D) -> StyledContent<D> {
-        StyledContent::new(self.clone(), val)
+    #[inline]
+    pub fn apply<D: Display>(self, val: D) -> StyledContent<D> {
+        StyledContent::new(self, val)
     }
 
     /// Creates a new `ContentStyle`.
+    #[inline]
     pub fn new() -> ContentStyle {
         ContentStyle::default()
     }
 
     /// Sets the background color.
-    pub fn background(mut self, color: Color) -> ContentStyle {
-        self.background_color = Some(color);
-        self
+    #[inline]
+    pub fn background(self, color: Color) -> ContentStyle {
+        Self {
+            background_color: Some(color),
+            ..self
+        }
     }
 
     /// Sets the foreground color.
-    pub fn foreground(mut self, color: Color) -> ContentStyle {
-        self.foreground_color = Some(color);
-        self
+    #[inline]
+    pub fn foreground(self, color: Color) -> ContentStyle {
+        Self {
+            foreground_color: Some(color),
+            ..self
+        }
     }
 
     /// Adds the attribute.
     ///
     /// You can add more attributes by calling this method multiple times.
+    #[inline]
     pub fn attribute(mut self, attr: Attribute) -> ContentStyle {
         self.attributes.push(attr);
         self

src/style/styled_content.rs

@@ -28,51 +28,70 @@ use crate::{
 /// println!("{}", styled);
 /// ```
 #[derive(Clone)]
-pub struct StyledContent<D: Display + Clone> {
+pub struct StyledContent<D: Display> {
     /// The style (colors, content attributes).
     style: ContentStyle,
     /// A content to apply the style on.
     content: D,
 }
 
-impl<'a, D: Display + 'a + Clone> StyledContent<D> {
+impl<D: Display> StyledContent<D> {
     /// Creates a new `StyledContent`.
+    #[inline]
     pub fn new(style: ContentStyle, content: D) -> StyledContent<D> {
         StyledContent { style, content }
     }
 
     /// Sets the foreground color.
-    pub fn with(mut self, foreground_color: Color) -> StyledContent<D> {
-        self.style = self.style.foreground(foreground_color);
-        self
+    #[inline]
+    pub fn with(self, foreground_color: Color) -> StyledContent<D> {
+        Self {
+            style: self.style.foreground(foreground_color),
+            ..self
+        }
     }
 
     /// Sets the background color.
-    pub fn on(mut self, background_color: Color) -> StyledContent<D> {
-        self.style = self.style.background(background_color);
-        self
+    #[inline]
+    pub fn on(self, background_color: Color) -> StyledContent<D> {
+        Self {
+            style: self.style.background(background_color),
+            ..self
+        }
     }
 
     /// Adds the attribute.
     ///
     /// You can add more attributes by calling this method multiple times.
-    pub fn attribute(mut self, attr: Attribute) -> StyledContent<D> {
-        self.style = self.style.attribute(attr);
-        self
+    #[inline]
+    pub fn attribute(self, attr: Attribute) -> StyledContent<D> {
+        Self {
+            style: self.style.attribute(attr),
+            ..self
+        }
     }
 
     /// Returns the content.
+    #[inline]
     pub fn content(&self) -> &D {
         &self.content
     }
 
     /// Returns the style.
+    #[inline]
     pub fn style(&self) -> &ContentStyle {
         &self.style
     }
+
+    /// Returns a mutable reference to the style, so that it can be futher
+    /// manipulated
+    #[inline]
+    pub fn style_mut(&mut self) -> &mut ContentStyle {
+        &mut self.style
+    }
 }
 
-impl<D: Display + Clone> Display for StyledContent<D> {
+impl<D: Display> Display for StyledContent<D> {
     fn fmt(&self, f: &mut Formatter<'_>) -> result::Result<(), fmt::Error> {
         let mut reset = false;
 
@@ -85,13 +104,16 @@ impl<D: Display + Clone> Display for StyledContent<D> {
             reset = true;
         }
 
-        for attr in self.style.attributes.iter() {
+        for attr in &self.style.attributes {
             queue!(f, SetAttribute(*attr)).map_err(|_| fmt::Error)?;
             reset = true;
         }
 
-        fmt::Display::fmt(&self.content, f)?;
+        self.content.fmt(f)?;
 
+        // TODO: There are specific command sequences for "reset forground
+        // color (39m)" and "reset background color (49m)"; consider using
+        // these.
         if reset {
             queue!(f, ResetColor).map_err(|_| fmt::Error)?;
         }