Merge pull request #92 from Nukesor/fix-dynamic-calculation

Fix dynamic calculation

.github/workflows/test.yml

@@ -4,17 +4,17 @@ on:
   push:
     branches: [main]
     paths:
-      - '.github/workflows/test.yml'
-      - '**.rs'
-      - 'Cargo.toml'
-      - 'Cargo.lock'
+      - ".github/workflows/test.yml"
+      - "**.rs"
+      - "Cargo.toml"
+      - "Cargo.lock"
   pull_request:
     branches: [main]
     paths:
-      - '.github/workflows/test.yml'
-      - '**.rs'
-      - 'Cargo.toml'
-      - 'Cargo.lock'
+      - ".github/workflows/test.yml"
+      - "**.rs"
+      - "Cargo.toml"
+      - "Cargo.lock"
 
 jobs:
   test:
@@ -23,11 +23,11 @@ jobs:
     strategy:
       matrix:
         target:
-            - x86_64-unknown-linux-gnu
-            - x86_64-pc-windows-msvc
-            - x86_64-apple-darwin
-            - wasm32-wasi
-        toolchain: [stable, nightly, 1.59]
+          - x86_64-unknown-linux-gnu
+          - x86_64-pc-windows-msvc
+          - x86_64-apple-darwin
+          - wasm32-wasi
+        toolchain: [stable, nightly, 1.62]
         include:
           - target: x86_64-unknown-linux-gnu
             os: ubuntu-latest
@@ -42,10 +42,9 @@ jobs:
             os: macos-latest
             minimal_setup: false
 
-# minimal_setup: This is needed for targets that don't support our dev dependencies.
-#                It also excludes the default features, i.e. [tty].
-#                For instance, "wasm32-wasi" is such a target.
-
+    # minimal_setup: This is needed for targets that don't support our dev dependencies.
+    #                It also excludes the default features, i.e. [tty].
+    #                For instance, "wasm32-wasi" is such a target.
     steps:
       - name: Checkout code
         uses: actions/checkout@v3
@@ -62,7 +61,7 @@ jobs:
         if: ${{ !matrix.minimal_setup }}
 
       - name: cargo test
-        run: cargo test --target=${{ matrix.target }}
+        run: cargo test --target=${{ matrix.target }} --features=integration_test
         if: ${{ !matrix.minimal_setup }}
 
       - name: cargo build without default features and without dev dependencies

CHANGELOG.md

@@ -1,8 +1,26 @@
 # Changelog
+
 All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [6.1.1] - unreleased
