simplify the min_columns calculation and add edge cases tests

emar-kar · emar-kar · commit c417ae07791a · 2025-04-09T11:01:40.000+01:00
diff --git a/src/lib.rs b/src/lib.rs
@@ -196,30 +196,34 @@ impl<T: AsRef<str>> Grid<T> {
             };
         }
 
-        // 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::<usize>() <= 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::<usize>() <= self.options.width - new_width {
+                potential_dimension = new_dimension;
+            }
         }
 
         potential_dimension
diff --git 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 {
