Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Cleanup 2 #238

Merged
merged 30 commits into from
Sep 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
ecbb7d0
Move test to a separate module
zrzka Sep 19, 2019
621fa7e
Return proper source of the error
zrzka Sep 19, 2019
58ae934
Make it easier to add another errors
zrzka Sep 19, 2019
3884a20
Remove one return
zrzka Sep 19, 2019
12689a8
Add missing import
zrzka Sep 19, 2019
5e128d4
Remove redundant implementations
zrzka Sep 19, 2019
178f7a4
Add tests
zrzka Sep 19, 2019
a7bca56
Remove unwrap
zrzka Sep 19, 2019
5b38591
Remove Result & simplify
zrzka Sep 19, 2019
75515fe
Remove unnecessary Clone constraint
zrzka Sep 19, 2019
b079067
Reformat & cleanup
zrzka Sep 19, 2019
160cb4d
Derive Default & simplify
zrzka Sep 19, 2019
cab816c
Remove unnecessary Clone constraint
zrzka Sep 19, 2019
e1e4907
Remove unnecessary Clone constraint
zrzka Sep 19, 2019
9618b12
Reenable & fix tests
zrzka Sep 19, 2019
9139026
Replace unwrap() calls
zrzka Sep 19, 2019
e7a028a
Replace unwrap() calls
zrzka Sep 19, 2019
46eee1a
Use if let
zrzka Sep 19, 2019
9531711
unwrap -> wrap_with_result & cleanup
zrzka Sep 19, 2019
b29cfb8
Fix test modules name
zrzka Sep 19, 2019
79c6219
Empty line between #[inline] fns
zrzka Sep 19, 2019
4198401
Replace #[inline] with #[inline(always)]
zrzka Sep 19, 2019
92ee226
AsyncReader::stop_reading() -> stop()
zrzka Sep 19, 2019
a606653
Better fn name & docs
zrzka Sep 19, 2019
2b1ed31
Replace expect()
zrzka Sep 19, 2019
00b3a17
Put the Clone back for clarity
zrzka Sep 19, 2019
340f1ba
Put the Clone back for clarity
zrzka Sep 19, 2019
744d5a9
Put back Clone
zrzka Sep 19, 2019
b906d24
Move the macro to macros.rs
zrzka Sep 19, 2019
ef79d5a
No more #[inline]s
zrzka Sep 19, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions crossterm_cursor/src/cursor/ansi_cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,22 @@ use crate::sys::{get_cursor_position, show_cursor};

use super::ITerminalCursor;

#[inline]
pub fn get_goto_ansi(x: u16, y: u16) -> String {
format!(csi!("{};{}H"), y + 1, x + 1)
}
#[inline]

pub fn get_move_up_ansi(count: u16) -> String {
format!(csi!("{}A"), count)
}
#[inline]

pub fn get_move_right_ansi(count: u16) -> String {
format!(csi!("{}C"), count)
}
#[inline]

pub fn get_move_down_ansi(count: u16) -> String {
format!(csi!("{}B"), count)
}
#[inline]