+
+### Fixed
+
+- Fixed an issue where dynamic arrangement failed when setting the table to the exact width of the content [#90](https://github.com/Nukesor/comfy-table/issues/90).
+- The header size is now properly respected in the final optimization step [#90](https://github.com/Nukesor/comfy-table/issues/90).
+    Previously, this wasn't the case and lead to weird formatting behavior when both of the following were true
+  - Dynamic content adjustment was active.
+  - The table didn't fit into the the available space.
+  - The header of a row was longer than its content.
+- Fix wrong LowerBoundary calculation. This was introduced in commit bee764d, when this logic was refactored. [#90](https://github.com/Nukesor/comfy-table/issues/90).
+- `Table::column_iter` no longer requires a `&mut self`, but only `&self`.
+
+### Added
+
+- Expose current ContentArrangement for table via `table.content_arrangement`.
+
 ## [6.1.0] - 2022-08-28
 
 ### Added

Cargo.toml

@@ -9,7 +9,7 @@ documentation = "https://docs.rs/comfy-table/"
 license = "MIT"
 keywords = ["terminal", "table", "unicode"]
 readme = "README.md"
-rust-version = "1.59"
+rust-version = "1.62"
 edition = "2021"
 
 [badges]
@@ -36,6 +36,8 @@ default = ["tty"]
 tty = ["crossterm"]
 # This flag is for library debugging only!
 debug = []
+# This feature is used to expose internal functionality for integration testing.
+integration_test= []
 
 [dependencies]
 crossterm = { version = "0.25", optional = true }
@@ -49,3 +51,4 @@ doc-comment = "0.3"
 proptest = "1"
 criterion = "0.4"
 rand = "0.8"
+rstest = "0.15"

src/lib.rs

@@ -4,6 +4,12 @@ mod column;
 mod row;
 mod style;
 mod table;
+#[cfg(feature = "integration_test")]
+/// We publicly expose the internal [utils] module for our integration tests.
+/// There's some logic we need from inside here.
+/// The API inside of this isn't considered stable and shouldnt' be used.
+pub mod utils;
+#[cfg(not(feature = "integration_test"))]
 mod utils;
 
 pub use crate::cell::{Cell, Cells};

src/style/column.rs

@@ -10,7 +10,8 @@ pub enum ColumnConstraint {
     /// This will completely hide a column.
     Hidden,
     /// Force the column to be as long as it's content.
-    /// Use with caution! This can easily break your table, if the column's content is overly long.
+    /// Use with caution! This can easily mess up your table formatting,
+    /// if a column's content is overly long.
     ContentWidth,
     /// Enforce a absolute width for a column.
     Absolute(Width),
@@ -25,13 +26,10 @@ pub enum ColumnConstraint {
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 pub enum Width {
     /// A fixed amount of characters.
-    /// This can be used to specify an upper/lower boundary as well as a fixed size for the column.
     Fixed(u16),
     /// A width equivalent to a certain percentage of the available width.
     /// Values above 100 will be automatically reduced to 100.
     ///
-    /// This can be used to specify an upper/lower boundary as well as a fixed size for the column.
-    ///
     /// **Warning:** This option will be ignored if:
     /// - you aren't using one of ContentArrangement::{Dynamic, DynamicFullWidth}
     /// - the width of the table/terminal cannot be determined.

src/table.rs

@@ -212,6 +212,11 @@ impl Table {
         self
     }
 
+    /// Get the current content arrangement of the table.
+    pub fn content_arrangement(&self) -> ContentArrangement {
+        self.arrangement.clone()
+    }
+
     /// Set the delimiter used to split text in all cells.
     ///
     /// A custom delimiter on a cell in will overwrite the column's delimiter.\
@@ -502,7 +507,7 @@ impl Table {
     }
 
     /// Iterator over all columns
-    pub fn column_iter(&mut self) -> Iter<Column> {
+    pub fn column_iter(&self) -> Iter<Column> {
         self.columns.iter()
     }
 
@@ -560,6 +565,37 @@ impl Table {
         }
     }
 
+    /// Get a mutable iterator over cells of a column, including the header cell.
+    /// The header cell will be the very first cell returned.
+    /// The iterator returns a nested Option<Option<Cell>>, since there might be
+    /// rows that are missing this specific Cell.
+    ///
+    /// ```
+    /// use comfy_table::Table;
+    /// let mut table = Table::new();
+    /// table.set_header(&vec!["A", "B"]);
+    /// table.add_row(&vec!["First", "Second"]);
+    /// table.add_row(&vec!["Third"]);
+    /// table.add_row(&vec!["Fourth", "Fifth"]);
+    ///
+    /// // Create an iterator over the second column
+    /// let mut cell_iter = table.column_cells_with_header_iter(1);
+    /// assert_eq!(cell_iter.next().unwrap().unwrap().content(), "B");
+    /// assert_eq!(cell_iter.next().unwrap().unwrap().content(), "Second");
+    /// assert!(cell_iter.next().unwrap().is_none());
+    /// assert_eq!(cell_iter.next().unwrap().unwrap().content(), "Fifth");
+    /// assert!(cell_iter.next().is_none());
+    /// ```
+    pub fn column_cells_with_header_iter(&self, column_index: usize) -> ColumnCellsWithHeaderIter {
+        ColumnCellsWithHeaderIter {
+            header_checked: false,
+            header: &self.header,
+            rows: &self.rows,
+            column_index,
+            row_index: 0,
+        }
+    }
+
     /// Reference to a specific row
     pub fn row(&self, index: usize) -> Option<&Row> {
         self.rows.get(index)
@@ -679,12 +715,47 @@ impl<'a> Iterator for ColumnCellIter<'a> {
         if let Some(row) = self.rows.get(self.row_index) {
             self.row_index += 1;
 
-            // Check if the row has the requested column.
-            if let Some(cell) = row.cells.get(self.column_index) {
-                return Some(Some(cell));
-            }
+            // Return the cell (if it exists).
+            return Some(row.cells.get(self.column_index));
+        }
+
+        None
+    }
+}
+
+/// An iterator over cells of a specific column.
+/// A dedicated struct is necessary, as data is usually handled by rows and thereby stored in
+/// `Table::rows`. This type is returned by [Table::column_cells_iter].
+pub struct ColumnCellsWithHeaderIter<'a> {
+    header_checked: bool,
+    header: &'a Option<Row>,
+    rows: &'a [Row],
+    column_index: usize,
+    row_index: usize,
+}
+
+impl<'a> Iterator for ColumnCellsWithHeaderIter<'a> {
+    type Item = Option<&'a Cell>;
+    fn next(&mut self) -> Option<Option<&'a Cell>> {
+        // Get the header as the first cell
+        if !self.header_checked {
+            self.header_checked = true;
+
+            return match self.header {
+                Some(header) => {
+                    // Return the cell (if it exists).
+                    Some(header.cells.get(self.column_index))
+                }
+                None => Some(None),
+            };
+        }
+
+        // Check if there's a next row
+        if let Some(row) = self.rows.get(self.row_index) {
+            self.row_index += 1;
 
-            return Some(None);
+            // Return the cell (if it exists).
+            return Some(row.cells.get(self.column_index));
         }
 
         None

src/utils/arrangement/constraint.rs

@@ -13,7 +13,6 @@ use crate::{Column, Table};
 /// - The Column is supposed to be hidden.
 pub fn evaluate(
     table: &Table,
-    table_width: Option<usize>,
     visible_columns: usize,
     infos: &mut DisplayInfos,
     column: &Column,
@@ -25,9 +24,7 @@ pub fn evaluate(
             infos.insert(column.index, info);
         }
         Some(Absolute(width)) => {
-            if let Some(width) =
-                absolute_value_from_width(table, width, visible_columns, table_width)
-            {
+            if let Some(width) = absolute_value_from_width(table, width, visible_columns) {
                 // The column should get always get a fixed width.
                 let width = absolute_width_with_padding(column, width);
                 let info = ColumnDisplayInfo::new(column, width);
@@ -42,10 +39,11 @@ pub fn evaluate(
         _ => {}
     }
 
-    if let Some(min_width) = min(table, &column.constraint, table_width, visible_columns) {
+    if let Some(min_width) = min(table, &column.constraint, visible_columns) {
         // In case a min_width is specified, we may already fix the size of the column.
         // We do this, if we know that the content is smaller than the min size.
-        if max_content_width <= min_width {
+        let max_width = max_content_width + column.padding_width();
+        if max_width <= min_width {
             let width = absolute_width_with_padding(column, min_width);
             let info = ColumnDisplayInfo::new(column, width);
             infos.insert(column.index, info);
@@ -63,7 +61,6 @@ pub fn evaluate(
 pub fn min(
     table: &Table,
     constraint: &Option<ColumnConstraint>,
-    table_width: Option<usize>,
     visible_columns: usize,
 ) -> Option<u16> {
     let constraint = if let Some(constraint) = constraint {
@@ -74,7 +71,7 @@ pub fn min(
 
     match constraint {
         LowerBoundary(width) | Boundaries { lower: width, .. } => {
-            absolute_value_from_width(table, width, visible_columns, table_width)
+            absolute_value_from_width(table, width, visible_columns)
         }
         _ => None,
     }
@@ -90,7 +87,6 @@ pub fn min(
 pub fn max(
     table: &Table,
     constraint: &Option<ColumnConstraint>,
-    table_width: Option<usize>,
     visible_columns: usize,
 ) -> Option<u16> {
     let constraint = if let Some(constraint) = constraint {
@@ -101,7 +97,7 @@ pub fn max(
 
     match constraint {
         UpperBoundary(width) | Boundaries { upper: width, .. } => {
-            absolute_value_from_width(table, width, visible_columns, table_width)
+            absolute_value_from_width(table, width, visible_columns)
         }
         _ => None,
     }
@@ -112,13 +108,12 @@ pub fn absolute_value_from_width(
     table: &Table,
     width: &Width,
     visible_columns: usize,
-    table_width: Option<usize>,
 ) -> Option<u16> {
     match width {
         Width::Fixed(width) => Some(*width),
         Width::Percentage(percent) => {
             // Don't return a value, if we cannot determine the current table width.
-            let table_width = table_width?;
+            let table_width = table.width().map(usize::from)?;
 
             // Enforce at most 100%
             let percent = std::cmp::min(*percent, 100u16);

src/utils/arrangement/disabled.rs

@@ -20,7 +20,7 @@ pub fn arrange(
         let mut width = max_content_widths[column.index];
 
         // Reduce the width, if a column has longer content than the specified MaxWidth constraint.
-        if let Some(max_width) = constraint::max(table, &column.constraint, None, visible_columns) {
+        if let Some(max_width) = constraint::max(table, &column.constraint, visible_columns) {
             if max_width < width {
                 width = absolute_width_with_padding(column, max_width);
             }