fix(display): use uniform column widths in grid layout#1215
Open
dawitlabs wants to merge 1 commit into
Open
Conversation
GNU ls uses uniform column widths (all columns = max cell width) when laying out filenames in grid mode. lsd was using term_grid's fit_into_width, which assigns each column its own variable width based on the widest cell in that column. This could produce misleading output where a short filename appeared to be the last entry in a row while a later entry was hidden in an unexpectedly wide column. Track the maximum cell width while building the grid, then compute the largest number of columns that fit in the terminal width assuming every column is that width. Pass that count to fit_into_columns instead of calling fit_into_width. Fixes lsd-rs#594
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR changes grid rendering to use uniform column widths (similar to GNU ls) to prevent short entries from appearing as if they’re the last visible entry in a row when a later, wider entry starts off-screen.
Changes:
- Track the maximum visible cell width while building grid cells.
- Replace
fit_into_width()with a uniformfit_into_columns(num_cols)calculation based on terminal width. - Add a regression test covering the “hidden wide column” scenario (issue #594).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+158
to
+167
| // Use uniform column widths (like GNU ls) so that no entry appears | ||
| // to be the last in a row while a later entry is hidden off-screen. | ||
| // Variable-width columns (fit_into_width) can place a short entry | ||
| // just before a wide column whose left edge is beyond the terminal. | ||
| let filling = 2; // matches Filling::Spaces(2) above | ||
| let num_cols = if max_cell_width == 0 { | ||
| 1 | ||
| } else { | ||
| //does not fit into grid, usually because (some) filename(s) | ||
| //are longer or almost as long as term_width | ||
| //print line by line instead! | ||
| output += &grid.fit_into_columns(1).to_string(); | ||
| } | ||
| ((tw + filling) / (max_cell_width + filling)).max(1) | ||
| }; |
| .unwrap(); | ||
| dir.child("file_16").touch().unwrap(); | ||
|
|
||
| let argv = ["lsd"]; |
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
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.
What
lsd's grid layout usesterm_grid::fit_into_width, which assigns each column its own width (the widest cell in that column). This produces misleading output when filenames have mixed lengths: a short entry appears to be the last on a row, while a later entry is hidden in a wider column that starts unexpectedly far to the right.This is exactly what GNU
lsavoids by using uniform column widths — every column is as wide as the widest filename.Reported in #594.
Fix
While building the grid cells, track the maximum cell width. Then compute the largest number of columns that fit in the terminal width assuming all columns have that uniform width:
This replaces the
fit_into_widthcall and matches GNUlsbehaviour.Changes
src/display.rs: trackmax_cell_width, replacefit_into_widthwith uniform-column-count calculation, add regression test for the issue's exact reproduce case (17 files, 2 with ~60-char names, terminal width 91)Testing
Manually reproduced the issue's steps:
Before: variable-width columns could place
file_16in a column that started beyond the visible area.After: uniform columns ensure every entry is visible, matching GNU
lsoutput.