feat: migrate color handling to colored_text crate#114
Merged
Conversation
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI
participant App
participant ColorConfig
participant Renderer
participant Output
User->>CLI: invoke lsplus (args, env, config)
CLI->>App: run_with_flags(args, config)
App->>App: Params::merge(flags, config)
App->>ColorConfig: configure_color_output(¶ms)
ColorConfig->>ColorConfig: color_mode_for(params) → ColorMode::Never/Auto
ColorConfig->>ColorConfig: ColorizeConfig::set_color_mode(...)
App->>Renderer: display_long_format(...) / display_short_format(...)
Renderer->>Renderer: build FileInfo (short_name,name_style,dimmed) and StyledText
Renderer->>Output: print lines (coloured if ColorMode permits)
Output->>User: visible/plain output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 35 |
| Duplication | 14 |
TIP This summary will be updated as you push new changes. Give us feedback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
tests/integration.rs (1)
36-38: Duplicatehas_ansihelper function.This function is identical to the one in
tests/crate/file.rs. Consider extracting it to a shared test utilities module to avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration.rs` around lines 36 - 38, The helper function has_ansi is duplicated (also present in tests/crate/file.rs); extract it into a shared test utilities module (e.g., tests/utils.rs or tests/common/mod.rs), move the has_ansi(text: &str) -> bool implementation there, export it (pub) and update both callers (tests/integration.rs and tests/crate/file.rs) to use the shared symbol (crate::tests::utils::has_ansi or appropriate path), then remove the duplicate definitions from the individual test files.src/utils/render.rs (2)
116-126: Potential inefficiency:check_display_namecalled multiple times per item.In the short format rendering path,
check_display_nameis called in multiple functions for the sameFileInfo:
plain_short_cell_content(line 118)short_cell_parts(line 134)- Indirectly via
short_column_widthsandrender_short_rowFor large directories, this could add up. Consider caching the result or restructuring to compute it once per item.
♻️ Suggested optimisation
-fn short_cell_parts(info: &FileInfo) -> (String, String) { +fn short_cell_parts(info: &FileInfo, checked_name: &str) -> (String, String) { let prefix = info .item_icon .as_ref() .map(|icon| format!("{} ", icon)) .unwrap_or_default(); - let display_name = check_display_name(info); - let name = if display_name == info.display_name { + let name = if checked_name == info.display_name { info.short_name.clone() } else { - strip_str(&display_name) + strip_str(checked_name) }; (prefix, name) }Then compute
check_display_nameonce at the row rendering level and pass it through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/render.rs` around lines 116 - 126, The code calls check_display_name repeatedly for the same FileInfo (e.g., in plain_short_cell_content, short_cell_parts, short_column_widths, and render_short_row); compute check_display_name once per FileInfo at the row rendering level (for example in render_short_row or the caller that iterates files) and thread that computed display_name through the helper functions instead of calling check_display_name again—update plain_short_cell_content and short_cell_parts to accept a display_name (&str or String) parameter (or return a tuple that includes it), and change callers (short_column_widths, render_short_row, etc.) to pass the cached value so each FileInfo’s display name is determined only once.
143-155: Column width calculation iterates all rows multiple times.
short_column_widthscallsplain_short_cell_contentfor each cell, which in turn callsshort_cell_partsandcheck_display_name. Combined with the earlier call inshort_cell_width(viashort_column_count), this means each cell's content is computed twice.For typical directory listings this is likely negligible, but for very large directories it could be noticeable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/render.rs` around lines 143 - 155, The code recomputes each cell's display string twice because short_column_widths calls plain_short_cell_content (which calls short_cell_parts and check_display_name) and earlier short_column_count/short_cell_width also compute it; fix by computing each cell's plain short content once and reusing it: in the pipeline, either change short_column_count/short_cell_width to return/store a Vec<Vec<String>> of plain_short_cell_content (or a single shared Vec of widths) instead of just counts, or add a new helper that builds a Vec<Vec<String>> of plain_short_cell_content by iterating rows once and then have short_column_widths use those precomputed strings to compute visible_text_width; update references to short_column_count/short_cell_width and short_column_widths accordingly and remove duplicate plain_short_cell_content calls (affecting functions short_cell_parts and check_display_name usage).src/utils/file.rs (2)
291-321: Conditional symlink formatting logic is correct but could be simplified.The logic correctly handles ignored symlinks by using
format_symlink_display_name_with_dimwithdimmed=true. However, both branches call the same underlying function with differentdimmedvalues. Consider simplifying:♻️ Optional simplification
let (display_name, short_name, name_style) = if metadata.is_symlink() { ( - if ignored { - format_symlink_display_name_with_dim( - &safe_file_name, - path, - fs::read_link(path), - params, - true, - ) - } else { - format_symlink_display_name( - &safe_file_name, - path, - fs::read_link(path), - params, - ) - }, + format_symlink_display_name_with_dim( + &safe_file_name, + path, + fs::read_link(path), + params, + ignored, + ), format!("{safe_file_name}{}", symlink_short_suffix(params)), NameStyle::Symlink, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/file.rs` around lines 291 - 321, The symlink branch duplicates calls that only differ by a dimmed flag; replace the if metadata.is_symlink() conditional body to call format_symlink_display_name_with_dim(&safe_file_name, path, fs::read_link(path), params, ignored) (using ignored as the dimmed flag) for the display_name, keep the same short_name via format!("{safe_file_name}{}", symlink_short_suffix(params)), and NameStyle::Symlink; leave the non-symlink branch unchanged (functions: format_symlink_display_name_with_dim, format_symlink_display_name, fs::read_link, symlink_short_suffix, NameStyle::Symlink, colorize_name_by_metadata, name_style_by_metadata).
421-439:name_style_by_metadataduplicates logic fromcolorize_name_by_metadata.Both functions determine the file type (symlink, directory, executable, plain) using nearly identical conditional logic. This duplication could lead to inconsistencies if one is updated without the other.
Consider refactoring to have one function call the other, or extract the type determination into a shared helper:
♻️ Suggested refactor to reduce duplication
fn name_style_by_metadata(metadata: &fs::Metadata) -> NameStyle { if metadata.is_symlink() { NameStyle::Symlink } else if metadata.is_dir() { NameStyle::Directory } else if is_executable(metadata) { NameStyle::Executable } else { NameStyle::Plain } } fn is_executable(metadata: &fs::Metadata) -> bool { #[cfg(unix)] { metadata.permissions().mode() & 0o111 != 0 } #[cfg(windows)] { false } } fn colorize_name_by_metadata( safe_name: &str, metadata: &fs::Metadata, dimmed: bool, ) -> String { let styled = match name_style_by_metadata(metadata) { NameStyle::Symlink => safe_name.cyan(), NameStyle::Directory => safe_name.blue(), NameStyle::Executable => safe_name.green().bold(), NameStyle::Plain => StyledText::plain(safe_name), }; apply_dim(styled, dimmed).to_string() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/file.rs` around lines 421 - 439, name_style_by_metadata duplicates file-type/executable detection logic present in colorize_name_by_metadata; extract the executable check into a shared helper (e.g., is_executable(metadata: &fs::Metadata) -> bool with #[cfg(unix)] and #[cfg(windows)] branches) and then simplify name_style_by_metadata to use that helper (returning NameStyle::Symlink, Directory, Executable, or Plain), and update colorize_name_by_metadata to call name_style_by_metadata(metadata) and match on its result to pick styling; this removes duplicated conditional logic and centralizes platform-specific executable detection.tests/crate/file.rs (1)
73-79: Hardcoded ANSI escape sequences may be fragile.The assertions use raw ANSI sequences like
"\u{1b}[34m.\u{1b}[0m"which tightly couple the test to the exact output format ofcolored_text. If the library changes its reset sequence or uses different SGR codes, these tests will break.Consider using the
has_ansihelper combined with content checks, or extract the expected sequences into named constants for easier maintenance:const BLUE_DOT: &str = "\u{1b}[34m.\u{1b}[0m"; assert_eq!(check_display_name(&dot), BLUE_DOT);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/crate/file.rs` around lines 73 - 79, The tests assert exact ANSI escape sequences for colored output in with_color_output_enabled() using basic_info(...) and check_display_name(...); replace these fragile assertions by either (a) using a helper like has_ansi(&s) to assert that the string contains ANSI sequences and separately assert the visible content (e.g., "." or ".."), or (b) extract the expected sequence into a named constant (e.g., BLUE_DOT) and use that constant in assert_eq! to centralize the value; update the two assertions for dot and dotdot accordingly so tests no longer depend on inline raw SGR bytes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/crate/render.rs`:
- Around line 43-68: Tests mutate global ColorizeConfig without synchronization;
make those mutations serial by introducing a global Mutex and have
ColorModeGuard hold the lock while it saves/restores state. Add a static lock
(e.g., once_cell::sync::Lazy<Mutex<()>> or parking_lot::Mutex) and change
ColorModeGuard to store the MutexGuard alongside the previous ColorMode so
acquiring the lock happens in ColorModeGuard::set and the guard is dropped in
Drop (releasing the lock) after restoring the previous mode; update
with_color_output_enabled to create the ColorModeGuard as before so the lock is
held for the test duration. Ensure references to ColorModeGuard, ColorizeConfig,
with_color_output_enabled and has_ansi are used to locate the changes.
In `@tests/integration.rs`:
- Around line 360-361: The test
test_gitignore_flag_dims_ignored_entries_in_short_output currently asserts no
ANSI codes (assert!(!has_ansi(...))) which contradicts its intent to verify
dimming; either rename the test to reflect plain/captured output (e.g.,
test_gitignore_flag_output_is_plain_when_captured) or change the test to force
colour and capture raw output: use the runner helper run_and_capture_raw (or
equivalent) and pass the --color=always flag, then assert that ignored_line
contains ANSI dim codes (using has_ansi or a specific dim-sequence check) while
visible_line shows expected colouring; update assertions and test name
accordingly.
---
Nitpick comments:
In `@src/utils/file.rs`:
- Around line 291-321: The symlink branch duplicates calls that only differ by a
dimmed flag; replace the if metadata.is_symlink() conditional body to call
format_symlink_display_name_with_dim(&safe_file_name, path, fs::read_link(path),
params, ignored) (using ignored as the dimmed flag) for the display_name, keep
the same short_name via format!("{safe_file_name}{}",
symlink_short_suffix(params)), and NameStyle::Symlink; leave the non-symlink
branch unchanged (functions: format_symlink_display_name_with_dim,
format_symlink_display_name, fs::read_link, symlink_short_suffix,
NameStyle::Symlink, colorize_name_by_metadata, name_style_by_metadata).
- Around line 421-439: name_style_by_metadata duplicates file-type/executable
detection logic present in colorize_name_by_metadata; extract the executable
check into a shared helper (e.g., is_executable(metadata: &fs::Metadata) -> bool
with #[cfg(unix)] and #[cfg(windows)] branches) and then simplify
name_style_by_metadata to use that helper (returning NameStyle::Symlink,
Directory, Executable, or Plain), and update colorize_name_by_metadata to call
name_style_by_metadata(metadata) and match on its result to pick styling; this
removes duplicated conditional logic and centralizes platform-specific
executable detection.
In `@src/utils/render.rs`:
- Around line 116-126: The code calls check_display_name repeatedly for the same
FileInfo (e.g., in plain_short_cell_content, short_cell_parts,
short_column_widths, and render_short_row); compute check_display_name once per
FileInfo at the row rendering level (for example in render_short_row or the
caller that iterates files) and thread that computed display_name through the
helper functions instead of calling check_display_name again—update
plain_short_cell_content and short_cell_parts to accept a display_name (&str or
String) parameter (or return a tuple that includes it), and change callers
(short_column_widths, render_short_row, etc.) to pass the cached value so each
FileInfo’s display name is determined only once.
- Around line 143-155: The code recomputes each cell's display string twice
because short_column_widths calls plain_short_cell_content (which calls
short_cell_parts and check_display_name) and earlier
short_column_count/short_cell_width also compute it; fix by computing each
cell's plain short content once and reusing it: in the pipeline, either change
short_column_count/short_cell_width to return/store a Vec<Vec<String>> of
plain_short_cell_content (or a single shared Vec of widths) instead of just
counts, or add a new helper that builds a Vec<Vec<String>> of
plain_short_cell_content by iterating rows once and then have
short_column_widths use those precomputed strings to compute visible_text_width;
update references to short_column_count/short_cell_width and short_column_widths
accordingly and remove duplicate plain_short_cell_content calls (affecting
functions short_cell_parts and check_display_name usage).
In `@tests/crate/file.rs`:
- Around line 73-79: The tests assert exact ANSI escape sequences for colored
output in with_color_output_enabled() using basic_info(...) and
check_display_name(...); replace these fragile assertions by either (a) using a
helper like has_ansi(&s) to assert that the string contains ANSI sequences and
separately assert the visible content (e.g., "." or ".."), or (b) extract the
expected sequence into a named constant (e.g., BLUE_DOT) and use that constant
in assert_eq! to centralize the value; update the two assertions for dot and
dotdot accordingly so tests no longer depend on inline raw SGR bytes.
In `@tests/integration.rs`:
- Around line 36-38: The helper function has_ansi is duplicated (also present in
tests/crate/file.rs); extract it into a shared test utilities module (e.g.,
tests/utils.rs or tests/common/mod.rs), move the has_ansi(text: &str) -> bool
implementation there, export it (pub) and update both callers
(tests/integration.rs and tests/crate/file.rs) to use the shared symbol
(crate::tests::utils::has_ansi or appropriate path), then remove the duplicate
definitions from the individual test files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c11aff4-0e43-4be1-8552-e4c1bf450022
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlREADME.mddocs/src/config.mddocs/src/usage.mdsrc/app.rssrc/cli.rssrc/lib.rssrc/structs.rssrc/utils.rssrc/utils/color.rssrc/utils/file.rssrc/utils/render.rstests/crate/app.rstests/crate/cli.rstests/crate/file.rstests/crate/render.rstests/crate/settings.rstests/integration.rstests/seams.rstests/structs.rs
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
seapagan
changed the title
colored_text crate
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
This was referenced Apr 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This migrates lsplus from
inline_colorizationtocolored_textfor colour output, and adds explicit no-color support.It also fixes the short-view rendering regressions discovered during the migration by restoring the old aligned behavior and then removing
prettytablefrom short mode entirely.Key changes:
inline_colorizationcrate withcolored_text0.4.1prettytablefor nowSummary by CodeRabbit
New Features
Behaviour Change
Documentation