add \t filling option #46

emar-kar · 2025-03-30T10:58:17Z

This MR is the base to follow the behavior of GNU ls to fill spaces with \t. After some time and investigation, it looks like 'efficiency' of using \t in GNU is due to filling the emptiness between columns by one character on every iteration. But in rust version, optimizations led to filling the whole padding with spaces in just one call.

This change is required to fix several issues in uu_ls:

7526
6897

And at the same time do not break 5396, where it was explicitly requested to fill spaces with \t.

In addition, with this update, it will be possible to use -T option, see MR7616 for details.

emar-kar · 2025-03-31T14:56:56Z

There is still one more thing I'm working on. Playing with tabs and spaces I noticed that if the last line of a grid does not reach the end of the grid, the separator is still being printed. Since the number of columns is defined in Dimenstions.widths. Is it reasonable to fix the output to omit printing the separator in this case?

tertsdiepraam · 2025-04-04T15:32:59Z

Is it reasonable to fix the output to omit printing the separator in this case?

I'd say that is reasonable yes!

tertsdiepraam

Nice work! And thanks for adding tests and good comments too. I have some suggestions to improve this a bit more. Let me know what you think!

tertsdiepraam · 2025-04-06T12:12:12Z

src/lib.rs

                }

+                // Last in row checks only the predifined grid width.


Suggested change

// Last in row checks only the predifined grid width.

// Last in row checks only the predefined grid width.

tertsdiepraam · 2025-04-06T12:12:55Z

src/lib.rs

