From c911b5f6e46ca82ee51c88a13a01c03ec342c4d0 Mon Sep 17 00:00:00 2001 From: Benjamin Sago Date: Thu, 17 Dec 2015 08:25:20 +0800 Subject: [PATCH 1/7] Replace Cells with growable TextCells A recent change to ansi-term [1] means that `ANSIString`s can now hold either owned *or* borrowed data (Rust calls this the Cow type). This means that we can delay formatting ANSIStrings into ANSI-control-code-formatted strings until it's absolutely necessary. The process for doing this was: 1. Replace the `Cell` type with a `TextCell` type that holds a vector of `ANSIString` values instead of a formatted string. It still does the width tracking. 2. Rework the details module's `render` functions to emit values of this type. 3. Similarly, rework the functions that produce cells containing filenames to use a `File` value's `name` field, which is an owned `String` that can now be re-used. 4. Update the printing, formatting, and width-calculating code in the details and grid-details views to produce a table by adding vectors together instead of adding strings together, delaying the formatting as long as it can. This results in fewer allocations (as fewer `String` values are produced), and makes the API tidier (as fewer `String` values are being passed around without having their contents specified). This also paves the way to Windows support, or at least support for non-ANSI terminals: by delaying the time until strings are formatted, it'll now be easier to change *how* they are formatted. Casualties include: - Bump to ansi_term v0.7.1, which impls `PartialEq` and `Debug` on `ANSIString`. - The grid_details and lines views now need to take a vector of files, rather than a borrowed slice, so the filename cells produced now own the filename strings that get taken from files. - Fixed the signature of `File#link_target` to specify that the file produced refers to the same directory, rather than some phantom directory with the same lifetime as the file. (This was wrong from the start, but it broke nothing until now) References: [1]: ansi-term@f6a6579ba8174de1cae64d181ec04af32ba2a4f0 --- Cargo.lock | 32 ++--- Cargo.toml | 2 +- src/file.rs | 2 +- src/main.rs | 4 +- src/output/cell.rs | 130 +++++++++++++++++++ src/output/column.rs | 38 ------ src/output/details.rs | 253 +++++++++++++++++++------------------ src/output/grid_details.rs | 30 +++-- src/output/lines.rs | 6 +- src/output/mod.rs | 37 ++++-- 10 files changed, 327 insertions(+), 207 deletions(-) create mode 100644 src/output/cell.rs diff --git a/Cargo.lock b/Cargo.lock index ceafcad..4b22ccf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2,13 +2,13 @@ name = "exa" version = "0.4.0" dependencies = [ - "ansi_term 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)", + "ansi_term 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)", "bitflags 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", "datetime 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)", "getopts 0.2.14 (registry+https://github.com/rust-lang/crates.io-index)", "git2 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 0.1.15 (registry+https://github.com/rust-lang/crates.io-index)", - "libc 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "locale 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)", "natord 1.0.9 (registry+https://github.com/rust-lang/crates.io-index)", "num_cpus 0.2.10 (registry+https://github.com/rust-lang/crates.io-index)", @@ -38,7 +38,7 @@ dependencies = [ [[package]] name = "ansi_term" -version = "0.7.0" +version = "0.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] @@ -92,7 +92,7 @@ version = "0.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "bitflags 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", - "libc 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "libgit2-sys 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)", "url 0.2.38 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -118,7 +118,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "libc" -version = "0.2.2" +version = "0.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] @@ -127,10 +127,10 @@ version = "0.3.8" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "cmake 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", - "libc 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "libssh2-sys 0.1.34 (registry+https://github.com/rust-lang/crates.io-index)", "libz-sys 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", - "openssl-sys 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)", + "openssl-sys 0.7.2 (registry+https://github.com/rust-lang/crates.io-index)", "pkg-config 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -148,9 +148,9 @@ version = "0.1.34" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "cmake 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)", - "libc 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "libz-sys 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", - "openssl-sys 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)", + "openssl-sys 0.7.2 (registry+https://github.com/rust-lang/crates.io-index)", "pkg-config 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", "winapi 0.2.5 (registry+https://github.com/rust-lang/crates.io-index)", "ws2_32-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", @@ -162,7 +162,7 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "gcc 0.3.20 (registry+https://github.com/rust-lang/crates.io-index)", - "libc 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "pkg-config 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -185,7 +185,7 @@ name = "memchr" version = "0.1.7" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ - "libc 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -208,7 +208,7 @@ version = "0.2.10" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "kernel32-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", - "libc 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "winapi 0.2.5 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -222,10 +222,10 @@ dependencies = [ [[package]] name = "openssl-sys" -version = "0.7.1" +version = "0.7.2" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ - "libc 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "libressl-pnacl-sys 2.1.6 (registry+https://github.com/rust-lang/crates.io-index)", "pkg-config 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -257,7 +257,7 @@ version = "0.3.12" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "advapi32-sys 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)", - "libc 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "winapi 0.2.5 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -346,7 +346,7 @@ name = "users" version = "0.4.4" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ - "libc 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 01ef139..f328769 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,7 +7,7 @@ authors = [ "ogham@bsago.me" ] name = "exa" [dependencies] -ansi_term = "0.7.0" +ansi_term = "0.7.1" bitflags = "0.1" datetime = "0.4.1" getopts = "0.2.14" diff --git a/src/file.rs b/src/file.rs index 08c45bb..d4c9b9e 100644 --- a/src/file.rs +++ b/src/file.rs @@ -195,7 +195,7 @@ impl<'dir> File<'dir> { /// If statting the file fails (usually because the file on the /// other end doesn't exist), returns the *filename* of the file /// that should be there. - pub fn link_target(&self) -> Result { + pub fn link_target(&self) -> Result, String> { let path = match fs::read_link(&self.path) { Ok(path) => path, Err(_) => return Err(self.name.clone()), diff --git a/src/main.rs b/src/main.rs index 1e9f76c..11a2fc0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -135,8 +135,8 @@ impl Exa { match self.options.view { View::Grid(g) => g.view(&files), View::Details(d) => d.view(dir, files), - View::GridDetails(gd) => gd.view(dir, &files), - View::Lines(l) => l.view(&files), + View::GridDetails(gd) => gd.view(dir, files), + View::Lines(l) => l.view(files), } } } diff --git a/src/output/cell.rs b/src/output/cell.rs new file mode 100644 index 0000000..1d3f6e2 --- /dev/null +++ b/src/output/cell.rs @@ -0,0 +1,130 @@ +//! The `TextCell` type for the details and lines views. + +use ansi_term::{Style, ANSIString, ANSIStrings}; +use unicode_width::UnicodeWidthStr; + + +/// An individual cell that holds text in a table, used in the details and +/// lines views to store ANSI-terminal-formatted data before it is printed. +/// +/// A text cell is made up of zero or more strings coupled with the +/// pre-computed length of all the strings combined. When constructing details +/// or grid-details tables, the length will have to be queried multiple times, +/// so it makes sense to cache it. +/// +/// (This used to be called `Cell`, but was renamed because there’s a Rust +/// type by that name too.) +#[derive(PartialEq, Debug, Clone, Default)] +pub struct TextCell { + + /// The contents of this cell, as a vector of ANSI-styled strings. + pub contents: TextCellContents, + + /// The Unicode “display width” of this cell, in characters. + /// + /// As with the `File` type’s width, this is related to the number of + /// *graphemes*, rather than *characters*, in the cell: most are 1 column + /// wide, but in some contexts, certain characters are two columns wide. + pub length: usize, +} + +impl TextCell { + + /// Creates a new text cell that holds the given text in the given style, + /// computing the Unicode width of the text. + pub fn paint(style: Style, text: String) -> Self { + TextCell { + length: text.width(), + contents: vec![ style.paint(text) ], + } + } + + /// Creates a new text cell that holds the given text in the given style, + /// computing the Unicode width of the text. (This could be merged with + /// `paint`, but.) + pub fn paint_str(style: Style, text: &'static str) -> Self { + TextCell { + length: text.len(), + contents: vec![ style.paint(text) ], + } + } + + /// Creates a new “blank” text cell that contains a single hyphen in the + /// given style, which should be the “punctuation” style from a `Colours` + /// value. + /// + /// This is used in place of empty table cells, as it is easier to read + /// tabular data when there is *something* in each cell. + pub fn blank(style: Style) -> Self { + TextCell { + length: 1, + contents: vec![ style.paint("-") ], + } + } + + /// Adds the given number of unstyled spaces after this cell. + /// + /// This method allocates a `String` to hold the spaces. + pub fn add_spaces(&mut self, count: usize) { + use std::iter::repeat; + + self.length += count; + + let spaces: String = repeat(' ').take(count).collect(); + self.contents.push(Style::default().paint(spaces)); + } + + /// Adds the contents of another `ANSIString` to the end of this cell. + pub fn push(&mut self, string: ANSIString<'static>, length: usize) { + self.contents.push(string); + self.length += length; + } + + /// Adds all the contents of another `TextCell` to the end of this cell. + pub fn append(&mut self, other: TextCell) { + self.length += other.length; + self.contents.extend(other.contents); + } + + /// Produces an `ANSIStrings` value that can be used to print the styled + /// values of this cell as an ANSI-terminal-formatted string. + pub fn strings(&self) -> ANSIStrings { + ANSIStrings(&self.contents) + } +} + + +// I’d like to eventually abstract cells so that instead of *every* cell +// storing a vector, only variable-length cells would, and individual cells +// would just store an array of a fixed length (which would usually be just 1 +// or 2), which wouldn’t require a heap allocation. +// +// For examples, look at the `render_*` methods in the `Table` object in the +// details view: +// +// - `render_blocks`, `inode`, and `links` will always return a +// one-string-long TextCell; +// - `render_size` will return one or two strings in a TextCell, depending on +// the size and whether one is present; +// - `render_permissions` will return ten or eleven strings; +// - `filename` and `symlink_filename` in the output module root return six or +// five strings. +// +// In none of these cases are we dealing with a *truly variable* number of +// strings: it is only when the strings are concatenated together do we need a +// growable, heap-allocated buffer. +// +// So it would be nice to abstract the `TextCell` type so instead of a `Vec`, +// it can use anything of type `T: IntoIterator>`. +// This would allow us to still hold all the data, but allocate less. +// +// But exa still has bugs and I need to fix those first :( + + +/// The contents of a text cell, as a vector of ANSI-styled strings. +/// +/// It’s possible to use this type directly in the case where you want a +/// `TextCell` but aren’t concerned with tracking its width, because it occurs +/// in the final cell of a table or grid and there’s no point padding it. This +/// happens when dealing with file names. +pub type TextCellContents = Vec>; \ No newline at end of file diff --git a/src/output/column.rs b/src/output/column.rs index c2b8568..cfec1f9 100644 --- a/src/output/column.rs +++ b/src/output/column.rs @@ -1,6 +1,3 @@ -use ansi_term::Style; -use unicode_width::UnicodeWidthStr; - use dir::Dir; @@ -194,38 +191,3 @@ impl Default for TimeTypes { TimeTypes { accessed: false, modified: true, created: false } } } - - -#[derive(PartialEq, Debug, Clone)] -pub struct Cell { - pub length: usize, - pub text: String, -} - -impl Cell { - pub fn empty() -> Cell { - Cell { - text: String::new(), - length: 0, - } - } - - pub fn paint(style: Style, string: &str) -> Cell { - Cell { - text: style.paint(string).to_string(), - length: UnicodeWidthStr::width(string), - } - } - - pub fn add_spaces(&mut self, count: usize) { - self.length += count; - for _ in 0 .. count { - self.text.push(' '); - } - } - - pub fn append(&mut self, other: &Cell) { - self.length += other.length; - self.text.push_str(&*other.text); - } -} diff --git a/src/output/details.rs b/src/output/details.rs index a7d1da9..e9e59b2 100644 --- a/src/output/details.rs +++ b/src/output/details.rs @@ -124,9 +124,10 @@ use feature::xattr::{Attribute, FileAttributes}; use file::fields as f; use file::File; use options::{FileFilter, RecurseOptions}; -use output::column::{Alignment, Column, Columns, Cell, SizeFormat}; +use output::column::{Alignment, Column, Columns, SizeFormat}; +use output::cell::TextCell; -use ansi_term::{ANSIString, ANSIStrings, Style}; +use ansi_term::Style; use datetime::local::{LocalDateTime, DatePiece}; use datetime::format::DateFormat; @@ -198,7 +199,7 @@ impl Details { // Then add files to the table and print it out. self.add_files_to_table(&mut table, files, 0); for cell in table.print_table() { - println!("{}", cell.text); + println!("{}", cell.strings()); } } @@ -213,20 +214,18 @@ impl Details { let mut file_eggs = Vec::new(); struct Egg<'_> { - cells: Vec, - name: Cell, + cells: Vec, xattrs: Vec, errors: Vec<(io::Error, Option)>, dir: Option, - file: Arc>, + file: File<'_>, } pool.scoped(|scoped| { let file_eggs = Arc::new(Mutex::new(&mut file_eggs)); let table = Arc::new(Mutex::new(&mut table)); - for file in src.into_iter() { - let file: Arc = Arc::new(file); + for file in src { let file_eggs = file_eggs.clone(); let table = table.clone(); @@ -251,11 +250,6 @@ impl Details { let cells = table.lock().unwrap().cells_for_file(&file, !xattrs.is_empty()); - let name = Cell { - text: filename(&file, &self.colours, true), - length: file.file_name_width() - }; - let mut dir = None; if let Some(r) = self.recurse { @@ -268,7 +262,6 @@ impl Details { let egg = Egg { cells: cells, - name: name, xattrs: xattrs, errors: errors, dir: dir, @@ -280,17 +273,22 @@ impl Details { } }); - file_eggs.sort_by(|a, b| self.filter.compare_files(&*a.file, &*b.file)); + file_eggs.sort_by(|a, b| self.filter.compare_files(&a.file, &b.file)); let num_eggs = file_eggs.len(); for (index, egg) in file_eggs.into_iter().enumerate() { let mut files = Vec::new(); let mut errors = egg.errors; + let name = TextCell { + length: egg.file.file_name_width(), + contents: filename(egg.file, &self.colours, true), + }; + let row = Row { depth: depth, cells: Some(egg.cells), - name: egg.name, + name: name, last: index == num_eggs - 1, }; @@ -342,14 +340,11 @@ struct Row { /// almost always be `Some`, containing a vector of cells. It will only be /// `None` for a row displaying an attribute or error, neither of which /// have cells. - cells: Option>, - - // Did You Know? - // A Vec and an Option> actually have the same byte size! + cells: Option>, /// This file's name, in coloured output. The name is treated separately /// from the other cells, as it never requires padding. - name: Cell, + name: TextCell, /// How many directories deep into the tree structure this is. Directories /// on top have depth 0. @@ -429,8 +424,8 @@ impl Table where U: Users { pub fn add_header(&mut self) { let row = Row { depth: 0, - cells: Some(self.columns.iter().map(|c| Cell::paint(self.colours.header, c.header())).collect()), - name: Cell::paint(self.colours.header, "Name"), + cells: Some(self.columns.iter().map(|c| TextCell::paint_str(self.colours.header, c.header())).collect()), + name: TextCell::paint_str(self.colours.header, "Name"), last: false, }; @@ -446,7 +441,7 @@ impl Table where U: Users { let row = Row { depth: depth, cells: None, - name: Cell::paint(self.colours.broken_arrow, &error_message), + name: TextCell::paint(self.colours.broken_arrow, error_message), last: last, }; @@ -457,19 +452,26 @@ impl Table where U: Users { let row = Row { depth: depth, cells: None, - name: Cell::paint(self.colours.perms.attribute, &format!("{} (len {})", xattr.name, xattr.size)), + name: TextCell::paint(self.colours.perms.attribute, format!("{} (len {})", xattr.name, xattr.size)), last: last, }; self.rows.push(row); } - pub fn add_file_with_cells(&mut self, cells: Vec, file: &File, depth: usize, last: bool, links: bool) { + pub fn filename_cell(&self, file: File, links: bool) -> TextCell { + TextCell { + length: file.file_name_width(), + contents: filename(file, &self.colours, links), + } + } + + pub fn add_file_with_cells(&mut self, cells: Vec, name_cell: TextCell, depth: usize, last: bool) { let row = Row { - depth: depth, - cells: Some(cells), - name: Cell { text: filename(file, &self.colours, links), length: file.file_name_width() }, - last: last, + depth: depth, + cells: Some(cells), + name: name_cell, + last: last, }; self.rows.push(row); @@ -477,13 +479,13 @@ impl Table where U: Users { /// Use the list of columns to find which cells should be produced for /// this file, per-column. - pub fn cells_for_file(&mut self, file: &File, xattrs: bool) -> Vec { + pub fn cells_for_file(&mut self, file: &File, xattrs: bool) -> Vec { self.columns.clone().iter() .map(|c| self.display(file, c, xattrs)) .collect() } - fn display(&mut self, file: &File, column: &Column, xattrs: bool) -> Cell { + fn display(&mut self, file: &File, column: &Column, xattrs: bool) -> TextCell { use output::column::TimeType::*; match *column { @@ -501,7 +503,7 @@ impl Table where U: Users { } } - fn render_permissions(&self, permissions: f::Permissions, xattrs: bool) -> Cell { + fn render_permissions(&self, permissions: f::Permissions, xattrs: bool) -> TextCell { let c = self.colours.perms; let bit = |bit, chr: &'static str, style: Style| { if bit { style.paint(chr) } else { self.colours.punctuation.paint("-") } @@ -518,7 +520,7 @@ impl Table where U: Users { let x_colour = if let f::Type::File = permissions.file_type { c.user_execute_file } else { c.user_execute_other }; - let mut columns = vec![ + let mut chars = vec![ file_type, bit(permissions.user_read, "r", c.user_read), bit(permissions.user_write, "w", c.user_write), @@ -532,63 +534,71 @@ impl Table where U: Users { ]; if xattrs { - columns.push(c.attribute.paint("@")); + chars.push(c.attribute.paint("@")); } - Cell { - text: ANSIStrings(&columns).to_string(), - length: columns.len(), + TextCell { + length: chars.len(), + contents: chars, } } - fn render_links(&self, links: f::Links) -> Cell { + fn render_links(&self, links: f::Links) -> TextCell { let style = if links.multiple { self.colours.links.multi_link_file } else { self.colours.links.normal }; - Cell::paint(style, &self.numeric.format_int(links.count)) + TextCell::paint(style, self.numeric.format_int(links.count)) } - fn render_blocks(&self, blocks: f::Blocks) -> Cell { + fn render_blocks(&self, blocks: f::Blocks) -> TextCell { match blocks { - f::Blocks::Some(blocks) => Cell::paint(self.colours.blocks, &blocks.to_string()), - f::Blocks::None => Cell::paint(self.colours.punctuation, "-"), + f::Blocks::Some(blk) => TextCell::paint(self.colours.blocks, blk.to_string()), + f::Blocks::None => TextCell::blank(self.colours.punctuation), } } - fn render_inode(&self, inode: f::Inode) -> Cell { - Cell::paint(self.colours.inode, &inode.0.to_string()) + fn render_inode(&self, inode: f::Inode) -> TextCell { + TextCell::paint(self.colours.inode, inode.0.to_string()) } - fn render_size(&self, size: f::Size, size_format: SizeFormat) -> Cell { - use number_prefix::{binary_prefix, decimal_prefix, Prefixed, Standalone, PrefixNames}; + fn render_size(&self, size: f::Size, size_format: SizeFormat) -> TextCell { + use number_prefix::{binary_prefix, decimal_prefix}; + use number_prefix::{Prefixed, Standalone, PrefixNames}; - if let f::Size::Some(offset) = size { - let result = match size_format { - SizeFormat::DecimalBytes => decimal_prefix(offset as f64), - SizeFormat::BinaryBytes => binary_prefix(offset as f64), - SizeFormat::JustBytes => return Cell::paint(self.colours.size.numbers, &self.numeric.format_int(offset)), - }; + let size = match size { + f::Size::Some(s) => s, + f::Size::None => return TextCell::blank(self.colours.punctuation), + }; - match result { - Standalone(bytes) => Cell::paint(self.colours.size.numbers, &*bytes.to_string()), - Prefixed(prefix, n) => { - let number = if n < 10f64 { self.numeric.format_float(n, 1) } else { self.numeric.format_int(n as isize) }; - let symbol = prefix.symbol(); + let result = match size_format { + SizeFormat::DecimalBytes => decimal_prefix(size as f64), + SizeFormat::BinaryBytes => binary_prefix(size as f64), + SizeFormat::JustBytes => { + let string = self.numeric.format_int(size); + return TextCell::paint(self.colours.size.numbers, string); + }, + }; - Cell { - text: ANSIStrings( &[ self.colours.size.numbers.paint(&number[..]), self.colours.size.unit.paint(symbol) ]).to_string(), - length: number.len() + symbol.len(), - } - } - } - } - else { - Cell::paint(self.colours.punctuation, "-") + let (prefix, n) = match result { + Standalone(b) => return TextCell::paint(self.colours.size.numbers, b.to_string()), + Prefixed(p, n) => (p, n) + }; + + let symbol = prefix.symbol(); + let number = if n < 10f64 { self.numeric.format_float(n, 1) } + else { self.numeric.format_int(n as isize) }; + + TextCell { + length: number.len() + symbol.len(), + contents: vec![ + self.colours.size.numbers.paint(number), + self.colours.size.unit.paint(symbol), + ], } } #[allow(trivial_numeric_casts)] - fn render_time(&self, timestamp: f::Time) -> Cell { + fn render_time(&self, timestamp: f::Time) -> TextCell { let date = self.tz.at(LocalDateTime::at(timestamp.0 as i64)); let datestamp = if date.year() == self.current_year { @@ -598,60 +608,60 @@ impl Table where U: Users { DATE_AND_YEAR.format(&date, &self.time) }; - Cell::paint(self.colours.date, &datestamp) + TextCell::paint(self.colours.date, datestamp) } - fn render_git_status(&self, git: f::Git) -> Cell { - Cell { - text: ANSIStrings(&[ self.render_git_char(git.staged), - self.render_git_char(git.unstaged) ]).to_string(), - length: 2, - } - } - - fn render_git_char(&self, status: f::GitStatus) -> ANSIString { - match status { + fn render_git_status(&self, git: f::Git) -> TextCell { + let git_char = |status| match status { f::GitStatus::NotModified => self.colours.punctuation.paint("-"), f::GitStatus::New => self.colours.git.new.paint("N"), f::GitStatus::Modified => self.colours.git.modified.paint("M"), f::GitStatus::Deleted => self.colours.git.deleted.paint("D"), f::GitStatus::Renamed => self.colours.git.renamed.paint("R"), f::GitStatus::TypeChange => self.colours.git.typechange.paint("T"), + }; + + TextCell { + length: 2, + contents: vec![ + git_char(git.staged), + git_char(git.unstaged) + ], } } - fn render_user(&mut self, user: f::User) -> Cell { + fn render_user(&mut self, user: f::User) -> TextCell { let user_name = match self.users.get_user_by_uid(user.0) { Some(user) => user.name, None => user.0.to_string(), }; let style = if self.users.get_current_uid() == user.0 { self.colours.users.user_you } - else { self.colours.users.user_someone_else }; - Cell::paint(style, &*user_name) + else { self.colours.users.user_someone_else }; + TextCell::paint(style, user_name) } - fn render_group(&mut self, group: f::Group) -> Cell { + fn render_group(&mut self, group: f::Group) -> TextCell { let mut style = self.colours.users.group_not_yours; - let group_name = match self.users.get_group_by_gid(group.0) { - Some(group) => { - let current_uid = self.users.get_current_uid(); - if let Some(current_user) = self.users.get_user_by_uid(current_uid) { - if current_user.primary_group == group.gid || group.members.contains(¤t_user.name) { - style = self.colours.users.group_yours; - } - } - group.name - }, - None => group.0.to_string(), + let group = match self.users.get_group_by_gid(group.0) { + Some(g) => g, + None => return TextCell::paint(style, group.0.to_string()), }; - Cell::paint(style, &*group_name) + let current_uid = self.users.get_current_uid(); + if let Some(current_user) = self.users.get_user_by_uid(current_uid) { + if current_user.primary_group == group.gid + || group.members.contains(¤t_user.name) { + style = self.colours.users.group_yours; + } + } + + TextCell::paint(style, group.name) } /// Render the table as a vector of Cells, to be displayed on standard output. - pub fn print_table(&self) -> Vec { + pub fn print_table(self) -> Vec { let mut stack = Vec::new(); let mut cells = Vec::new(); @@ -664,14 +674,16 @@ impl Table where U: Users { let total_width: usize = self.columns.len() + column_widths.iter().fold(0, Add::add); - for row in self.rows.iter() { - let mut cell = Cell::empty(); + for row in self.rows { + let mut cell = TextCell::default(); + + if let Some(cells) = row.cells { + for (n, (this_cell, width)) in cells.into_iter().zip(column_widths.iter()).enumerate() { + let padding = width - this_cell.length; - if let Some(ref cells) = row.cells { - for (n, width) in column_widths.iter().enumerate() { match self.columns[n].alignment() { - Alignment::Left => { cell.append(&cells[n]); cell.add_spaces(width - cells[n].length); } - Alignment::Right => { cell.add_spaces(width - cells[n].length); cell.append(&cells[n]); } + Alignment::Left => { cell.append(this_cell); cell.add_spaces(padding); } + Alignment::Right => { cell.add_spaces(padding); cell.append(this_cell); } } cell.add_spaces(1); @@ -681,8 +693,7 @@ impl Table where U: Users { cell.add_spaces(total_width) } - let mut filename = String::new(); - let mut filename_length = 0; + let mut filename = TextCell::default(); // A stack tracks which tree characters should be printed. It's // necessary to maintain information about the previously-printed @@ -699,8 +710,7 @@ impl Table where U: Users { stack[row.depth] = if row.last { TreePart::Corner } else { TreePart::Edge }; for i in 1 .. row.depth + 1 { - filename.push_str(&*self.colours.punctuation.paint(stack[i].ascii_art()).to_string()); - filename_length += 4; + filename.push(self.colours.punctuation.paint(stack[i].ascii_art()), 4); } stack[row.depth] = if row.last { TreePart::Blank } else { TreePart::Line }; @@ -708,15 +718,13 @@ impl Table where U: Users { // If any tree characters have been printed, then add an extra // space, which makes the output look much better. if row.depth != 0 { - filename.push(' '); - filename_length += 1; + filename.add_spaces(1); } // Print the name without worrying about padding. - filename.push_str(&*row.name.text); - filename_length += row.name.length; + filename.append(row.name); - cell.append(&Cell { text: filename, length: filename_length }); + cell.append(filename); cells.push(cell); } @@ -767,7 +775,8 @@ pub mod test { pub use super::Table; pub use file::File; pub use file::fields as f; - pub use output::column::{Cell, Column}; + pub use output::column::Column; + pub use output::cell::TextCell; pub use users::{User, Group, uid_t, gid_t}; pub use users::mock::MockUsers; @@ -806,7 +815,7 @@ pub mod test { table.users = users; let user = f::User(1000); - let expected = Cell::paint(Red.bold(), "enoch"); + let expected = TextCell::paint_str(Red.bold(), "enoch"); assert_eq!(expected, table.render_user(user)) } @@ -819,7 +828,7 @@ pub mod test { table.users = users; let user = f::User(1000); - let expected = Cell::paint(Cyan.bold(), "1000"); + let expected = TextCell::paint_str(Cyan.bold(), "1000"); assert_eq!(expected, table.render_user(user)); } @@ -830,7 +839,7 @@ pub mod test { table.users.add_user(newser(1000, "enoch", 100)); let user = f::User(1000); - let expected = Cell::paint(Green.bold(), "enoch"); + let expected = TextCell::paint_str(Green.bold(), "enoch"); assert_eq!(expected, table.render_user(user)); } @@ -840,7 +849,7 @@ pub mod test { table.colours.users.user_someone_else = Red.normal(); let user = f::User(1000); - let expected = Cell::paint(Red.normal(), "1000"); + let expected = TextCell::paint_str(Red.normal(), "1000"); assert_eq!(expected, table.render_user(user)); } @@ -850,7 +859,7 @@ pub mod test { table.colours.users.user_someone_else = Blue.underline(); let user = f::User(2_147_483_648); - let expected = Cell::paint(Blue.underline(), "2147483648"); + let expected = TextCell::paint_str(Blue.underline(), "2147483648"); assert_eq!(expected, table.render_user(user)); } } @@ -869,7 +878,7 @@ pub mod test { table.users = users; let group = f::Group(100); - let expected = Cell::paint(Fixed(101).normal(), "folk"); + let expected = TextCell::paint_str(Fixed(101).normal(), "folk"); assert_eq!(expected, table.render_group(group)) } @@ -882,7 +891,7 @@ pub mod test { table.users = users; let group = f::Group(100); - let expected = Cell::paint(Fixed(87).normal(), "100"); + let expected = TextCell::paint_str(Fixed(87).normal(), "100"); assert_eq!(expected, table.render_group(group)); } @@ -897,7 +906,7 @@ pub mod test { table.users = users; let group = f::Group(100); - let expected = Cell::paint(Fixed(64).normal(), "folk"); + let expected = TextCell::paint_str(Fixed(64).normal(), "folk"); assert_eq!(expected, table.render_group(group)) } @@ -912,7 +921,7 @@ pub mod test { table.users = users; let group = f::Group(100); - let expected = Cell::paint(Fixed(31).normal(), "folk"); + let expected = TextCell::paint_str(Fixed(31).normal(), "folk"); assert_eq!(expected, table.render_group(group)) } @@ -922,7 +931,7 @@ pub mod test { table.colours.users.group_not_yours = Blue.underline(); let group = f::Group(2_147_483_648); - let expected = Cell::paint(Blue.underline(), "2147483648"); + let expected = TextCell::paint_str(Blue.underline(), "2147483648"); assert_eq!(expected, table.render_group(group)); } } diff --git a/src/output/grid_details.rs b/src/output/grid_details.rs index e482464..332bfce 100644 --- a/src/output/grid_details.rs +++ b/src/output/grid_details.rs @@ -1,5 +1,6 @@ use std::iter::repeat; +use ansi_term::ANSIStrings; use users::OSUsers; use term_grid as grid; @@ -7,7 +8,8 @@ use dir::Dir; use feature::xattr::FileAttributes; use file::File; -use output::column::{Column, Cell}; +use output::cell::TextCell; +use output::column::Column; use output::details::{Details, Table}; use output::grid::Grid; @@ -25,19 +27,25 @@ fn file_has_xattrs(file: &File) -> bool { } impl GridDetails { - pub fn view(&self, dir: Option<&Dir>, files: &[File]) { + pub fn view(&self, dir: Option<&Dir>, files: Vec) { let columns_for_dir = match self.details.columns { Some(cols) => cols.for_dir(dir), None => Vec::new(), }; let mut first_table = Table::with_options(self.details.colours, columns_for_dir.clone()); - let cells: Vec<_> = files.iter().map(|file| first_table.cells_for_file(file, file_has_xattrs(file))).collect(); + let cells = files.iter() + .map(|file| first_table.cells_for_file(file, file_has_xattrs(file))) + .collect::>(); - let mut last_working_table = self.make_grid(1, &*columns_for_dir, files, cells.clone()); + let file_names = files.into_iter() + .map(|file| first_table.filename_cell(file, false)) + .collect::>(); + + let mut last_working_table = self.make_grid(1, &columns_for_dir, &file_names, cells.clone()); for column_count in 2.. { - let grid = self.make_grid(column_count, &*columns_for_dir, files, cells.clone()); + let grid = self.make_grid(column_count, &columns_for_dir, &file_names, cells.clone()); let the_grid_fits = { let d = grid.fit_into_columns(column_count); @@ -60,7 +68,7 @@ impl GridDetails { table } - fn make_grid(&self, column_count: usize, columns_for_dir: &[Column], files: &[File], cells: Vec>) -> grid::Grid { + fn make_grid(&self, column_count: usize, columns_for_dir: &[Column], file_names: &[TextCell], cells: Vec>) -> grid::Grid { let mut tables: Vec<_> = repeat(()).map(|_| self.make_table(columns_for_dir)).take(column_count).collect(); let mut num_cells = cells.len(); @@ -71,7 +79,7 @@ impl GridDetails { let original_height = divide_rounding_up(cells.len(), column_count); let height = divide_rounding_up(num_cells, column_count); - for (i, (file, row)) in files.iter().zip(cells.into_iter()).enumerate() { + for (i, (file_name, row)) in file_names.iter().zip(cells.into_iter()).enumerate() { let index = if self.grid.across { i % column_count } @@ -79,10 +87,10 @@ impl GridDetails { i / original_height }; - tables[index].add_file_with_cells(row, file, 0, false, false); + tables[index].add_file_with_cells(row, file_name.clone(), 0, false); } - let columns: Vec<_> = tables.iter().map(|t| t.print_table()).collect(); + let columns: Vec<_> = tables.into_iter().map(|t| t.print_table()).collect(); let direction = if self.grid.across { grid::Direction::LeftToRight } else { grid::Direction::TopToBottom }; @@ -97,7 +105,7 @@ impl GridDetails { for column in columns.iter() { if row < column.len() { let cell = grid::Cell { - contents: column[row].text.clone(), + contents: ANSIStrings(&column[row].contents).to_string(), width: column[row].length, }; @@ -110,7 +118,7 @@ impl GridDetails { for column in columns.iter() { for cell in column.iter() { let cell = grid::Cell { - contents: cell.text.clone(), + contents: ANSIStrings(&cell.contents).to_string(), width: cell.length, }; diff --git a/src/output/lines.rs b/src/output/lines.rs index 7f19105..39ef2c3 100644 --- a/src/output/lines.rs +++ b/src/output/lines.rs @@ -1,6 +1,8 @@ use colours::Colours; use file::File; +use ansi_term::ANSIStrings; + use super::filename; @@ -11,9 +13,9 @@ pub struct Lines { /// The lines view literally just displays each file, line-by-line. impl Lines { - pub fn view(&self, files: &[File]) { + pub fn view(&self, files: Vec) { for file in files { - println!("{}", filename(file, &self.colours, true)); + println!("{}", ANSIStrings(&filename(file, &self.colours, true))); } } } diff --git a/src/output/mod.rs b/src/output/mod.rs index c875cde..642786e 100644 --- a/src/output/mod.rs +++ b/src/output/mod.rs @@ -1,9 +1,10 @@ -use ansi_term::ANSIStrings; +use ansi_term::Style; use colours::Colours; use file::File; use filetype::file_colour; +pub use self::cell::{TextCell, TextCellContents}; pub use self::details::Details; pub use self::grid::Grid; pub use self::lines::Lines; @@ -14,29 +15,37 @@ pub mod details; mod lines; mod grid_details; pub mod column; +mod cell; -pub fn filename(file: &File, colours: &Colours, links: bool) -> String { +pub fn filename(file: File, colours: &Colours, links: bool) -> TextCellContents { if links && file.is_link() { symlink_filename(file, colours) } else { - let style = file_colour(colours, file); - style.paint(&*file.name).to_string() + vec![ + file_colour(colours, &file).paint(file.name) + ] } } -fn symlink_filename(file: &File, colours: &Colours) -> String { +fn symlink_filename(file: File, colours: &Colours) -> TextCellContents { match file.link_target() { - Ok(target) => format!("{} {} {}", - file_colour(colours, file).paint(&*file.name), - colours.punctuation.paint("->"), - ANSIStrings(&[ colours.symlink_path.paint(target.path_prefix()), - file_colour(colours, &target).paint(target.name) ])), + Ok(target) => vec![ + file_colour(colours, &file).paint(file.name), + Style::default().paint(" "), + colours.punctuation.paint("->"), + Style::default().paint(" "), + colours.symlink_path.paint(target.path_prefix()), + file_colour(colours, &target).paint(target.name) + ], - Err(filename) => format!("{} {} {}", - file_colour(colours, file).paint(&*file.name), - colours.broken_arrow.paint("->"), - colours.broken_filename.paint(filename)), + Err(filename) => vec![ + file_colour(colours, &file).paint(file.name), + Style::default().paint(" "), + colours.broken_arrow.paint("->"), + Style::default().paint(" "), + colours.broken_filename.paint(filename), + ], } } From 4c2bf2f2e6231c14a8aa44cb49c828034c342a69 Mon Sep 17 00:00:00 2001 From: Benjamin Sago Date: Thu, 17 Dec 2015 10:15:09 +0800 Subject: [PATCH 2/7] Encapsulate "display width" in a struct This commit introduces the `output::cell::DisplayWidth` struct, which encapsulates the Unicode *display width* of a string in a struct that makes it less easily confused with the *length* of a string. The use of this type means that it's now harder to accidentally use a string's length-in-bytes as its width. I've fixed at least one case in the code where this was being done! The only casualty is that it introduces a dependency on the output module from the file module, which will be removed next commit. --- src/file.rs | 7 ++-- src/output/cell.rs | 67 ++++++++++++++++++++++++++++++-------- src/output/details.rs | 21 ++++++++---- src/output/grid.rs | 2 +- src/output/grid_details.rs | 4 +-- src/output/mod.rs | 2 +- 6 files changed, 76 insertions(+), 27 deletions(-) diff --git a/src/file.rs b/src/file.rs index d4c9b9e..dd105b2 100644 --- a/src/file.rs +++ b/src/file.rs @@ -7,9 +7,8 @@ use std::io::Result as IOResult; use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::{Component, Path, PathBuf}; -use unicode_width::UnicodeWidthStr; - use dir::Dir; +use output::DisplayWidth; use self::fields as f; @@ -185,8 +184,8 @@ impl<'dir> File<'dir> { /// This is related to the number of graphemes in the string: most /// characters are 1 columns wide, but in some contexts, certain /// characters are actually 2 columns wide. - pub fn file_name_width(&self) -> usize { - UnicodeWidthStr::width(&self.name[..]) + pub fn file_name_width(&self) -> DisplayWidth { + DisplayWidth::from(&*self.name) } /// Assuming the current file is a symlink, follows the link and diff --git a/src/output/cell.rs b/src/output/cell.rs index 1d3f6e2..bb0d8ee 100644 --- a/src/output/cell.rs +++ b/src/output/cell.rs @@ -1,5 +1,7 @@ //! The `TextCell` type for the details and lines views. +use std::ops::{Deref, DerefMut}; + use ansi_term::{Style, ANSIString, ANSIStrings}; use unicode_width::UnicodeWidthStr; @@ -20,12 +22,8 @@ pub struct TextCell { /// The contents of this cell, as a vector of ANSI-styled strings. pub contents: TextCellContents, - /// The Unicode “display width” of this cell, in characters. - /// - /// As with the `File` type’s width, this is related to the number of - /// *graphemes*, rather than *characters*, in the cell: most are 1 column - /// wide, but in some contexts, certain characters are two columns wide. - pub length: usize, + /// The Unicode “display width” of this cell. + pub length: DisplayWidth, } impl TextCell { @@ -34,7 +32,7 @@ impl TextCell { /// computing the Unicode width of the text. pub fn paint(style: Style, text: String) -> Self { TextCell { - length: text.width(), + length: DisplayWidth::from(&*text), contents: vec![ style.paint(text) ], } } @@ -44,7 +42,7 @@ impl TextCell { /// `paint`, but.) pub fn paint_str(style: Style, text: &'static str) -> Self { TextCell { - length: text.len(), + length: DisplayWidth::from(text), contents: vec![ style.paint(text) ], } } @@ -57,7 +55,7 @@ impl TextCell { /// tabular data when there is *something* in each cell. pub fn blank(style: Style) -> Self { TextCell { - length: 1, + length: DisplayWidth::from(1), contents: vec![ style.paint("-") ], } } @@ -68,7 +66,7 @@ impl TextCell { pub fn add_spaces(&mut self, count: usize) { use std::iter::repeat; - self.length += count; + (*self.length) += count; let spaces: String = repeat(' ').take(count).collect(); self.contents.push(Style::default().paint(spaces)); @@ -77,12 +75,12 @@ impl TextCell { /// Adds the contents of another `ANSIString` to the end of this cell. pub fn push(&mut self, string: ANSIString<'static>, length: usize) { self.contents.push(string); - self.length += length; + (*self.length) += length; } /// Adds all the contents of another `TextCell` to the end of this cell. pub fn append(&mut self, other: TextCell) { - self.length += other.length; + (*self.length) += *other.length; self.contents.extend(other.contents); } @@ -127,4 +125,47 @@ impl TextCell { /// `TextCell` but aren’t concerned with tracking its width, because it occurs /// in the final cell of a table or grid and there’s no point padding it. This /// happens when dealing with file names. -pub type TextCellContents = Vec>; \ No newline at end of file +pub type TextCellContents = Vec>; + + +/// The Unicode “display width” of a string. +/// +/// This is related to the number of *graphemes* of a string, rather than the +/// number of *characters*, or *bytes*: although most characters are one +/// column wide, a few can be two columns wide, and this is important to note +/// when calculating widths for displaying tables in a terminal. +/// +/// This type is used to ensure that the width, rather than the length, is +/// used when constructing a `TextCell` -- it's too easy to write something +/// like `file_name.len()` and assume it will work! +/// +/// It has `From` impls that convert an input string or fixed with to values +/// of this type, and will `Deref` to the contained `usize` value. +#[derive(PartialEq, Debug, Clone, Copy, Default)] +pub struct DisplayWidth(usize); + +impl<'a> From<&'a str> for DisplayWidth { + fn from(input: &'a str) -> DisplayWidth { + DisplayWidth(UnicodeWidthStr::width(input)) + } +} + +impl From for DisplayWidth { + fn from(width: usize) -> DisplayWidth { + DisplayWidth(width) + } +} + +impl Deref for DisplayWidth { + type Target = usize; + + fn deref<'a>(&'a self) -> &'a Self::Target { + &self.0 + } +} + +impl DerefMut for DisplayWidth { + fn deref_mut<'a>(&'a mut self) -> &'a mut Self::Target { + &mut self.0 + } +} \ No newline at end of file diff --git a/src/output/details.rs b/src/output/details.rs index e9e59b2..4f224bd 100644 --- a/src/output/details.rs +++ b/src/output/details.rs @@ -125,7 +125,7 @@ use file::fields as f; use file::File; use options::{FileFilter, RecurseOptions}; use output::column::{Alignment, Column, Columns, SizeFormat}; -use output::cell::TextCell; +use output::cell::{TextCell, DisplayWidth}; use ansi_term::Style; @@ -361,7 +361,7 @@ impl Row { /// not, returns 0. fn column_width(&self, index: usize) -> usize { match self.cells { - Some(ref cells) => cells[index].length, + Some(ref cells) => *cells[index].length, None => 0, } } @@ -537,8 +537,13 @@ impl Table where U: Users { chars.push(c.attribute.paint("@")); } + // As these are all ASCII characters, we can guarantee that they’re + // all going to be one character wide, and don’t need to compute the + // cell’s display width. + let width = DisplayWidth::from(chars.len()); + TextCell { - length: chars.len(), + length: width, contents: chars, } } @@ -588,8 +593,12 @@ impl Table where U: Users { let number = if n < 10f64 { self.numeric.format_float(n, 1) } else { self.numeric.format_int(n as isize) }; + // The numbers and symbols are guaranteed to be written in ASCII, so + // we can skip the display width calculation. + let width = DisplayWidth::from(number.len() + symbol.len()); + TextCell { - length: number.len() + symbol.len(), + length: width, contents: vec![ self.colours.size.numbers.paint(number), self.colours.size.unit.paint(symbol), @@ -622,7 +631,7 @@ impl Table where U: Users { }; TextCell { - length: 2, + length: DisplayWidth::from(2), contents: vec![ git_char(git.staged), git_char(git.unstaged) @@ -679,7 +688,7 @@ impl Table where U: Users { if let Some(cells) = row.cells { for (n, (this_cell, width)) in cells.into_iter().zip(column_widths.iter()).enumerate() { - let padding = width - this_cell.length; + let padding = width - *this_cell.length; match self.columns[n].alignment() { Alignment::Left => { cell.append(this_cell); cell.add_spaces(padding); } diff --git a/src/output/grid.rs b/src/output/grid.rs index 13d2f8d..73c79e4 100644 --- a/src/output/grid.rs +++ b/src/output/grid.rs @@ -27,7 +27,7 @@ impl Grid { for file in files.iter() { grid.add(grid::Cell { contents: file_colour(&self.colours, file).paint(&*file.name).to_string(), - width: file.file_name_width(), + width: *file.file_name_width(), }); } diff --git a/src/output/grid_details.rs b/src/output/grid_details.rs index 332bfce..271a3a3 100644 --- a/src/output/grid_details.rs +++ b/src/output/grid_details.rs @@ -106,7 +106,7 @@ impl GridDetails { if row < column.len() { let cell = grid::Cell { contents: ANSIStrings(&column[row].contents).to_string(), - width: column[row].length, + width: *column[row].length, }; grid.add(cell); @@ -119,7 +119,7 @@ impl GridDetails { for cell in column.iter() { let cell = grid::Cell { contents: ANSIStrings(&cell.contents).to_string(), - width: cell.length, + width: *cell.length, }; grid.add(cell); diff --git a/src/output/mod.rs b/src/output/mod.rs index 642786e..88fcbe4 100644 --- a/src/output/mod.rs +++ b/src/output/mod.rs @@ -4,7 +4,7 @@ use colours::Colours; use file::File; use filetype::file_colour; -pub use self::cell::{TextCell, TextCellContents}; +pub use self::cell::{TextCell, TextCellContents, DisplayWidth}; pub use self::details::Details; pub use self::grid::Grid; pub use self::lines::Lines; From 88653a00eb0512be43466e697886ee4902a99920 Mon Sep 17 00:00:00 2001 From: Benjamin Sago Date: Thu, 17 Dec 2015 10:27:44 +0800 Subject: [PATCH 3/7] Remove dependency between file and output mods By removing the `File#file_name_width` method, we can make the file module have no dependency on the output module -- in other words, the model (file) and the view (output) are now separate again! --- src/file.rs | 10 ---------- src/output/details.rs | 4 ++-- src/output/grid.rs | 3 ++- 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/src/file.rs b/src/file.rs index dd105b2..53265d7 100644 --- a/src/file.rs +++ b/src/file.rs @@ -8,7 +8,6 @@ use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::{Component, Path, PathBuf}; use dir::Dir; -use output::DisplayWidth; use self::fields as f; @@ -179,15 +178,6 @@ impl<'dir> File<'dir> { path_prefix } - /// The Unicode 'display width' of the filename. - /// - /// This is related to the number of graphemes in the string: most - /// characters are 1 columns wide, but in some contexts, certain - /// characters are actually 2 columns wide. - pub fn file_name_width(&self) -> DisplayWidth { - DisplayWidth::from(&*self.name) - } - /// Assuming the current file is a symlink, follows the link and /// returns a File object from the path the link points to. /// diff --git a/src/output/details.rs b/src/output/details.rs index 4f224bd..edfc2a6 100644 --- a/src/output/details.rs +++ b/src/output/details.rs @@ -281,7 +281,7 @@ impl Details { let mut errors = egg.errors; let name = TextCell { - length: egg.file.file_name_width(), + length: DisplayWidth::from(&*egg.file.name), contents: filename(egg.file, &self.colours, true), }; @@ -461,7 +461,7 @@ impl Table where U: Users { pub fn filename_cell(&self, file: File, links: bool) -> TextCell { TextCell { - length: file.file_name_width(), + length: DisplayWidth::from(&*file.name), contents: filename(file, &self.colours, links), } } diff --git a/src/output/grid.rs b/src/output/grid.rs index 73c79e4..69001bf 100644 --- a/src/output/grid.rs +++ b/src/output/grid.rs @@ -1,6 +1,7 @@ use colours::Colours; use file::File; use filetype::file_colour; +use output::DisplayWidth; use term_grid as grid; @@ -27,7 +28,7 @@ impl Grid { for file in files.iter() { grid.add(grid::Cell { contents: file_colour(&self.colours, file).paint(&*file.name).to_string(), - width: *file.file_name_width(), + width: *DisplayWidth::from(&*file.name), }); } From 39aa2104379cc26b3b75c9c1518e6cdb21a3c3b6 Mon Sep 17 00:00:00 2001 From: Benjamin Sago Date: Thu, 17 Dec 2015 10:34:11 +0800 Subject: [PATCH 4/7] Rename cell 'length' to 'width' Because, strictly speaking, it's not a length, it's a width! Also, re-order some struct constructors so that they're no longer order-dependent (it's no longer the case that a value will be borrowed for one field then consumed in another, meaning they have to be ordered in a certain way to compile. Now the value is just worked out beforehand and the fields can be specified in any order) --- src/output/cell.rs | 20 ++++++++++++-------- src/output/details.rs | 17 ++++++++++------- src/output/grid_details.rs | 4 ++-- 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/output/cell.rs b/src/output/cell.rs index bb0d8ee..4e4acbe 100644 --- a/src/output/cell.rs +++ b/src/output/cell.rs @@ -23,7 +23,7 @@ pub struct TextCell { pub contents: TextCellContents, /// The Unicode “display width” of this cell. - pub length: DisplayWidth, + pub width: DisplayWidth, } impl TextCell { @@ -31,9 +31,11 @@ impl TextCell { /// Creates a new text cell that holds the given text in the given style, /// computing the Unicode width of the text. pub fn paint(style: Style, text: String) -> Self { + let width = DisplayWidth::from(&*text); + TextCell { - length: DisplayWidth::from(&*text), contents: vec![ style.paint(text) ], + width: width, } } @@ -41,9 +43,11 @@ impl TextCell { /// computing the Unicode width of the text. (This could be merged with /// `paint`, but.) pub fn paint_str(style: Style, text: &'static str) -> Self { + let width = DisplayWidth::from(text); + TextCell { - length: DisplayWidth::from(text), contents: vec![ style.paint(text) ], + width: width, } } @@ -55,8 +59,8 @@ impl TextCell { /// tabular data when there is *something* in each cell. pub fn blank(style: Style) -> Self { TextCell { - length: DisplayWidth::from(1), contents: vec![ style.paint("-") ], + width: DisplayWidth::from(1), } } @@ -66,21 +70,21 @@ impl TextCell { pub fn add_spaces(&mut self, count: usize) { use std::iter::repeat; - (*self.length) += count; + (*self.width) += count; let spaces: String = repeat(' ').take(count).collect(); self.contents.push(Style::default().paint(spaces)); } /// Adds the contents of another `ANSIString` to the end of this cell. - pub fn push(&mut self, string: ANSIString<'static>, length: usize) { + pub fn push(&mut self, string: ANSIString<'static>, extra_width: usize) { self.contents.push(string); - (*self.length) += length; + (*self.width) += extra_width; } /// Adds all the contents of another `TextCell` to the end of this cell. pub fn append(&mut self, other: TextCell) { - (*self.length) += *other.length; + (*self.width) += *other.width; self.contents.extend(other.contents); } diff --git a/src/output/details.rs b/src/output/details.rs index edfc2a6..97be06f 100644 --- a/src/output/details.rs +++ b/src/output/details.rs @@ -279,10 +279,11 @@ impl Details { for (index, egg) in file_eggs.into_iter().enumerate() { let mut files = Vec::new(); let mut errors = egg.errors; + let width = DisplayWidth::from(&*egg.file.name); let name = TextCell { - length: DisplayWidth::from(&*egg.file.name), contents: filename(egg.file, &self.colours, true), + width: width, }; let row = Row { @@ -361,7 +362,7 @@ impl Row { /// not, returns 0. fn column_width(&self, index: usize) -> usize { match self.cells { - Some(ref cells) => *cells[index].length, + Some(ref cells) => *cells[index].width, None => 0, } } @@ -460,9 +461,11 @@ impl Table where U: Users { } pub fn filename_cell(&self, file: File, links: bool) -> TextCell { + let width = DisplayWidth::from(&*file.name); + TextCell { - length: DisplayWidth::from(&*file.name), contents: filename(file, &self.colours, links), + width: width, } } @@ -543,8 +546,8 @@ impl Table where U: Users { let width = DisplayWidth::from(chars.len()); TextCell { - length: width, contents: chars, + width: width, } } @@ -598,7 +601,7 @@ impl Table where U: Users { let width = DisplayWidth::from(number.len() + symbol.len()); TextCell { - length: width, + width: width, contents: vec![ self.colours.size.numbers.paint(number), self.colours.size.unit.paint(symbol), @@ -631,7 +634,7 @@ impl Table where U: Users { }; TextCell { - length: DisplayWidth::from(2), + width: DisplayWidth::from(2), contents: vec![ git_char(git.staged), git_char(git.unstaged) @@ -688,7 +691,7 @@ impl Table where U: Users { if let Some(cells) = row.cells { for (n, (this_cell, width)) in cells.into_iter().zip(column_widths.iter()).enumerate() { - let padding = width - *this_cell.length; + let padding = width - *this_cell.width; match self.columns[n].alignment() { Alignment::Left => { cell.append(this_cell); cell.add_spaces(padding); } diff --git a/src/output/grid_details.rs b/src/output/grid_details.rs index 271a3a3..3dcf4b1 100644 --- a/src/output/grid_details.rs +++ b/src/output/grid_details.rs @@ -106,7 +106,7 @@ impl GridDetails { if row < column.len() { let cell = grid::Cell { contents: ANSIStrings(&column[row].contents).to_string(), - width: *column[row].length, + width: *column[row].width, }; grid.add(cell); @@ -119,7 +119,7 @@ impl GridDetails { for cell in column.iter() { let cell = grid::Cell { contents: ANSIStrings(&cell.contents).to_string(), - width: *cell.length, + width: *cell.width, }; grid.add(cell); From 15cd67abe67ffeaef812f4080e1e35b06bf31784 Mon Sep 17 00:00:00 2001 From: Benjamin Sago Date: Thu, 17 Dec 2015 17:51:42 +0800 Subject: [PATCH 5/7] Turn TextCellContents into a struct The benefit of this is that it make it possible to convert text cell contents vectors into text cells with a method (see next commit). Casualties include having to call `.into()` on vectors everywhere, which I'm not convinced is a bad thing. --- src/output/cell.rs | 56 +++++++++++++++++++++++++++++++++---------- src/output/details.rs | 6 ++--- src/output/mod.rs | 6 ++--- 3 files changed, 49 insertions(+), 19 deletions(-) diff --git a/src/output/cell.rs b/src/output/cell.rs index 4e4acbe..d900f6d 100644 --- a/src/output/cell.rs +++ b/src/output/cell.rs @@ -26,6 +26,14 @@ pub struct TextCell { pub width: DisplayWidth, } +impl Deref for TextCell { + type Target = TextCellContents; + + fn deref<'a>(&'a self) -> &'a Self::Target { + &self.contents + } +} + impl TextCell { /// Creates a new text cell that holds the given text in the given style, @@ -34,7 +42,7 @@ impl TextCell { let width = DisplayWidth::from(&*text); TextCell { - contents: vec![ style.paint(text) ], + contents: vec![ style.paint(text) ].into(), width: width, } } @@ -46,7 +54,7 @@ impl TextCell { let width = DisplayWidth::from(text); TextCell { - contents: vec![ style.paint(text) ], + contents: vec![ style.paint(text) ].into(), width: width, } } @@ -59,7 +67,7 @@ impl TextCell { /// tabular data when there is *something* in each cell. pub fn blank(style: Style) -> Self { TextCell { - contents: vec![ style.paint("-") ], + contents: vec![ style.paint("-") ].into(), width: DisplayWidth::from(1), } } @@ -73,25 +81,19 @@ impl TextCell { (*self.width) += count; let spaces: String = repeat(' ').take(count).collect(); - self.contents.push(Style::default().paint(spaces)); + self.contents.0.push(Style::default().paint(spaces)); } /// Adds the contents of another `ANSIString` to the end of this cell. pub fn push(&mut self, string: ANSIString<'static>, extra_width: usize) { - self.contents.push(string); + self.contents.0.push(string); (*self.width) += extra_width; } /// Adds all the contents of another `TextCell` to the end of this cell. pub fn append(&mut self, other: TextCell) { (*self.width) += *other.width; - self.contents.extend(other.contents); - } - - /// Produces an `ANSIStrings` value that can be used to print the styled - /// values of this cell as an ANSI-terminal-formatted string. - pub fn strings(&self) -> ANSIStrings { - ANSIStrings(&self.contents) + self.contents.0.extend(other.contents.0); } } @@ -129,7 +131,35 @@ impl TextCell { /// `TextCell` but aren’t concerned with tracking its width, because it occurs /// in the final cell of a table or grid and there’s no point padding it. This /// happens when dealing with file names. -pub type TextCellContents = Vec>; +#[derive(PartialEq, Debug, Clone, Default)] +pub struct TextCellContents(Vec>); + +impl From>> for TextCellContents { + fn from(strings: Vec>) -> TextCellContents { + TextCellContents(strings) + } +} + +impl Deref for TextCellContents { + type Target = [ANSIString<'static>]; + + fn deref<'a>(&'a self) -> &'a Self::Target { + &*self.0 + } +} + +// No DerefMut implementation here -- it would be publicly accessible, and as +// the contents only get changed in this module, the mutators in the struct +// above can just access the value directly. + +impl TextCellContents { + + /// Produces an `ANSIStrings` value that can be used to print the styled + /// values of this cell as an ANSI-terminal-formatted string. + pub fn strings(&self) -> ANSIStrings { + ANSIStrings(&self.0) + } +} /// The Unicode “display width” of a string. diff --git a/src/output/details.rs b/src/output/details.rs index 97be06f..0b45b7b 100644 --- a/src/output/details.rs +++ b/src/output/details.rs @@ -546,7 +546,7 @@ impl Table where U: Users { let width = DisplayWidth::from(chars.len()); TextCell { - contents: chars, + contents: chars.into(), width: width, } } @@ -605,7 +605,7 @@ impl Table where U: Users { contents: vec![ self.colours.size.numbers.paint(number), self.colours.size.unit.paint(symbol), - ], + ].into(), } } @@ -638,7 +638,7 @@ impl Table where U: Users { contents: vec![ git_char(git.staged), git_char(git.unstaged) - ], + ].into(), } } diff --git a/src/output/mod.rs b/src/output/mod.rs index 88fcbe4..d1c7e14 100644 --- a/src/output/mod.rs +++ b/src/output/mod.rs @@ -25,7 +25,7 @@ pub fn filename(file: File, colours: &Colours, links: bool) -> TextCellContents else { vec![ file_colour(colours, &file).paint(file.name) - ] + ].into() } } @@ -38,7 +38,7 @@ fn symlink_filename(file: File, colours: &Colours) -> TextCellContents { Style::default().paint(" "), colours.symlink_path.paint(target.path_prefix()), file_colour(colours, &target).paint(target.name) - ], + ].into(), Err(filename) => vec![ file_colour(colours, &file).paint(file.name), @@ -46,6 +46,6 @@ fn symlink_filename(file: File, colours: &Colours) -> TextCellContents { colours.broken_arrow.paint("->"), Style::default().paint(" "), colours.broken_filename.paint(filename), - ], + ].into(), } } From 1b3492ce45552fa4335374ed6d3692992d00278b Mon Sep 17 00:00:00 2001 From: Benjamin Sago Date: Sun, 20 Dec 2015 17:56:57 +1100 Subject: [PATCH 6/7] Move colours module into output This commit moves the colours module to be a sub-module of the output one. This makes sense because finding which colour a certain file should be is only done during output, and (I think) the only places that the `Colours` struct's fields are ever queried is from the output module. The only casualty was that the `file_colour` from the filetype module had to be moved, as determining colours is no longer part of that module - only determining filetype is. So it now reflects its name! --- src/filetype.rs | 26 +------------------------- src/main.rs | 1 - src/options.rs | 2 +- src/{ => output}/colours.rs | 0 src/output/details.rs | 2 +- src/output/grid.rs | 10 +++++----- src/output/lines.rs | 6 +++--- src/output/mod.rs | 29 +++++++++++++++++++++++++---- 8 files changed, 36 insertions(+), 40 deletions(-) rename src/{ => output}/colours.rs (100%) diff --git a/src/filetype.rs b/src/filetype.rs index ed2f369..b8427d4 100644 --- a/src/filetype.rs +++ b/src/filetype.rs @@ -1,31 +1,7 @@ -use ansi_term::Style; - use file::File; -use colours::Colours; -pub fn file_colour(colours: &Colours, file: &File) -> Style { - match file { - f if f.is_directory() => colours.filetypes.directory, - f if f.is_executable_file() => colours.filetypes.executable, - f if f.is_link() => colours.filetypes.symlink, - f if !f.is_file() => colours.filetypes.special, - f if f.is_immediate() => colours.filetypes.immediate, - f if f.is_image() => colours.filetypes.image, - f if f.is_video() => colours.filetypes.video, - f if f.is_music() => colours.filetypes.music, - f if f.is_lossless() => colours.filetypes.lossless, - f if f.is_crypto() => colours.filetypes.crypto, - f if f.is_document() => colours.filetypes.document, - f if f.is_compressed() => colours.filetypes.compressed, - f if f.is_temp() => colours.filetypes.temp, - f if f.is_compiled() => colours.filetypes.compiled, - _ => colours.filetypes.normal, - } -} - - -trait FileTypes { +pub trait FileTypes { fn is_immediate(&self) -> bool; fn is_image(&self) -> bool; fn is_video(&self) -> bool; diff --git a/src/main.rs b/src/main.rs index 11a2fc0..e290853 100644 --- a/src/main.rs +++ b/src/main.rs @@ -26,7 +26,6 @@ use dir::Dir; use file::File; use options::{Options, View}; -mod colours; mod dir; mod feature; mod file; diff --git a/src/options.rs b/src/options.rs index adffef1..c9baa2e 100644 --- a/src/options.rs +++ b/src/options.rs @@ -7,10 +7,10 @@ use std::os::unix::fs::MetadataExt; use getopts; use natord; -use colours::Colours; use feature::xattr; use file::File; use output::{Grid, Details, GridDetails, Lines}; +use output::Colours; use output::column::{Columns, TimeTypes, SizeFormat}; use term::dimensions; diff --git a/src/colours.rs b/src/output/colours.rs similarity index 100% rename from src/colours.rs rename to src/output/colours.rs diff --git a/src/output/details.rs b/src/output/details.rs index 0b45b7b..0a65fe6 100644 --- a/src/output/details.rs +++ b/src/output/details.rs @@ -118,12 +118,12 @@ use std::string::ToString; use std::ops::Add; use std::iter::repeat; -use colours::Colours; use dir::Dir; use feature::xattr::{Attribute, FileAttributes}; use file::fields as f; use file::File; use options::{FileFilter, RecurseOptions}; +use output::colours::Colours; use output::column::{Alignment, Column, Columns, SizeFormat}; use output::cell::{TextCell, DisplayWidth}; diff --git a/src/output/grid.rs b/src/output/grid.rs index 69001bf..f944fbb 100644 --- a/src/output/grid.rs +++ b/src/output/grid.rs @@ -1,10 +1,10 @@ -use colours::Colours; -use file::File; -use filetype::file_colour; -use output::DisplayWidth; - use term_grid as grid; +use file::File; +use output::DisplayWidth; +use output::colours::Colours; +use super::file_colour; + #[derive(PartialEq, Debug, Copy, Clone)] pub struct Grid { diff --git a/src/output/lines.rs b/src/output/lines.rs index 39ef2c3..07c4351 100644 --- a/src/output/lines.rs +++ b/src/output/lines.rs @@ -1,9 +1,9 @@ -use colours::Colours; -use file::File; - use ansi_term::ANSIStrings; +use file::File; + use super::filename; +use super::colours::Colours; #[derive(Clone, Copy, Debug, PartialEq)] diff --git a/src/output/mod.rs b/src/output/mod.rs index d1c7e14..f095a79 100644 --- a/src/output/mod.rs +++ b/src/output/mod.rs @@ -1,14 +1,13 @@ use ansi_term::Style; -use colours::Colours; use file::File; -use filetype::file_colour; pub use self::cell::{TextCell, TextCellContents, DisplayWidth}; +pub use self::colours::Colours; pub use self::details::Details; +pub use self::grid_details::GridDetails; pub use self::grid::Grid; pub use self::lines::Lines; -pub use self::grid_details::GridDetails; mod grid; pub mod details; @@ -16,7 +15,7 @@ mod lines; mod grid_details; pub mod column; mod cell; - +mod colours; pub fn filename(file: File, colours: &Colours, links: bool) -> TextCellContents { if links && file.is_link() { @@ -49,3 +48,25 @@ fn symlink_filename(file: File, colours: &Colours) -> TextCellContents { ].into(), } } + +pub fn file_colour(colours: &Colours, file: &File) -> Style { + use filetype::FileTypes; + + match file { + f if f.is_directory() => colours.filetypes.directory, + f if f.is_executable_file() => colours.filetypes.executable, + f if f.is_link() => colours.filetypes.symlink, + f if !f.is_file() => colours.filetypes.special, + f if f.is_immediate() => colours.filetypes.immediate, + f if f.is_image() => colours.filetypes.image, + f if f.is_video() => colours.filetypes.video, + f if f.is_music() => colours.filetypes.music, + f if f.is_lossless() => colours.filetypes.lossless, + f if f.is_crypto() => colours.filetypes.crypto, + f if f.is_document() => colours.filetypes.document, + f if f.is_compressed() => colours.filetypes.compressed, + f if f.is_temp() => colours.filetypes.temp, + f if f.is_compiled() => colours.filetypes.compiled, + _ => colours.filetypes.normal, + } +} \ No newline at end of file From 2e15b812496f9f1cc92da22874cfab3239170ee5 Mon Sep 17 00:00:00 2001 From: Benjamin Sago Date: Tue, 22 Dec 2015 12:15:59 +1100 Subject: [PATCH 7/7] Optimise imports 1. imports from std 2. imports from external crates 3. imports from local modules 4. imports from self --- src/output/details.rs | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/output/details.rs b/src/output/details.rs index 0a65fe6..a31816e 100644 --- a/src/output/details.rs +++ b/src/output/details.rs @@ -113,10 +113,21 @@ use std::error::Error; use std::io; +use std::iter::repeat; +use std::ops::Add; use std::path::PathBuf; use std::string::ToString; -use std::ops::Add; -use std::iter::repeat; + +use ansi_term::Style; + +use datetime::format::DateFormat; +use datetime::local::{LocalDateTime, DatePiece}; +use datetime::zoned::TimeZone; + +use locale; + +use users::{OSUsers, Users}; +use users::mock::MockUsers; use dir::Dir; use feature::xattr::{Attribute, FileAttributes}; @@ -126,18 +137,6 @@ use options::{FileFilter, RecurseOptions}; use output::colours::Colours; use output::column::{Alignment, Column, Columns, SizeFormat}; use output::cell::{TextCell, DisplayWidth}; - -use ansi_term::Style; - -use datetime::local::{LocalDateTime, DatePiece}; -use datetime::format::DateFormat; -use datetime::zoned::TimeZone; - -use locale; - -use users::{OSUsers, Users}; -use users::mock::MockUsers; - use super::filename;