From cb0c6934cfe55e9155ec5029daacc4941009e661 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Mar 2026 21:40:31 +0000 Subject: [PATCH 1/6] refactor: move coloring logic from methods.rs into coloring.rs Address the TODO items from the --color PR by: - Moving the `colored` and `tree` code blocks from `visualizer/methods.rs` into a new `maybe_color_tree_slice` method on `Coloring` in `visualizer/coloring.rs`. - Tightening visibility: `Color::ansi_prefix` and `AnsiPrefix` are now private to coloring.rs, and `ColoredTreeHorizontalSlice` fields are now private (struct remains pub(super) for enum variant visibility). - Removing the now-unnecessary `ColoredTreeHorizontalSlice` import from methods.rs. https://claude.ai/code/session_012139SmUSC1oYSKNkvsg78g --- src/visualizer/coloring.rs | 35 +++++++++++++++++++---------------- src/visualizer/methods.rs | 34 ++++++++++++++-------------------- 2 files changed, 33 insertions(+), 36 deletions(-) diff --git a/src/visualizer/coloring.rs b/src/visualizer/coloring.rs index 430c0d05..15ba3845 100644 --- a/src/visualizer/coloring.rs +++ b/src/visualizer/coloring.rs @@ -17,18 +17,27 @@ impl<'a> Coloring<'a> { Coloring { ls_colors, map } } - /// Return `(color, ls_colors)` for a node, used to build a colored slice for rendering. - pub(super) fn node_color( + /// Look up the color for a node identified by its path components and whether it has children, + /// then wrap the given [`TreeHorizontalSlice`] in the appropriate colored or colorless variant. + pub(super) fn maybe_color_tree_slice( &self, path_components: &[&'a OsStr], has_children: bool, - ) -> Option<(Color, &LsColors)> { + slice: TreeHorizontalSlice, + ) -> MaybeColoredTreeHorizontalSlice<'_> { let color = if has_children { Some(Color::Directory) } else { self.map.get(path_components).copied() - }?; - Some((color, &self.ls_colors)) + }; + match color { + Some(color) => MaybeColoredTreeHorizontalSlice::Colorful(ColoredTreeHorizontalSlice { + slice, + color, + ls_colors: &self.ls_colors, + }), + None => MaybeColoredTreeHorizontalSlice::Colorless(slice), + } } } @@ -46,19 +55,15 @@ pub enum Color { } impl Color { - // TODO: reconsider the visibility of this function once the TODOs in - // `visualizer/methods.rs` have been dealt with. /// Get the ANSI prefix for this color from the given prefix table. - pub(super) fn ansi_prefix(self, prefixes: &LsColors) -> AnsiPrefix<'_> { + fn ansi_prefix(self, prefixes: &LsColors) -> AnsiPrefix<'_> { AnsiPrefix(prefixes.prefix_str(self)) } } -// TODO: reconsider the of this struct once the TODOs in -// `visualizer/methods.rs` have been dealt with. /// ANSI prefix wrapper for a [`Color`] variant, implements [`Display`]. #[derive(Display)] -pub(super) struct AnsiPrefix<'a>(&'a str); +struct AnsiPrefix<'a>(&'a str); impl AnsiPrefix<'_> { /// Returns the reset suffix to emit after this prefix, or `""` if no prefix. @@ -73,11 +78,9 @@ impl AnsiPrefix<'_> { /// A [`TreeHorizontalSlice`] with its color applied, used for rendering. pub(super) struct ColoredTreeHorizontalSlice<'a> { - // TODO: reconsider the following visibilities once the TODOs in - // `visualizer/methods.rs` have been dealt with. - pub(super) slice: TreeHorizontalSlice, - pub(super) color: Color, - pub(super) ls_colors: &'a LsColors, + slice: TreeHorizontalSlice, + color: Color, + ls_colors: &'a LsColors, } impl fmt::Display for ColoredTreeHorizontalSlice<'_> { diff --git a/src/visualizer/methods.rs b/src/visualizer/methods.rs index 1a098adf..bfdf81b2 100644 --- a/src/visualizer/methods.rs +++ b/src/visualizer/methods.rs @@ -13,7 +13,7 @@ use table::*; use tree_table::*; use super::{ - coloring::{ColoredTreeHorizontalSlice, MaybeColoredTreeHorizontalSlice}, + coloring::MaybeColoredTreeHorizontalSlice, ColumnWidthDistribution, Visualizer, }; use crate::size; @@ -95,25 +95,19 @@ where tree_horizontal_slice, } = tree_row; - // TODO: move this `colored` code block into `visualizer/coloring.rs`. - let colored = self.coloring.and_then(|coloring| { - let path_components: Vec<&OsStr> = initial_row - .ancestors - .iter() - .map(|node| node.name.as_ref()) - .chain(initial_row.node_info.name.pipe_as_ref(once)) - .collect(); - coloring.node_color(&path_components, initial_row.node_info.children_count > 0) - }); - - // TODO: move this `tree` code block into `visualizer/coloring.rs`. - let tree = match colored { - Some((color, ls_colors)) => { - MaybeColoredTreeHorizontalSlice::Colorful(ColoredTreeHorizontalSlice { - slice: tree_horizontal_slice, - color, - ls_colors, - }) + let tree = match self.coloring { + Some(coloring) => { + let path_components: Vec<&OsStr> = initial_row + .ancestors + .iter() + .map(|node| node.name.as_ref()) + .chain(initial_row.node_info.name.pipe_as_ref(once)) + .collect(); + coloring.maybe_color_tree_slice( + &path_components, + initial_row.node_info.children_count > 0, + tree_horizontal_slice, + ) } None => MaybeColoredTreeHorizontalSlice::Colorless(tree_horizontal_slice), }; From b5bb1b75cac573845978d1c36b5869c4a74d20f2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Mar 2026 21:55:29 +0000 Subject: [PATCH 2/6] refactor: extract Option<&Coloring> dispatch into coloring.rs Move the Option matching on self.coloring out of methods.rs into a free function `maybe_color_slice` in coloring.rs. This further reduces coloring logic in methods.rs to just path component construction. Also tightens Coloring::maybe_color_tree_slice from pub(super) to private, since it's now only called within coloring.rs. https://claude.ai/code/session_012139SmUSC1oYSKNkvsg78g --- src/visualizer/coloring.rs | 15 ++++++++++++++- src/visualizer/methods.rs | 30 +++++++++++++----------------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/visualizer/coloring.rs b/src/visualizer/coloring.rs index 15ba3845..5971d34c 100644 --- a/src/visualizer/coloring.rs +++ b/src/visualizer/coloring.rs @@ -19,7 +19,7 @@ impl<'a> Coloring<'a> { /// Look up the color for a node identified by its path components and whether it has children, /// then wrap the given [`TreeHorizontalSlice`] in the appropriate colored or colorless variant. - pub(super) fn maybe_color_tree_slice( + fn maybe_color_tree_slice( &self, path_components: &[&'a OsStr], has_children: bool, @@ -109,6 +109,19 @@ impl Width for ColoredTreeHorizontalSlice<'_> { } } +/// Wrap a [`TreeHorizontalSlice`] with color if coloring is available, otherwise return it as-is. +pub(super) fn maybe_color_slice<'a, 'b>( + coloring: Option<&'b Coloring<'a>>, + path_components: &[&'a OsStr], + has_children: bool, + slice: TreeHorizontalSlice, +) -> MaybeColoredTreeHorizontalSlice<'b> { + match coloring { + Some(coloring) => coloring.maybe_color_tree_slice(path_components, has_children, slice), + None => MaybeColoredTreeHorizontalSlice::Colorless(slice), + } +} + /// Either a [`TreeHorizontalSlice`] (colorless) or a [`ColoredTreeHorizontalSlice`] (colorful). #[derive(Display)] pub(super) enum MaybeColoredTreeHorizontalSlice<'a> { diff --git a/src/visualizer/methods.rs b/src/visualizer/methods.rs index bfdf81b2..e3630b9c 100644 --- a/src/visualizer/methods.rs +++ b/src/visualizer/methods.rs @@ -13,7 +13,7 @@ use table::*; use tree_table::*; use super::{ - coloring::MaybeColoredTreeHorizontalSlice, + coloring::maybe_color_slice, ColumnWidthDistribution, Visualizer, }; use crate::size; @@ -95,22 +95,18 @@ where tree_horizontal_slice, } = tree_row; - let tree = match self.coloring { - Some(coloring) => { - let path_components: Vec<&OsStr> = initial_row - .ancestors - .iter() - .map(|node| node.name.as_ref()) - .chain(initial_row.node_info.name.pipe_as_ref(once)) - .collect(); - coloring.maybe_color_tree_slice( - &path_components, - initial_row.node_info.children_count > 0, - tree_horizontal_slice, - ) - } - None => MaybeColoredTreeHorizontalSlice::Colorless(tree_horizontal_slice), - }; + let path_components: Vec<&OsStr> = initial_row + .ancestors + .iter() + .map(|node| node.name.as_ref()) + .chain(initial_row.node_info.name.pipe_as_ref(once)) + .collect(); + let tree = maybe_color_slice( + self.coloring, + &path_components, + initial_row.node_info.children_count > 0, + tree_horizontal_slice, + ); format!( "{size} {tree}│{bar}│{ratio}", From 0bfaf8597abc2e37f10abe4b1ab375ff8ba56a88 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Mar 2026 21:57:41 +0000 Subject: [PATCH 3/6] style: cargo fmt https://claude.ai/code/session_012139SmUSC1oYSKNkvsg78g --- src/visualizer/methods.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/visualizer/methods.rs b/src/visualizer/methods.rs index e3630b9c..e4e692ba 100644 --- a/src/visualizer/methods.rs +++ b/src/visualizer/methods.rs @@ -12,10 +12,7 @@ use node_info::*; use table::*; use tree_table::*; -use super::{ - coloring::maybe_color_slice, - ColumnWidthDistribution, Visualizer, -}; +use super::{coloring::maybe_color_slice, ColumnWidthDistribution, Visualizer}; use crate::size; use pipe_trait::Pipe; use std::{cmp::min, ffi::OsStr, fmt::Display, iter::once}; From 36a3621358054d07b9e75e656411a7ef083bbcf0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Mar 2026 22:14:20 +0000 Subject: [PATCH 4/6] refactor: move path_components construction into maybe_color_slice Defer the path_components Vec allocation to inside maybe_color_slice, so it only happens when coloring is actually enabled. This avoids unnecessary per-row allocation in the common no-color case. Also removes the pipe_trait::Pipe dependency from methods.rs. https://claude.ai/code/session_012139SmUSC1oYSKNkvsg78g --- src/visualizer/coloring.rs | 11 +++++++++-- src/visualizer/methods.rs | 12 +++--------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/visualizer/coloring.rs b/src/visualizer/coloring.rs index 5971d34c..9085352d 100644 --- a/src/visualizer/coloring.rs +++ b/src/visualizer/coloring.rs @@ -110,14 +110,21 @@ impl Width for ColoredTreeHorizontalSlice<'_> { } /// Wrap a [`TreeHorizontalSlice`] with color if coloring is available, otherwise return it as-is. +/// +/// Path components are only constructed when coloring is enabled, avoiding +/// unnecessary allocation in the common no-color case. pub(super) fn maybe_color_slice<'a, 'b>( coloring: Option<&'b Coloring<'a>>, - path_components: &[&'a OsStr], + ancestors: impl Iterator, + name: &'a OsStr, has_children: bool, slice: TreeHorizontalSlice, ) -> MaybeColoredTreeHorizontalSlice<'b> { match coloring { - Some(coloring) => coloring.maybe_color_tree_slice(path_components, has_children, slice), + Some(coloring) => { + let path_components: Vec<&OsStr> = ancestors.chain(std::iter::once(name)).collect(); + coloring.maybe_color_tree_slice(&path_components, has_children, slice) + } None => MaybeColoredTreeHorizontalSlice::Colorless(slice), } } diff --git a/src/visualizer/methods.rs b/src/visualizer/methods.rs index e4e692ba..bfc38b75 100644 --- a/src/visualizer/methods.rs +++ b/src/visualizer/methods.rs @@ -14,8 +14,7 @@ use tree_table::*; use super::{coloring::maybe_color_slice, ColumnWidthDistribution, Visualizer}; use crate::size; -use pipe_trait::Pipe; -use std::{cmp::min, ffi::OsStr, fmt::Display, iter::once}; +use std::{cmp::min, ffi::OsStr, fmt::Display}; use zero_copy_pads::{align_left, align_right}; impl<'a, Name, Size> Visualizer<'a, Name, Size> @@ -92,15 +91,10 @@ where tree_horizontal_slice, } = tree_row; - let path_components: Vec<&OsStr> = initial_row - .ancestors - .iter() - .map(|node| node.name.as_ref()) - .chain(initial_row.node_info.name.pipe_as_ref(once)) - .collect(); let tree = maybe_color_slice( self.coloring, - &path_components, + initial_row.ancestors.iter().map(|node| node.name.as_ref()), + initial_row.node_info.name.as_ref(), initial_row.node_info.children_count > 0, tree_horizontal_slice, ); From c806ad903bf10bdbe9633ec94de4fce98de2ba33 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Mar 2026 22:18:28 +0000 Subject: [PATCH 5/6] refactor: rename maybe_color to maybe_colored Rename maybe_color_slice -> maybe_colored_slice and maybe_color_tree_slice -> maybe_colored_tree_slice for clarity, as the names describe the result rather than the action. https://claude.ai/code/session_012139SmUSC1oYSKNkvsg78g --- src/visualizer/coloring.rs | 6 +++--- src/visualizer/methods.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/visualizer/coloring.rs b/src/visualizer/coloring.rs index 9085352d..2aec2f19 100644 --- a/src/visualizer/coloring.rs +++ b/src/visualizer/coloring.rs @@ -19,7 +19,7 @@ impl<'a> Coloring<'a> { /// Look up the color for a node identified by its path components and whether it has children, /// then wrap the given [`TreeHorizontalSlice`] in the appropriate colored or colorless variant. - fn maybe_color_tree_slice( + fn maybe_colored_tree_slice( &self, path_components: &[&'a OsStr], has_children: bool, @@ -113,7 +113,7 @@ impl Width for ColoredTreeHorizontalSlice<'_> { /// /// Path components are only constructed when coloring is enabled, avoiding /// unnecessary allocation in the common no-color case. -pub(super) fn maybe_color_slice<'a, 'b>( +pub(super) fn maybe_colored_slice<'a, 'b>( coloring: Option<&'b Coloring<'a>>, ancestors: impl Iterator, name: &'a OsStr, @@ -123,7 +123,7 @@ pub(super) fn maybe_color_slice<'a, 'b>( match coloring { Some(coloring) => { let path_components: Vec<&OsStr> = ancestors.chain(std::iter::once(name)).collect(); - coloring.maybe_color_tree_slice(&path_components, has_children, slice) + coloring.maybe_colored_tree_slice(&path_components, has_children, slice) } None => MaybeColoredTreeHorizontalSlice::Colorless(slice), } diff --git a/src/visualizer/methods.rs b/src/visualizer/methods.rs index bfc38b75..8b061d9a 100644 --- a/src/visualizer/methods.rs +++ b/src/visualizer/methods.rs @@ -12,7 +12,7 @@ use node_info::*; use table::*; use tree_table::*; -use super::{coloring::maybe_color_slice, ColumnWidthDistribution, Visualizer}; +use super::{coloring::maybe_colored_slice, ColumnWidthDistribution, Visualizer}; use crate::size; use std::{cmp::min, ffi::OsStr, fmt::Display}; use zero_copy_pads::{align_left, align_right}; @@ -91,7 +91,7 @@ where tree_horizontal_slice, } = tree_row; - let tree = maybe_color_slice( + let tree = maybe_colored_slice( self.coloring, initial_row.ancestors.iter().map(|node| node.name.as_ref()), initial_row.node_info.name.as_ref(), From bd071fdaef21cbd3b2a9988da99a78b8380e97ec Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Mar 2026 22:21:21 +0000 Subject: [PATCH 6/6] refactor: inline maybe_colored_tree_slice into maybe_colored_slice Consolidate the two functions since maybe_colored_tree_slice was only ever called from maybe_colored_slice. https://claude.ai/code/session_012139SmUSC1oYSKNkvsg78g --- src/visualizer/coloring.rs | 44 ++++++++++++++------------------------ 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/src/visualizer/coloring.rs b/src/visualizer/coloring.rs index 2aec2f19..a5c2a392 100644 --- a/src/visualizer/coloring.rs +++ b/src/visualizer/coloring.rs @@ -16,29 +16,6 @@ impl<'a> Coloring<'a> { pub fn new(ls_colors: LsColors, map: HashMap, Color>) -> Self { Coloring { ls_colors, map } } - - /// Look up the color for a node identified by its path components and whether it has children, - /// then wrap the given [`TreeHorizontalSlice`] in the appropriate colored or colorless variant. - fn maybe_colored_tree_slice( - &self, - path_components: &[&'a OsStr], - has_children: bool, - slice: TreeHorizontalSlice, - ) -> MaybeColoredTreeHorizontalSlice<'_> { - let color = if has_children { - Some(Color::Directory) - } else { - self.map.get(path_components).copied() - }; - match color { - Some(color) => MaybeColoredTreeHorizontalSlice::Colorful(ColoredTreeHorizontalSlice { - slice, - color, - ls_colors: &self.ls_colors, - }), - None => MaybeColoredTreeHorizontalSlice::Colorless(slice), - } - } } /// The coloring to apply to a node name. @@ -120,11 +97,22 @@ pub(super) fn maybe_colored_slice<'a, 'b>( has_children: bool, slice: TreeHorizontalSlice, ) -> MaybeColoredTreeHorizontalSlice<'b> { - match coloring { - Some(coloring) => { - let path_components: Vec<&OsStr> = ancestors.chain(std::iter::once(name)).collect(); - coloring.maybe_colored_tree_slice(&path_components, has_children, slice) - } + let coloring = match coloring { + Some(coloring) => coloring, + None => return MaybeColoredTreeHorizontalSlice::Colorless(slice), + }; + let path_components: Vec<&OsStr> = ancestors.chain(std::iter::once(name)).collect(); + let color = if has_children { + Some(Color::Directory) + } else { + coloring.map.get(&path_components).copied() + }; + match color { + Some(color) => MaybeColoredTreeHorizontalSlice::Colorful(ColoredTreeHorizontalSlice { + slice, + color, + ls_colors: &coloring.ls_colors, + }), None => MaybeColoredTreeHorizontalSlice::Colorless(slice), } }