From b6ce907928d872957e3b47f95a9ccda80f1eed9c Mon Sep 17 00:00:00 2001 From: Maxim Zhiburt Date: Wed, 20 Nov 2024 00:31:28 +0300 Subject: [PATCH] =?UTF-8?q?nu-table/=20Do=20footer=5Finheritance=20by=20ac?= =?UTF-8?q?couting=20for=20rows=20rather=20then=20a=20f=E2=80=A6=20(#14380?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So it's my take on the comments in #14060 The change could be seen in this test. Looks like it works :) but I haven't done a lot of testing. https://github.com/zhiburt/nushell/blob/0b1af774157c182675e471e67c184f6080721794/crates/nu-command/tests/commands/table.rs#L3032-L3062 ```nushell $env.config.table.footer_inheritance = true; $env.config.footer_mode = 7; [[a b]; ['kv' {0: [[field]; [0] [1] [2] [3] [4] [5]]} ], ['data' 0], ['data' 0] ] | table --expand --width=80 ``` ```text ╭───┬──────┬───────────────────────╮ │ # │ a │ b │ ├───┼──────┼───────────────────────┤ │ 0 │ kv │ ╭───┬───────────────╮ │ │ │ │ │ │ ╭───┬───────╮ │ │ │ │ │ │ 0 │ │ # │ field │ │ │ │ │ │ │ │ ├───┼───────┤ │ │ │ │ │ │ │ │ 0 │ 0 │ │ │ │ │ │ │ │ │ 1 │ 1 │ │ │ │ │ │ │ │ │ 2 │ 2 │ │ │ │ │ │ │ │ │ 3 │ 3 │ │ │ │ │ │ │ │ │ 4 │ 4 │ │ │ │ │ │ │ │ │ 5 │ 5 │ │ │ │ │ │ │ │ ╰───┴───────╯ │ │ │ │ │ ╰───┴───────────────╯ │ │ 1 │ data │ 0 │ │ 2 │ data │ 0 │ ├───┼──────┼───────────────────────┤ │ # │ a │ b │ ╰───┴──────┴───────────────────────╯ ``` Maybe it will also solve the issue you @fdncred encountered. close #14060 cc: @NotTheDr01ds --- crates/nu-command/src/viewers/table.rs | 2 +- crates/nu-command/tests/commands/table.rs | 120 ++++++++++++++++++++++ crates/nu-table/src/common.rs | 8 +- crates/nu-table/src/table.rs | 4 + crates/nu-table/src/types/expanded.rs | 57 +++++----- crates/nu-table/src/types/general.rs | 13 ++- crates/nu-table/src/types/mod.rs | 31 +----- 7 files changed, 170 insertions(+), 65 deletions(-) diff --git a/crates/nu-command/src/viewers/table.rs b/crates/nu-command/src/viewers/table.rs index a9e7361a9e..50db949ced 100644 --- a/crates/nu-command/src/viewers/table.rs +++ b/crates/nu-command/src/viewers/table.rs @@ -1088,7 +1088,7 @@ fn create_empty_placeholder( let data = vec![vec![cell]]; let mut table = NuTable::from(data); table.set_data_style(TextStyle::default().dimmed()); - let out = TableOutput::new(table, false, false, false); + let out = TableOutput::new(table, false, false, 1); let style_computer = &StyleComputer::from_config(engine_state, stack); let config = create_nu_table_config(&config, style_computer, &out, false, TableMode::default()); diff --git a/crates/nu-command/tests/commands/table.rs b/crates/nu-command/tests/commands/table.rs index 96ae395e47..b9b80b9e87 100644 --- a/crates/nu-command/tests/commands/table.rs +++ b/crates/nu-command/tests/commands/table.rs @@ -2941,3 +2941,123 @@ fn table_footer_inheritance() { assert_eq!(actual.out.match_indices("x2").count(), 1); assert_eq!(actual.out.match_indices("x3").count(), 1); } + +#[test] +fn table_footer_inheritance_kv_rows() { + let actual = nu!( + concat!( + "$env.config.table.footer_inheritance = true;", + "$env.config.footer_mode = 7;", + "[[a b]; ['kv' {0: 0, 1: 1, 2: 2, 3: 3, 4: 4} ], ['data' 0], ['data' 0] ] | table --expand --width=80", + ) + ); + + assert_eq!( + actual.out, + "╭───┬──────┬───────────╮\ + │ # │ a │ b │\ + ├───┼──────┼───────────┤\ + │ 0 │ kv │ ╭───┬───╮ │\ + │ │ │ │ 0 │ 0 │ │\ + │ │ │ │ 1 │ 1 │ │\ + │ │ │ │ 2 │ 2 │ │\ + │ │ │ │ 3 │ 3 │ │\ + │ │ │ │ 4 │ 4 │ │\ + │ │ │ ╰───┴───╯ │\ + │ 1 │ data │ 0 │\ + │ 2 │ data │ 0 │\ + ╰───┴──────┴───────────╯" + ); + + let actual = nu!( + concat!( + "$env.config.table.footer_inheritance = true;", + "$env.config.footer_mode = 7;", + "[[a b]; ['kv' {0: 0, 1: 1, 2: 2, 3: 3, 4: 4, 5: 5} ], ['data' 0], ['data' 0] ] | table --expand --width=80", + ) + ); + + assert_eq!( + actual.out, + "╭───┬──────┬───────────╮\ + │ # │ a │ b │\ + ├───┼──────┼───────────┤\ + │ 0 │ kv │ ╭───┬───╮ │\ + │ │ │ │ 0 │ 0 │ │\ + │ │ │ │ 1 │ 1 │ │\ + │ │ │ │ 2 │ 2 │ │\ + │ │ │ │ 3 │ 3 │ │\ + │ │ │ │ 4 │ 4 │ │\ + │ │ │ │ 5 │ 5 │ │\ + │ │ │ ╰───┴───╯ │\ + │ 1 │ data │ 0 │\ + │ 2 │ data │ 0 │\ + ├───┼──────┼───────────┤\ + │ # │ a │ b │\ + ╰───┴──────┴───────────╯" + ); +} + +#[test] +fn table_footer_inheritance_list_rows() { + let actual = nu!( + concat!( + "$env.config.table.footer_inheritance = true;", + "$env.config.footer_mode = 7;", + "[[a b]; ['kv' {0: [[field]; [0] [1] [2] [3] [4]]} ], ['data' 0], ['data' 0] ] | table --expand --width=80", + ) + ); + + assert_eq!( + actual.out, + "╭───┬──────┬───────────────────────╮\ + │ # │ a │ b │\ + ├───┼──────┼───────────────────────┤\ + │ 0 │ kv │ ╭───┬───────────────╮ │\ + │ │ │ │ │ ╭───┬───────╮ │ │\ + │ │ │ │ 0 │ │ # │ field │ │ │\ + │ │ │ │ │ ├───┼───────┤ │ │\ + │ │ │ │ │ │ 0 │ 0 │ │ │\ + │ │ │ │ │ │ 1 │ 1 │ │ │\ + │ │ │ │ │ │ 2 │ 2 │ │ │\ + │ │ │ │ │ │ 3 │ 3 │ │ │\ + │ │ │ │ │ │ 4 │ 4 │ │ │\ + │ │ │ │ │ ╰───┴───────╯ │ │\ + │ │ │ ╰───┴───────────────╯ │\ + │ 1 │ data │ 0 │\ + │ 2 │ data │ 0 │\ + ╰───┴──────┴───────────────────────╯" + ); + + let actual = nu!( + concat!( + "$env.config.table.footer_inheritance = true;", + "$env.config.footer_mode = 7;", + "[[a b]; ['kv' {0: [[field]; [0] [1] [2] [3] [4] [5]]} ], ['data' 0], ['data' 0] ] | table --expand --width=80", + ) + ); + + assert_eq!( + actual.out, + "╭───┬──────┬───────────────────────╮\ + │ # │ a │ b │\ + ├───┼──────┼───────────────────────┤\ + │ 0 │ kv │ ╭───┬───────────────╮ │\ + │ │ │ │ │ ╭───┬───────╮ │ │\ + │ │ │ │ 0 │ │ # │ field │ │ │\ + │ │ │ │ │ ├───┼───────┤ │ │\ + │ │ │ │ │ │ 0 │ 0 │ │ │\ + │ │ │ │ │ │ 1 │ 1 │ │ │\ + │ │ │ │ │ │ 2 │ 2 │ │ │\ + │ │ │ │ │ │ 3 │ 3 │ │ │\ + │ │ │ │ │ │ 4 │ 4 │ │ │\ + │ │ │ │ │ │ 5 │ 5 │ │ │\ + │ │ │ │ │ ╰───┴───────╯ │ │\ + │ │ │ ╰───┴───────────────╯ │\ + │ 1 │ data │ 0 │\ + │ 2 │ data │ 0 │\ + ├───┼──────┼───────────────────────┤\ + │ # │ a │ b │\ + ╰───┴──────┴───────────────────────╯" + ); +} diff --git a/crates/nu-table/src/common.rs b/crates/nu-table/src/common.rs index d18e053a1c..13c8f84fbc 100644 --- a/crates/nu-table/src/common.rs +++ b/crates/nu-table/src/common.rs @@ -18,8 +18,12 @@ pub fn create_nu_table_config( expand: bool, mode: TableMode, ) -> NuTableConfig { - let with_footer = (config.table.footer_inheritance && out.with_footer) - || with_footer(config, out.with_header, out.table.count_rows()); + let mut count_rows = out.table.count_rows(); + if config.table.footer_inheritance { + count_rows = out.count_rows; + } + + let with_footer = with_footer(config, out.with_header, count_rows); NuTableConfig { theme: load_theme(mode), diff --git a/crates/nu-table/src/table.rs b/crates/nu-table/src/table.rs index 971e5b1f91..bf75e14380 100644 --- a/crates/nu-table/src/table.rs +++ b/crates/nu-table/src/table.rs @@ -615,12 +615,15 @@ fn load_theme( if let Some(style) = sep_color { let color = convert_style(style); let color = ANSIBuf::from(color); + // todo: use .modify(Segment::all(), color) --> it has this optimization table.get_config_mut().set_border_color_default(color); } if !with_header { + // todo: remove and use theme.remove_horizontal_lines(); table.with(RemoveHorizontalLine); } else if with_footer { + // todo: remove and set it on theme rather then here... table.with(CopyFirstHorizontalLineAtLast); } } @@ -1257,6 +1260,7 @@ fn remove_row(recs: &mut NuRecords, row: usize) -> Vec { columns } +// todo; use Format? struct StripColorFromRow(usize); impl TableOption> for StripColorFromRow { diff --git a/crates/nu-table/src/types/expanded.rs b/crates/nu-table/src/types/expanded.rs index a0b6cd0ff2..d87af74522 100644 --- a/crates/nu-table/src/types/expanded.rs +++ b/crates/nu-table/src/types/expanded.rs @@ -5,7 +5,7 @@ use crate::{ NuText, StringResult, TableResult, INDEX_COLUMN_NAME, }, string_width, - types::{has_footer, has_index}, + types::has_index, NuTable, NuTableCell, TableOpts, TableOutput, }; use nu_color_config::{Alignment, StyleComputer, TextStyle}; @@ -63,22 +63,22 @@ struct Cfg<'a> { struct CellOutput { text: String, style: TextStyle, - is_big: bool, + size: usize, is_expanded: bool, } impl CellOutput { - fn new(text: String, style: TextStyle, is_big: bool, is_expanded: bool) -> Self { + fn new(text: String, style: TextStyle, size: usize, is_expanded: bool) -> Self { Self { text, style, - is_big, + size, is_expanded, } } - fn clean(text: String, is_big: bool, is_expanded: bool) -> Self { - Self::new(text, Default::default(), is_big, is_expanded) + fn clean(text: String, size: usize, is_expanded: bool) -> Self { + Self::new(text, Default::default(), size, is_expanded) } fn text(text: String) -> Self { @@ -86,7 +86,7 @@ impl CellOutput { } fn styled(text: NuText) -> Self { - Self::new(text.0, text.1, false, false) + Self::new(text.0, text.1, 1, false) } } @@ -117,7 +117,7 @@ fn expanded_table_list(input: &[Value], cfg: Cfg<'_>) -> TableResult { let with_index = has_index(&cfg.opts, &headers); let row_offset = cfg.opts.index_offset; - let mut is_footer_used = false; + let mut rows_count = 0usize; // The header with the INDEX is removed from the table headers since // it is added to the natural table index @@ -199,9 +199,7 @@ fn expanded_table_list(input: &[Value], cfg: Cfg<'_>) -> TableResult { data[row].push(value); data_styles.insert((row, with_index as usize), cell.style); - if cell.is_big { - is_footer_used = cell.is_big; - } + rows_count = rows_count.saturating_add(cell.size); } let mut table = NuTable::from(data); @@ -209,12 +207,7 @@ fn expanded_table_list(input: &[Value], cfg: Cfg<'_>) -> TableResult { table.set_index_style(get_index_style(cfg.opts.style_computer)); set_data_styles(&mut table, data_styles); - return Ok(Some(TableOutput::new( - table, - false, - with_index, - is_footer_used, - ))); + return Ok(Some(TableOutput::new(table, false, with_index, rows_count))); } if !headers.is_empty() { @@ -269,6 +262,8 @@ fn expanded_table_list(input: &[Value], cfg: Cfg<'_>) -> TableResult { } } + let mut column_rows = 0usize; + for (row, item) in input.iter().enumerate() { cfg.opts.signals.check(cfg.opts.span)?; @@ -294,9 +289,7 @@ fn expanded_table_list(input: &[Value], cfg: Cfg<'_>) -> TableResult { data[row + 1].push(value); data_styles.insert((row + 1, col + with_index as usize), cell.style); - if cell.is_big { - is_footer_used = cell.is_big; - } + column_rows = column_rows.saturating_add(cell.size); } let head_cell = NuTableCell::new(header); @@ -316,6 +309,8 @@ fn expanded_table_list(input: &[Value], cfg: Cfg<'_>) -> TableResult { available_width -= pad_space + column_width; rendered_column += 1; + + rows_count = std::cmp::max(rows_count, column_rows); } if truncate && rendered_column == 0 { @@ -374,9 +369,7 @@ fn expanded_table_list(input: &[Value], cfg: Cfg<'_>) -> TableResult { table.set_indent(cfg.opts.indent.0, cfg.opts.indent.1); set_data_styles(&mut table, data_styles); - let has_footer = is_footer_used || has_footer(&cfg.opts, table.count_rows() as u64); - - Ok(Some(TableOutput::new(table, true, with_index, has_footer))) + Ok(Some(TableOutput::new(table, true, with_index, rows_count))) } fn expanded_table_kv(record: &Record, cfg: Cfg<'_>) -> CellResult { @@ -395,7 +388,7 @@ fn expanded_table_kv(record: &Record, cfg: Cfg<'_>) -> CellResult { let value_width = cfg.opts.width - key_width - count_borders - padding - padding; - let mut with_footer = false; + let mut count_rows = 0usize; let mut data = Vec::with_capacity(record.len()); for (key, value) in record { @@ -420,19 +413,17 @@ fn expanded_table_kv(record: &Record, cfg: Cfg<'_>) -> CellResult { data.push(row); - if cell.is_big { - with_footer = cell.is_big; - } + count_rows = count_rows.saturating_add(cell.size); } let mut table = NuTable::from(data); table.set_index_style(get_key_style(&cfg)); table.set_indent(cfg.opts.indent.0, cfg.opts.indent.1); - let out = TableOutput::new(table, false, true, with_footer); + let out = TableOutput::new(table, false, true, count_rows); maybe_expand_table(out, cfg.opts.width, &cfg.opts) - .map(|value| value.map(|value| CellOutput::clean(value, with_footer, false))) + .map(|value| value.map(|value| CellOutput::clean(value, count_rows, false))) } // the flag is used as an optimization to not do `value.lines().count()` search. @@ -441,7 +432,7 @@ fn expand_table_value(value: &Value, value_width: usize, cfg: &Cfg<'_>) -> CellR if is_limited { return Ok(Some(CellOutput::clean( value_to_string_clean(value, cfg), - false, + 1, false, ))); } @@ -457,7 +448,7 @@ fn expand_table_value(value: &Value, value_width: usize, cfg: &Cfg<'_>) -> CellR let cfg = create_table_cfg(cfg, &out); let value = out.table.draw(cfg, value_width); match value { - Some(value) => Ok(Some(CellOutput::clean(value, out.with_footer, true))), + Some(value) => Ok(Some(CellOutput::clean(value, out.count_rows, true))), None => Ok(None), } } @@ -484,7 +475,7 @@ fn expand_table_value(value: &Value, value_width: usize, cfg: &Cfg<'_>) -> CellR let inner_cfg = update_config(dive_options(cfg, span), value_width); let result = expanded_table_kv(record, inner_cfg)?; match result { - Some(result) => Ok(Some(CellOutput::clean(result.text, result.is_big, true))), + Some(result) => Ok(Some(CellOutput::clean(result.text, result.size, true))), None => Ok(Some(CellOutput::text(value_to_wrapped_string( value, cfg, @@ -575,7 +566,7 @@ fn expanded_table_entry2(item: &Value, cfg: Cfg<'_>) -> CellOutput { let table_config = create_table_cfg(&cfg, &out); let table = out.table.draw(table_config, usize::MAX); match table { - Some(table) => CellOutput::clean(table, out.with_footer, false), + Some(table) => CellOutput::clean(table, out.count_rows, false), None => CellOutput::styled(nu_value_to_string( item, cfg.opts.config, diff --git a/crates/nu-table/src/types/general.rs b/crates/nu-table/src/types/general.rs index ba0a1ceefa..e748b6eff2 100644 --- a/crates/nu-table/src/types/general.rs +++ b/crates/nu-table/src/types/general.rs @@ -56,8 +56,9 @@ fn kv_table(record: &Record, opts: TableOpts<'_>) -> StringResult { let mut table = NuTable::from(data); table.set_index_style(TextStyle::default_field()); + let count_rows = table.count_rows(); - let mut out = TableOutput::new(table, false, true, false); + let mut out = TableOutput::new(table, false, true, count_rows); let left = opts.config.table.padding.left; let right = opts.config.table.padding.right; @@ -82,7 +83,10 @@ fn table(input: &[Value], opts: &TableOpts<'_>) -> TableResult { let with_header = !headers.is_empty(); if !with_header { let table = to_table_with_no_header(input, with_index, row_offset, opts)?; - let table = table.map(|table| TableOutput::new(table, false, with_index, false)); + let table = table.map(|table| { + let count_rows = table.count_rows(); + TableOutput::new(table, false, with_index, count_rows) + }); return Ok(table); } @@ -98,7 +102,10 @@ fn table(input: &[Value], opts: &TableOpts<'_>) -> TableResult { .collect(); let table = to_table_with_header(input, &headers, with_index, row_offset, opts)?; - let table = table.map(|table| TableOutput::new(table, true, with_index, false)); + let table = table.map(|table| { + let count_rows = table.count_rows(); + TableOutput::new(table, true, with_index, count_rows) + }); Ok(table) } diff --git a/crates/nu-table/src/types/mod.rs b/crates/nu-table/src/types/mod.rs index adbe56bf27..829c87ed9e 100644 --- a/crates/nu-table/src/types/mod.rs +++ b/crates/nu-table/src/types/mod.rs @@ -1,8 +1,7 @@ -use terminal_size::{terminal_size, Height, Width}; +use nu_color_config::StyleComputer; +use nu_protocol::{Config, Signals, Span, TableIndexMode, TableMode}; use crate::{common::INDEX_COLUMN_NAME, NuTable}; -use nu_color_config::StyleComputer; -use nu_protocol::{Config, FooterMode, Signals, Span, TableIndexMode, TableMode}; mod collapse; mod expanded; @@ -16,16 +15,16 @@ pub struct TableOutput { pub table: NuTable, pub with_header: bool, pub with_index: bool, - pub with_footer: bool, + pub count_rows: usize, } impl TableOutput { - pub fn new(table: NuTable, with_header: bool, with_index: bool, with_footer: bool) -> Self { + pub fn new(table: NuTable, with_header: bool, with_index: bool, count_rows: usize) -> Self { Self { table, with_header, with_index, - with_footer, + count_rows, } } } @@ -79,23 +78,3 @@ fn has_index(opts: &TableOpts<'_>, headers: &[String]) -> bool { with_index && !opts.index_remove } - -fn has_footer(opts: &TableOpts<'_>, count_records: u64) -> bool { - match opts.config.footer_mode { - // Only show the footer if there are more than RowCount rows - FooterMode::RowCount(limit) => count_records > limit, - // Always show the footer - FooterMode::Always => true, - // Never show the footer - FooterMode::Never => false, - // Calculate the screen height and row count, if screen height is larger than row count, don't show footer - FooterMode::Auto => { - let (_width, height) = match terminal_size() { - Some((w, h)) => (Width(w.0).0 as u64, Height(h.0).0 as u64), - None => (Width(0).0 as u64, Height(0).0 as u64), - }; - - height <= count_records - } - } -}