pub fn get_move_left_ansi(count: u16) -> String {
format!(csi!("{}D"), count)
}
Expand Down
2 changes: 1 addition & 1 deletion crossterm_cursor/src/cursor/cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ impl TerminalCursor {

/// Move the current cursor position `n` times left.
pub fn move_left(&mut self, count: u16) -> Result<&mut TerminalCursor> {
self.cursor.move_left(count).unwrap();
self.cursor.move_left(count)?;
Ok(self)
}

Expand Down
24 changes: 14 additions & 10 deletions crossterm_input/src/input/unix_input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ impl AsyncReader {
/// # Remarks
/// - Background thread will be closed.
/// - This will consume the handle you won't be able to restart the reading with this handle, create a new `AsyncReader` instead.
pub fn stop_reading(&mut self) {
pub fn stop(&mut self) {
self.shutdown.store(true, Ordering::SeqCst);
}
}
Expand Down Expand Up @@ -161,7 +161,7 @@ impl Iterator for AsyncReader {

impl Drop for AsyncReader {
fn drop(&mut self) {
self.stop_reading();
self.stop();
}
}

Expand Down Expand Up @@ -490,13 +490,17 @@ where
}

#[cfg(test)]
#[test]
fn test_parse_utf8() {
let st = "abcéŷ¤£€ù%323";
let ref mut bytes = st.bytes();
let chars = st.chars();
for c in chars {
let b = bytes.next().unwrap();
assert_eq!(c, parse_utf8_char(b, bytes).unwrap());
mod tests {
use super::parse_utf8_char;

#[test]
fn test_parse_utf8() {
let st = "abcéŷ¤£€ù%323";
let ref mut bytes = st.bytes();
let chars = st.chars();
for c in chars {
let b = bytes.next().unwrap();
assert_eq!(c, parse_utf8_char(b, bytes).unwrap());
}
}
}
4 changes: 2 additions & 2 deletions crossterm_input/src/input/windows_input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,14 +180,14 @@ impl AsyncReader {
/// # Remarks
/// - Background thread will be closed.
/// - This will consume the handle you won't be able to restart the reading with this handle, create a new `AsyncReader` instead.
pub fn stop_reading(&mut self) {
pub fn stop(&mut self) {
self.shutdown.store(true, Ordering::SeqCst);
}
}

impl Drop for AsyncReader {
fn drop(&mut self) {
self.stop_reading();
self.stop();
}
}

Expand Down
4 changes: 2 additions & 2 deletions crossterm_screen/src/screen/raw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ impl RawScreen {
Ok(())
}

/// This will disable the drop logic of this type, which means that the rawscreen will not be disabled when this instance goes out of scope.
pub fn disable_raw_mode_on_drop(&mut self) {
/// Keeps the raw mode when the `RawMode` value is dropped.
pub fn keep_raw_mode_on_drop(&mut self) {
zrzka marked this conversation as resolved.
Show resolved Hide resolved
self.disable_raw_mode_on_drop = false;
}
}
Expand Down
2 changes: 1 addition & 1 deletion crossterm_screen/src/sys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ impl ToAlternateScreenCommand {
impl IAlternateScreenCommand for ToAlternateScreenCommand {
/// enable alternate screen.
fn enable(&self) -> Result<()> {
write_cout!(csi!("?1049h")).unwrap();
write_cout!(csi!("?1049h"))?;
Ok(())
TimonPost marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down
3 changes: 0 additions & 3 deletions crossterm_style/src/ansi_color.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,14 @@ use crossterm_utils::{csi, write_cout, Result};

use crate::{Attribute, Color, Colored, ITerminalColor};

#[inline]
pub fn get_set_fg_ansi(fg_color: Color) -> String {
format!(csi!("{}m"), color_value(Colored::Fg(fg_color)),)
}

#[inline]
pub fn get_set_bg_ansi(bg_color: Color) -> String {
format!(csi!("{}m"), color_value(Colored::Bg(bg_color)),)
}

#[inline]
pub fn get_set_attr_ansi(attribute: Attribute) -> String {
format!(csi!("{}m"), attribute as i16,)
}
Expand Down
22 changes: 9 additions & 13 deletions crossterm_style/src/color.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
//! Like applying attributes to text and changing the foreground and background.

use std::clone::Clone;
use std::env;
use std::fmt::Display;

#[cfg(windows)]
Expand Down Expand Up @@ -65,19 +66,14 @@ impl TerminalColor {
}

/// Get available color count.
/// (This does not always provide a good result.)
pub fn get_available_color_count(&self) -> Result<u16> {
use std::env;
Ok(match env::var_os("TERM") {
Some(val) => {
if val.to_str().unwrap_or("").contains("256color") {
256
} else {
8
}
}
None => 8,
})
///
/// # Remarks
///
/// This does not always provide a good result.
pub fn get_available_color_count(&self) -> u16 {
env::var("TERM")
.map(|x| if x.contains("256color") { 256 } else { 8 })
.unwrap_or(8)
}
}

Expand Down
51 changes: 36 additions & 15 deletions crossterm_style/src/enums/color.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,24 +46,15 @@ pub enum Color {
AnsiValue(u8),
}

impl<'a> From<&'a str> for Color {
/// Get a `Color` from a `&str` like `Color::from("blue")`.
fn from(src: &str) -> Self {
src.parse().unwrap_or(Color::White)
}
}

impl From<String> for Color {
/// Get a `Color` from a `&str` like `Color::from(String::from(blue))`.
fn from(src: String) -> Self {
src.parse().unwrap_or(Color::White)
}
}

impl FromStr for Color {
type Err = ();

/// Convert a `&str` to a `Color` value
/// Creates a `Color` from the string representation.
///
/// # Remarks
///
/// * `Color::White` is returned in case of an unknown color.
/// * This function does not return `Err` and you can safely unwrap.
fn from_str(src: &str) -> ::std::result::Result<Self, Self::Err> {
let src = src.to_lowercase();

Expand All @@ -88,3 +79,33 @@ impl FromStr for Color {
}
}
}

#[cfg(test)]
mod tests {
use super::Color;

#[test]
fn test_known_color_conversion() {
assert_eq!("black".parse(), Ok(Color::Black));
assert_eq!("dark_grey".parse(), Ok(Color::DarkGrey));
assert_eq!("red".parse(), Ok(Color::Red));
assert_eq!("dark_red".parse(), Ok(Color::DarkRed));
assert_eq!("green".parse(), Ok(Color::Green));
assert_eq!("dark_green".parse(), Ok(Color::DarkGreen));
assert_eq!("yellow".parse(), Ok(Color::Yellow));
assert_eq!("dark_yellow".parse(), Ok(Color::DarkYellow));
assert_eq!("blue".parse(), Ok(Color::Blue));
assert_eq!("dark_blue".parse(), Ok(Color::DarkBlue));
assert_eq!("magenta".parse(), Ok(Color::Magenta));
assert_eq!("dark_magenta".parse(), Ok(Color::DarkMagenta));
assert_eq!("cyan".parse(), Ok(Color::Cyan));
assert_eq!("dark_cyan".parse(), Ok(Color::DarkCyan));
assert_eq!("white".parse(), Ok(Color::White));
assert_eq!("grey".parse(), Ok(Color::Grey));
}

#[test]
fn test_unknown_color_conversion_yields_white() {
assert_eq!("foo".parse(), Ok(Color::White));
}
}
12 changes: 6 additions & 6 deletions crossterm_style/src/enums/colored.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@ impl Display for Colored {
let colored_terminal = color();

match *self {
Colored::Fg(color) => {
colored_terminal.set_fg(color).unwrap();
}
Colored::Bg(color) => {
colored_terminal.set_bg(color).unwrap();
}
Colored::Fg(color) => colored_terminal
.set_fg(color)
.map_err(|_| std::fmt::Error)?,
Colored::Bg(color) => colored_terminal
.set_bg(color)
.map_err(|_| std::fmt::Error)?,
}

Ok(())
Expand Down
26 changes: 12 additions & 14 deletions crossterm_style/src/macros.rs
Original file line number Diff line number Diff line change
@@ -1,34 +1,32 @@
macro_rules! def_attr {
($name: ident => $attr: path) => {
($name:ident => $attr:path) => {
fn $name(self) -> StyledObject<D> {
let so = self;

so.attr($attr)
self.attr($attr)
}
};
}

macro_rules! def_color {
($side:ident: $name: ident => $color: path) => {
($side:ident: $name:ident => $color:path) => {
fn $name(self) -> StyledObject<D> {
StyledObject {
object_style: ObjectStyle {
$side: Some($color),
.. self.object_style
..self.object_style
},
.. self
..self
}
}
};
}

macro_rules! def_str_color {
($side:ident: $name: ident => $color: path) => {
($side:ident: $name:ident => $color:path) => {
fn $name(self) -> StyledObject< &'static str> {
StyledObject {
object_style: ObjectStyle {
$side: Some($color),
.. ObjectStyle::default()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my opinion it is more clear if we use ObjectStyle, especially in macros where type inspection with idea is a pain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of disagree here. It's a standard pattern. See the documentation and an example:

fn main() {
    let options = SomeOptions { foo: 42, ..Default::default() };
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I can find my way in. In this case, we have a macro for which we don't have a type of inspection or highlighting for a lot of IDEs. ObjectStyle::default()is much more specific than Deafault::default()`, so someone who reads this macro can better understand what it says. So it's more about readability and being specific in macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The macro looks like this ...

StyledObject {
    object_style: ObjectStyle { // <-- here
        $side: Some($color),
        ..Default::default()
    },
    content: self
}

... and you have the ObjectStyle 2 lines above the ..Default::default(), which looks enough & fine to me.

There's also a similar case in the macro below (def_str_attr) ...

StyledObject {
    object_style: Default::default(),
    content: self,
}

... where Default::default() is used, but there's no ObjectStyle at all. Which is, from the readability point of view a bit worse than the first case.

I do not want to force my opinion here, both ways can be used, so, I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, merged while I was typing. I assume that there's no need to change it once it is already merged.

Copy link
Member

@TimonPost TimonPost Sep 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I am fine with it. Isn't that big of a deal. Otherwise, I wouldn't have merged it.

..Default::default()
},
content: self
}
Expand All @@ -37,12 +35,12 @@ macro_rules! def_str_color {
}

macro_rules! def_str_attr {
($name: ident => $color: path) => {
($name:ident => $color:path) => {
fn $name(self) -> StyledObject<&'static str> {
StyledObject {
object_style: ObjectStyle::default(),
content: self
}
StyledObject {
object_style: Default::default(),
content: self,
}
}
}
}
18 changes: 2 additions & 16 deletions crossterm_style/src/objectstyle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,13 @@ use std::fmt::Display;
use super::{Attribute, Color, StyledObject};

/// Struct that contains the style properties that can be applied to a displayable object.
#[derive(Debug, Clone)]
#[derive(Debug, Clone, Default)]
pub struct ObjectStyle {
pub fg_color: Option<Color>,
pub bg_color: Option<Color>,
pub attrs: Vec<Attribute>,
}

impl Default for ObjectStyle {
fn default() -> ObjectStyle {
ObjectStyle {
fg_color: None,
bg_color: None,
attrs: Vec::new(),
}
}
}

impl ObjectStyle {
/// Apply a `StyledObject` to the passed displayable object.
pub fn apply_to<D: Display + Clone>(&self, val: D) -> StyledObject<D> {
Expand All @@ -33,11 +23,7 @@ impl ObjectStyle {

/// Get a new instance of `ObjectStyle`
pub fn new() -> ObjectStyle {
ObjectStyle {
fg_color: None,
bg_color: None,
attrs: Vec::new(),
}
ObjectStyle::default()
}

/// Set the background color of `ObjectStyle` to the passed color.
Expand Down
6 changes: 3 additions & 3 deletions crossterm_style/src/styledobject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,11 @@ impl<D: Display + Clone> Display for StyledObject<D> {
let mut reset = false;

if let Some(bg) = self.object_style.bg_color {
queue!(f, SetBg(bg)).unwrap();
queue!(f, SetBg(bg)).map_err(|_| fmt::Error)?;
reset = true;
}
if let Some(fg) = self.object_style.fg_color {
queue!(f, SetFg(fg)).unwrap();
queue!(f, SetFg(fg)).map_err(|_| fmt::Error)?;
reset = true;
}

Expand All @@ -71,7 +71,7 @@ impl<D: Display + Clone> Display for StyledObject<D> {
fmt::Display::fmt(&self.content, f)?;

if reset {
colored_terminal.reset().unwrap();
colored_terminal.reset().map_err(|_| fmt::Error)?;
}

Ok(())
Expand Down
Loading