From 7fb8ad6fa1ab4c53818e8c20396fe8d110cf4446 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Fri, 17 Jan 2020 13:33:55 -0500 Subject: [PATCH] Cleaned up command macros and StyledContent (#369) --- CHANGELOG.md | 9 +- src/macros.rs | 295 ++++++++++++++++++++++++++++-------- src/style/content_style.rs | 25 ++- src/style/styled_content.rs | 50 ++++-- 4 files changed, 292 insertions(+), 87 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29815c5..2311df3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/macros.rs b/src/macros.rs index c2a1539..f7fccd2 100644 --- a/src/macros.rs +++ b/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(); - } - )* - - error - }} + ($writer:expr $(, $command:expr)* $(,)? ) => { + // Queue each command, then flush + $crate::queue!($writer $(, $command)*).and_then(|()| { + $writer.flush().map_err($crate::ErrorKind::IoError) + }) + } } #[doc(hidden)] @@ -206,36 +180,231 @@ 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, + } - use crate::command::Command; - #[cfg(windows)] - use crate::error::ErrorKind; - - pub struct FakeCommand; - - impl Command for FakeCommand { - type AnsiType = &'static str; - - fn ansi_code(&self) -> Self::AnsiType { - "" + impl io::Write for FakeWrite { + fn write(&mut self, content: &[u8]) -> io::Result { + 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()) } - #[cfg(windows)] - fn execute_winapi(&self) -> Result<(), ErrorKind> { + fn flush(&mut self) -> io::Result<()> { + self.flushed = true; Ok(()) } } - #[test] - fn test_queue() { - assert!(queue!(stdout(), FakeCommand,).is_ok()); - assert!(queue!(stdout(), FakeCommand).is_ok()); + #[cfg(not(windows))] + mod unix { + use std::io::Write; + + use super::FakeWrite; + use crate::command::Command; + + pub struct FakeCommand; + + impl Command for FakeCommand { + 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); + } } - #[test] - fn test_execute() { - assert!(execute!(stdout(), FakeCommand,).is_ok()); - assert!(execute!(stdout(), FakeCommand).is_ok()); + #[cfg(windows)] + 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(()) + } + } + + // 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( + 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_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")) + }) + } } } diff --git a/src/style/content_style.rs b/src/style/content_style.rs index 80a5b51..21dde1b 100644 --- a/src/style/content_style.rs +++ b/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(&self, val: D) -> StyledContent { - StyledContent::new(self.clone(), val) + #[inline] + pub fn apply(self, val: D) -> StyledContent { + 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 diff --git a/src/style/styled_content.rs b/src/style/styled_content.rs index 76f5b0c..abe9473 100644 --- a/src/style/styled_content.rs +++ b/src/style/styled_content.rs @@ -28,51 +28,70 @@ use crate::{ /// println!("{}", styled); /// ``` #[derive(Clone)] -pub struct StyledContent { +pub struct StyledContent { /// The style (colors, content attributes). style: ContentStyle, /// A content to apply the style on. content: D, } -impl<'a, D: Display + 'a + Clone> StyledContent { +impl StyledContent { /// Creates a new `StyledContent`. + #[inline] pub fn new(style: ContentStyle, content: D) -> StyledContent { StyledContent { style, content } } /// Sets the foreground color. - pub fn with(mut self, foreground_color: Color) -> StyledContent { - self.style = self.style.foreground(foreground_color); - self + #[inline] + pub fn with(self, foreground_color: Color) -> StyledContent { + Self { + style: self.style.foreground(foreground_color), + ..self + } } /// Sets the background color. - pub fn on(mut self, background_color: Color) -> StyledContent { - self.style = self.style.background(background_color); - self + #[inline] + pub fn on(self, background_color: Color) -> StyledContent { + 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 { - self.style = self.style.attribute(attr); - self + #[inline] + pub fn attribute(self, attr: Attribute) -> StyledContent { + 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 Display for StyledContent { +impl Display for StyledContent { fn fmt(&self, f: &mut Formatter<'_>) -> result::Result<(), fmt::Error> { let mut reset = false; @@ -85,13 +104,16 @@ impl Display for StyledContent { 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)?; }