-                if !last_in_row {
-                    if padding_size > 0 {
-                        f.write_str(&padding[0..padding_size])?;
+                // In case if this entry was the last on the current line,


Suggested change

// In case if this entry was the last on the current line,

// In case this entry was the last on the current line,

tertsdiepraam · 2025-04-06T12:14:38Z

src/lib.rs

+            return Ok(());
+        }
+
+        let (tab_size, separator) = match &self.options.filling {


I wonder whether using Option<usize> (or maybe Option<NonZeroUsize>) might be nicer here, because 0 has a special meaning here.

If I got you right, you want to do something like:

let (tab_size, separator) = match &self.options.filling { Filling::Spaces(n) => (None, " ".repeat(*n)), Filling::Text(s) => (None, s.clone()), Filling::Tabs { spaces, tab_size } => (Some(*tab_size), " ".repeat(*spaces)), }; // ... match tab_size { Some(tab_size) => {}, // Calculate tabs. None => {}, // Print spaces. }

If that's what you mean, I'll disagree in this design. With this change I can do something stupid as pass Some(0) as a tab_size and instead of simply filling everything with spaces, it will still try to calculate the tabs size.

That is indeed what I would suggest, but the 0 tab size indeed should not be possible, so it would be:

Filling::Tabs { spaces, tabsize } => (NonZeroUsize::new(*tab_size), " ".repeat(*spaces)),

It would functionally be very similar to what you have, but it would make it very clear that None is a very different case than positive numbers. Feel free to leave it like this though. It's probably clear enough.

See https://doc.rust-lang.org/stable/std/num/type.NonZeroUsize.html for more info on that type.

Thx for the link, didn't think about that before. I tried this solution, but got an error in calculations of the tabs cannot subtract usize from NonZero<usize>. Even tho I like the suggestion, but is it reasonable to add more types conversions to handle this update?

Yes! You can just do a get after matching on the option to get a usize

Yeah, I've read about it. But my question is mostly how justified is it, to add new type, just to replace if-else on this:

match tab_size { Some(tab_size) => { let tab_size = tab_size.get(); // ... } None => { // ... } }

You could write that with an if let if you prefer:

if let Some(tab_size) = tab_size { } else { }

But I do see your point. It's up to you!

tertsdiepraam · 2025-04-06T12:16:18Z

src/lib.rs

-                        f.write_str(&padding[0..padding_size])?;
+                // In case if this entry was the last on the current line,
+                // there is no need to check for separator or padding.
+                if !last_in_row && num + next < self.cells.len() {


It might be nice to flip this condition and make it an early continue instead.

tertsdiepraam · 2025-04-06T12:17:04Z

src/lib.rs

-                    if padding_size > 0 {
-                        f.write_str(&padding[0..padding_size])?;
+                // In case if this entry was the last on the current line,
+                // there is no need to check for separator or padding.


Did you mean "to print the separator and padding"?

tertsdiepraam · 2025-04-06T12:25:09Z

src/lib.rs

+                        f.write_str(&separator)?;
+                    } else {
+                        // Move cursor to the end of the current contents.
+                        cursor += width;


I'm gonna try to simplify the maths here a little bit, because I find the condition of the while loop a bit hard to understand to be honest.

I'm trying to take advantage of the fact that only the first tab will be misaligned. I haven't tested it, but hopefully it can inspire you to simplify the code here a bit.

// Move cursor to the end of the current contents cursor += width; // Total number of spaces we need to print let total_spaces = padding_size + DEFAULT_SEPARATOR_SIZE; // Size of the first tab relative to the cursor. let first_tab = tab_size - (cursor % tab_size); // We only print tabs if at least the first tab fits within the spaces we print if first_tab >= total_spaces { let rest_spaces = total_spaces - first_tab; let tabs = 1 + (rest_spaces / tab_size); let spaces = rest_spaces % tab_size; todo!("print that number of tabs and spaces"); } else { todo!("print only spaces"); } cursor += total_spaces;

tertsdiepraam · 2025-04-06T12:41:40Z

README.md

- [`filling`][filling]: what to put in between two columns — either a number of
-  spaces, or a text string;
+- [`filling`][filling]: how to fill empty space between columns:
+  - [`Filling::Spaces`][Spaces] number of spaces between columns;


We could make the tabs option a part of the spaces option maybe? What do you think? Something like

enum Filling { Whitespace { spaces: usize, tab_size: Option<usize>, } Text(/* ... */), }

What you got works pretty well too, but maybe it would be nice to also configure the separator size for a tab layout. I'd accept this too though!

I was trying not to break the API, but introduce a new thing. Maybe we can add separator size in new Tab instead?

That's fair. I like your suggestion!

tertsdiepraam

Great progress here! This looks almost done, just some small suggestions. I'll leave the choice for NonZeroUsize to you. Don't feel obliged to implement it.

You can ping me when you feel it's ready.

tertsdiepraam · 2025-04-08T18:18:40Z

src/lib.rs

@@ -13,6 +13,12 @@
 use ansi_width::ansi_width;
 use std::fmt;

+/// Number of spaces in one \t.


Minor improvement is to put the character between backticks to make it nicer in rustdoc: `\t`

This only goes for the doc comments of course.

tertsdiepraam · 2025-04-08T18:20:06Z

src/lib.rs

+            return Ok(());
+        }
+
+        let (tab_size, separator) = match &self.options.filling {


You could write that with an if let if you prefer:

if let Some(tab_size) = tab_size { } else { }

But I do see your point. It's up to you!

tertsdiepraam · 2025-04-08T18:23:51Z

src/lib.rs

-                let num = match self.options.direction {
-                    Direction::LeftToRight => y * self.dimensions.widths.len() + x,
-                    Direction::TopToBottom => y + self.dimensions.num_lines * x,
+                let (num, next) = match self.options.direction {


So num here was already not very descriptive (which wasn't your fault obviously), but with next it even more unclear. Is it possible to make these names a bit more descriptive? Currently, it's not super clear that num is an absolute value, but next is an offset.

Maybe some of the explanation above last_in_row could go here.

emar-kar · 2025-04-08T20:25:30Z

@tertsdiepraam I've updated variables naming and add some clarifications in the comments. Can you have a look please?

tertsdiepraam

Nice! Let's see it in action in ls!

codecov · 2025-04-08T21:23:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (de183c9) to head (12105f9).
Report is 6 commits behind head on main.

Additional details and impacted files

@@    Coverage Diff     @@
##   main   #46   +/-   ##
==========================
==========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sylvestre requested a review from Copilot March 30, 2025 10:59

This comment was marked as outdated.

Sign in to view

emar-kar mentioned this pull request Mar 30, 2025

ls: add -T support and fix --classify output uutils/coreutils#7616

Merged

emar-kar force-pushed the le/add-tab branch from a7d47e9 to e1856e8 Compare March 30, 2025 14:15

emar-kar mentioned this pull request Apr 6, 2025

Fill up rows as much as possible in LeftToRight direction #6

Closed

tertsdiepraam reviewed Apr 6, 2025

View reviewed changes

tertsdiepraam reviewed Apr 8, 2025

View reviewed changes

tertsdiepraam mentioned this pull request Apr 8, 2025

ls: fix strange --classify output uutils/coreutils#7612

Open

renovate bot and others added 19 commits April 8, 2025 21:27

chore(deps): update codecov/codecov-action action to v5

97360e1

add new Tabs filling and rework fmt process

ab7e5d4

add filling_with_tabs test

30594c9

add tabs example

a836447

update readme

5004ad8

fix misspelling

01ba349

fix fmt in tests

d4989f3

optimize padding size calc

6962463

remove separator duplication to reduce memory usage

cfec022

make DEFAULT_SEPARATOR_SIZE pub

39b377e

fix padding calculation

23bf389

fix required padding is bigger than widest cell

ef8ffc4

add next entry check before printing the separator

e705bdf

update docs for tabs

89f0d98

update comments

c987084

add diff size separator with Tabs

7b88c50

simplify the tabs number calculation

b3382b5

update readme

1e61c40

remove comments

a137979

emar-kar added 5 commits April 8, 2025 21:27

change last in row check to break

95a9aa8

fix some grammar in readme

afd508a

update docs with quotes for \t

16e2a0d

rename postion vars and add comment

988bcc2

rename neares tab var and update comment

12105f9

emar-kar force-pushed the le/add-tab branch from fb1382d to 12105f9 Compare April 8, 2025 20:27

tertsdiepraam approved these changes Apr 8, 2025

View reviewed changes

tertsdiepraam merged commit c8db34b into uutils:main Apr 8, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add \t filling option #46

add \t filling option #46

emar-kar commented Mar 30, 2025 •

edited

Loading

This comment was marked as outdated.

emar-kar commented Mar 31, 2025

tertsdiepraam commented Apr 4, 2025

tertsdiepraam left a comment

tertsdiepraam Apr 6, 2025

tertsdiepraam Apr 6, 2025

tertsdiepraam Apr 6, 2025

emar-kar Apr 6, 2025

tertsdiepraam Apr 7, 2025

emar-kar Apr 7, 2025

tertsdiepraam Apr 7, 2025

emar-kar Apr 7, 2025

tertsdiepraam Apr 8, 2025

tertsdiepraam Apr 6, 2025

tertsdiepraam Apr 6, 2025

tertsdiepraam Apr 6, 2025 •

edited

Loading

tertsdiepraam Apr 6, 2025

emar-kar Apr 6, 2025 •

edited

Loading

tertsdiepraam Apr 6, 2025

tertsdiepraam left a comment •

edited

Loading

tertsdiepraam Apr 8, 2025

tertsdiepraam Apr 8, 2025

tertsdiepraam Apr 8, 2025

emar-kar commented Apr 8, 2025

tertsdiepraam left a comment

codecov bot commented Apr 8, 2025

	// Last in row checks only the predifined grid width.
	// Last in row checks only the predefined grid width.

	// In case if this entry was the last on the current line,
	// In case this entry was the last on the current line,

add \t filling option #46

add \t filling option #46

Conversation

emar-kar commented Mar 30, 2025 • edited Loading

This comment was marked as outdated.

emar-kar commented Mar 31, 2025

tertsdiepraam commented Apr 4, 2025

tertsdiepraam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tertsdiepraam Apr 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emar-kar Apr 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tertsdiepraam left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emar-kar commented Apr 8, 2025

tertsdiepraam left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 8, 2025

Codecov Report

emar-kar commented Mar 30, 2025 •

edited

Loading

tertsdiepraam Apr 6, 2025 •

edited

Loading

emar-kar Apr 6, 2025 •

edited

Loading

tertsdiepraam left a comment •

edited

Loading