From f02c1f31f00eb94b8a37b9fdcf14693758c82460 Mon Sep 17 00:00:00 2001 From: Leonid Emar-Kar Date: Sun, 30 Mar 2025 12:58:00 +0100 Subject: [PATCH 1/7] optimize padding size calc --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index b6a0d68..33719bd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -319,7 +319,7 @@ impl> fmt::Display for Grid { let contents = &self.cells[current]; let width = self.widths[current]; let col_width = self.dimensions.widths[x]; - let padding_size = col_width - width; + let padding_size = col_width - width + separator.len(); // The final column doesn’t need to have trailing spaces, // as long as it’s left-aligned. From 43ceb3715f0ced7688334318ddf87ddee3487ca2 Mon Sep 17 00:00:00 2001 From: Leonid Emar-Kar Date: Sun, 30 Mar 2025 13:34:39 +0100 Subject: [PATCH 2/7] remove separator duplication to reduce memory usage --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 33719bd..b6a0d68 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -319,7 +319,7 @@ impl> fmt::Display for Grid { let contents = &self.cells[current]; let width = self.widths[current]; let col_width = self.dimensions.widths[x]; - let padding_size = col_width - width + separator.len(); + let padding_size = col_width - width; // The final column doesn’t need to have trailing spaces, // as long as it’s left-aligned. From bba542a97a06a92114cf4df4c3944aab4b42da2e Mon Sep 17 00:00:00 2001 From: Leonid Emar-Kar Date: Sun, 6 Apr 2025 11:30:26 +0100 Subject: [PATCH 3/7] rework dimensions calculations --- src/lib.rs | 114 ++++++++++++++++------------------------------------- 1 file changed, 35 insertions(+), 79 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index b6a0d68..52c8098 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -113,7 +113,6 @@ impl> Grid { pub fn new(cells: Vec, options: GridOptions) -> Self { let widths: Vec = cells.iter().map(|c| ansi_width(c.as_ref())).collect(); let widest_cell_width = widths.iter().copied().max().unwrap_or(0); - let width = options.width; let mut grid = Self { options, @@ -126,10 +125,9 @@ impl> Grid { }, }; - grid.dimensions = grid.width_dimensions(width).unwrap_or(Dimensions { - num_lines: grid.cells.len(), - widths: vec![widest_cell_width], - }); + if !grid.cells.is_empty() { + grid.dimensions = grid.width_dimensions(); + } grid } @@ -179,94 +177,52 @@ impl> Grid { } } - fn theoretical_max_num_lines(&self, maximum_width: usize) -> usize { - // TODO: Make code readable / efficient. - let mut widths = self.widths.clone(); - - // Sort widths in reverse order - widths.sort_unstable_by(|a, b| b.cmp(a)); - - let mut col_total_width_so_far = 0; - for (i, &width) in widths.iter().enumerate() { - let adjusted_width = if i == 0 { - width - } else { - width + self.options.filling.width() - }; - if col_total_width_so_far + adjusted_width <= maximum_width { - col_total_width_so_far += adjusted_width; - } else { - return div_ceil(self.cells.len(), i); - } - } - - // If we make it to this point, we have exhausted all cells before - // reaching the maximum width; the theoretical max number of lines - // needed to display all cells is 1. - 1 - } - - fn width_dimensions(&self, maximum_width: usize) -> Option { - if self.widest_cell_width > maximum_width { - // Largest cell is wider than maximum width; it is impossible to fit. - return None; - } - - if self.cells.is_empty() { - return Some(Dimensions { - num_lines: 0, - widths: Vec::new(), - }); - } - + fn width_dimensions(&mut self) -> Dimensions { if self.cells.len() == 1 { let cell_widths = self.widths[0]; - return Some(Dimensions { + return Dimensions { num_lines: 1, widths: vec![cell_widths], - }); + }; } - let theoretical_max_num_lines = self.theoretical_max_num_lines(maximum_width); - if theoretical_max_num_lines == 1 { - // This if—statement is necessary for the function to work correctly - // for small inputs. - return Some(Dimensions { - num_lines: 1, - widths: self.widths.clone(), - }); + // Calculate widest column size with separator. + let widest_column = self.widest_cell_width + self.options.filling.width(); + // If it exceeds terminal's width, return, since it is impossible to fit. + if widest_column > self.options.width { + return Dimensions { + num_lines: self.cells.len(), + widths: vec![self.widest_cell_width], + }; } - // Instead of numbers of columns, try to find the fewest number of *lines* - // that the output will fit in. - let mut smallest_dimensions_yet = None; - for num_lines in (1..=theoretical_max_num_lines).rev() { - // The number of columns is the number of cells divided by the number - // of lines, *rounded up*. - let num_columns = div_ceil(self.cells.len(), num_lines); - - // Early abort: if there are so many columns that the width of the - // *column separators* is bigger than the width of the screen, then - // don’t even try to tabulate it. - // This is actually a necessary check, because the width is stored as - // a usize, and making it go negative makes it huge instead, but it - // also serves as a speed-up. - let total_separator_width = (num_columns - 1) * self.options.filling.width(); - if maximum_width < total_separator_width { - continue; - } - // Remove the separator width from the available space. - let adjusted_width = maximum_width - total_separator_width; + // Calculate number of columns with widest column size. + let max_num_columns = self.options.width / widest_column; + + // Caculate approximate number of lines and columns. + let appr_num_lines = div_ceil(self.cells.len(), max_num_columns); + let appr_num_columns = div_ceil(self.cells.len(), appr_num_lines); + + // This is a potential dimension, which can definitely fit all of the cells. + let mut potential_dimension = self.compute_dimensions(appr_num_lines, appr_num_columns); + // If all of the cells can be fit on one line, return. + if appr_num_lines == 1 { + return potential_dimension; + } - let potential_dimensions = self.compute_dimensions(num_lines, num_columns); - if potential_dimensions.widths.iter().sum::() <= adjusted_width { - smallest_dimensions_yet = Some(potential_dimensions); + // Try to increase number of columns, to see if new dimension can still fit. + for num_columns in appr_num_columns + 1.. { + let new_width = self.options.width - (num_columns - 1) * self.options.filling.width(); + let num_lines = div_ceil(self.cells.len(), num_columns); + let new_dimension = self.compute_dimensions(num_lines, num_columns); + if new_dimension.widths.iter().sum::() <= new_width { + potential_dimension = new_dimension; } else { break; } } - smallest_dimensions_yet + potential_dimension } } From 2aa401b5d0d15d9b5aa19166b4963b8da6c5e9d1 Mon Sep 17 00:00:00 2001 From: Leonid Emar-Kar Date: Sun, 6 Apr 2025 11:30:40 +0100 Subject: [PATCH 4/7] add max possible width test --- tests/test.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/test.rs b/tests/test.rs index b1a0db4..52d27d6 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -325,6 +325,26 @@ fn different_size_separator_with_tabs() { assert_eq!(grid.to_string(), bits); } +#[test] +fn use_max_possible_width() { + let grid = Grid::new( + vec![ + "test1", "test2", "test3", "test4", "test5", "test6", "test7", "test8", "test9", + "test10", "test11", + ], + GridOptions { + filling: Filling::Text("||".to_string()), + direction: Direction::LeftToRight, + width: 69, + }, + ); + + let bits = "test1 ||test2 ||test3||test4||test5||test6||test7||test8||test9\ntest10||test11\n"; + + assert_eq!(grid.to_string(), bits); + assert_eq!(grid.row_count(), 2); +} + // These test are based on the tests in uutils ls, to ensure we won't break // it while editing this library. mod uutils_ls { From c417ae07791a04b385f7c6e2c83c0d5c7148db92 Mon Sep 17 00:00:00 2001 From: Leonid Emar-Kar Date: Wed, 9 Apr 2025 11:01:40 +0100 Subject: [PATCH 5/7] simplify the min_columns calculation and add edge cases tests --- src/lib.rs | 34 +++++++++++++++++++--------------- tests/test.rs | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 15 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 52c8098..91a3a4a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -196,30 +196,34 @@ impl> Grid { }; } - // Calculate number of columns with widest column size. - let max_num_columns = self.options.width / widest_column; - - // Caculate approximate number of lines and columns. - let appr_num_lines = div_ceil(self.cells.len(), max_num_columns); - let appr_num_columns = div_ceil(self.cells.len(), appr_num_lines); + // Calculate minimum number of columns with the widest column size. + let min_columns = self + .cells + .len() + .min((self.options.width + self.options.filling.width()) / widest_column); + // Caculate approximate number of rows and columns. + let min_lines = div_ceil(self.cells.len(), min_columns); + // let appr_num_columns = div_ceil(self.cells.len(), appr_num_lines); // This is a potential dimension, which can definitely fit all of the cells. - let mut potential_dimension = self.compute_dimensions(appr_num_lines, appr_num_columns); + let mut potential_dimension = self.compute_dimensions(min_lines, min_columns); // If all of the cells can be fit on one line, return. - if appr_num_lines == 1 { + if min_lines == 1 { return potential_dimension; } // Try to increase number of columns, to see if new dimension can still fit. - for num_columns in appr_num_columns + 1.. { - let new_width = self.options.width - (num_columns - 1) * self.options.filling.width(); - let num_lines = div_ceil(self.cells.len(), num_columns); - let new_dimension = self.compute_dimensions(num_lines, num_columns); - if new_dimension.widths.iter().sum::() <= new_width { - potential_dimension = new_dimension; - } else { + for num_columns in min_columns + 1.. { + let new_width = (num_columns - 1) * self.options.filling.width(); + if new_width > self.options.width { break; } + + let num_rows = div_ceil(self.cells.len(), num_columns); + let new_dimension = self.compute_dimensions(num_rows, num_columns); + if new_dimension.widths.iter().sum::() <= self.options.width - new_width { + potential_dimension = new_dimension; + } } potential_dimension diff --git a/tests/test.rs b/tests/test.rs index 52d27d6..21b2c7f 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -345,6 +345,40 @@ fn use_max_possible_width() { assert_eq!(grid.row_count(), 2); } +#[test] +fn use_minimal_optimal_lines() { + let grid = Grid::new( + vec!["a", "b", "ccc", "ddd"], + GridOptions { + direction: Direction::TopToBottom, + filling: Filling::Spaces(2), + width: 6, + }, + ); + + let expected = "a ccc\nb ddd\n"; + assert_eq!(grid.to_string(), expected); +} + +#[test] +fn weird_column_edge_case() { + let grid = Grid::new( + vec!["0", "1", "222222222", "333333333", "4", "5", "6", "7", "8"], + GridOptions { + direction: Direction::TopToBottom, + filling: Filling::Spaces(2), + width: 21, + }, + ); + + let expected = "\ + 0 222222222 4 6 8\n\ + 1 333333333 5 7\n\ + "; + + assert_eq!(grid.to_string(), expected); +} + // These test are based on the tests in uutils ls, to ensure we won't break // it while editing this library. mod uutils_ls { From 5ec4dc5be951b48a5f5c23f9e7bcf77bea0bcec5 Mon Sep 17 00:00:00 2001 From: Leonid Emar-Kar Date: Wed, 9 Apr 2025 11:45:32 +0100 Subject: [PATCH 6/7] fix some comments and remove leftovers --- src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 91a3a4a..f28af60 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -201,9 +201,8 @@ impl> Grid { .cells .len() .min((self.options.width + self.options.filling.width()) / widest_column); - // Caculate approximate number of rows and columns. + // Caculate minimum number of rows. let min_lines = div_ceil(self.cells.len(), min_columns); - // let appr_num_columns = div_ceil(self.cells.len(), appr_num_lines); // This is a potential dimension, which can definitely fit all of the cells. let mut potential_dimension = self.compute_dimensions(min_lines, min_columns); From b2d435bc8161ba2c019a0960fed7640a6820ed0e Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Wed, 9 Apr 2025 23:11:15 +0200 Subject: [PATCH 7/7] small improvements to variable names and comments --- src/lib.rs | 45 +++++++++++++++++++++++++-------------------- tests/test.rs | 23 +++++++++++++++++++++++ 2 files changed, 48 insertions(+), 20 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index f28af60..ea46e8c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -79,7 +79,7 @@ pub struct GridOptions { #[derive(PartialEq, Eq, Debug)] struct Dimensions { /// The number of lines in the grid. - num_lines: usize, + num_rows: usize, /// The width of each column in the grid. The length of this vector serves /// as the number of columns. @@ -120,7 +120,7 @@ impl> Grid { widths, widest_cell_width, dimensions: Dimensions { - num_lines: 0, + num_rows: 0, widths: Vec::new(), }, }; @@ -140,7 +140,7 @@ impl> Grid { /// The number of rows this display takes up. pub fn row_count(&self) -> usize { - self.dimensions.num_lines + self.dimensions.num_rows } /// The width of each column @@ -172,7 +172,7 @@ impl> Grid { } Dimensions { - num_lines, + num_rows: num_lines, widths: column_widths, } } @@ -181,7 +181,7 @@ impl> Grid { if self.cells.len() == 1 { let cell_widths = self.widths[0]; return Dimensions { - num_lines: 1, + num_rows: 1, widths: vec![cell_widths], }; } @@ -191,36 +191,41 @@ impl> Grid { // If it exceeds terminal's width, return, since it is impossible to fit. if widest_column > self.options.width { return Dimensions { - num_lines: self.cells.len(), + num_rows: self.cells.len(), widths: vec![self.widest_cell_width], }; } - // Calculate minimum number of columns with the widest column size. + // Calculate the number of columns if all columns had the size of the largest + // column. This is a lower bound on the number of columns. let min_columns = self .cells .len() .min((self.options.width + self.options.filling.width()) / widest_column); - // Caculate minimum number of rows. - let min_lines = div_ceil(self.cells.len(), min_columns); + + // Calculate maximum number of lines and columns. + let max_rows = div_ceil(self.cells.len(), min_columns); // This is a potential dimension, which can definitely fit all of the cells. - let mut potential_dimension = self.compute_dimensions(min_lines, min_columns); - // If all of the cells can be fit on one line, return. - if min_lines == 1 { + let mut potential_dimension = self.compute_dimensions(max_rows, min_columns); + + // If all of the cells can be fit on one line, return immediately. + if max_rows == 1 { return potential_dimension; } // Try to increase number of columns, to see if new dimension can still fit. - for num_columns in min_columns + 1.. { - let new_width = (num_columns - 1) * self.options.filling.width(); - if new_width > self.options.width { + for num_columns in min_columns + 1..self.cells.len() { + let Some(adjusted_width) = self + .options + .width + .checked_sub((num_columns - 1) * self.options.filling.width()) + else { break; - } - + }; let num_rows = div_ceil(self.cells.len(), num_columns); let new_dimension = self.compute_dimensions(num_rows, num_columns); - if new_dimension.widths.iter().sum::() <= self.options.width - new_width { + if new_dimension.widths.iter().sum::() <= adjusted_width { potential_dimension = new_dimension; } } @@ -252,7 +257,7 @@ impl> fmt::Display for Grid { // get exactly right. let padding = " ".repeat(self.widest_cell_width + self.options.filling.width()); - for y in 0..self.dimensions.num_lines { + for y in 0..self.dimensions.num_rows { // Current position on the line. let mut cursor: usize = 0; for x in 0..self.dimensions.widths.len() { @@ -261,7 +266,7 @@ impl> fmt::Display for Grid { let (current, offset) = match self.options.direction { Direction::LeftToRight => (y * self.dimensions.widths.len() + x, 1), Direction::TopToBottom => { - (y + self.dimensions.num_lines * x, self.dimensions.num_lines) + (y + self.dimensions.num_rows * x, self.dimensions.num_rows) } }; diff --git a/tests/test.rs b/tests/test.rs index 21b2c7f..d1a61a1 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -345,6 +345,26 @@ fn use_max_possible_width() { assert_eq!(grid.row_count(), 2); } +#[test] +fn dont_use_max_possible_width() { + let grid = Grid::new( + vec![ + "test1", "test2", "test3", "test4", "test5", "test6", "test7", "test8", "test9", + "test10", "test11", + ], + GridOptions { + filling: Filling::Text("||".to_string()), + direction: Direction::TopToBottom, + width: 69, + }, + ); + + let bits = "test1||test3||test5||test7||test9 ||test11\ntest2||test4||test6||test8||test10\n"; + + assert_eq!(grid.to_string(), bits); + assert_eq!(grid.row_count(), 2); +} + #[test] fn use_minimal_optimal_lines() { let grid = Grid::new( @@ -362,6 +382,8 @@ fn use_minimal_optimal_lines() { #[test] fn weird_column_edge_case() { + // Here, 5 columns fit while fewer columns don't. So if we exit too early + // while increasing columns, we don't find the optimal solution. let grid = Grid::new( vec!["0", "1", "222222222", "333333333", "4", "5", "6", "7", "8"], GridOptions { @@ -376,6 +398,7 @@ fn weird_column_edge_case() { 1 333333333 5 7\n\ "; + println!("{grid}"); assert_eq!(grid.to_string(), expected); }