Add `Tab` option #24

allaboutevemirolive · 2023-10-18T13:18:27Z

As mentioned in Issue #5396, I have added an option to use Tab as a separator to fix the problem with the ls command.

codecov · 2023-10-18T13:52:47Z

Codecov Report

Merging #24 (9cfb306) into main (154253a) will increase coverage by 1.27%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
+ Coverage   75.14%   76.42%   +1.27%     
==========================================
  Files           4        4              
  Lines         338      369      +31     
  Branches       50       56       +6     
==========================================
+ Hits          254      282      +28     
  Misses         70       70              
- Partials       14       17       +3

Flag	Coverage Δ
macos_latest	`76.42% <90.90%> (+1.27%)`	⬆️
ubuntu_latest	`76.42% <90.90%> (+1.27%)`	⬆️
windows_latest	`20.32% <9.09%> (-1.57%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/lib.rs	`66.46% <100.00%> (+0.20%)`	⬆️
tests/test.rs	`97.19% <90.00%> (-1.46%)`	⬇️

allaboutevemirolive · 2023-10-18T14:55:38Z

Should I add test-case?

@tertsdiepraam @sylvestre

tertsdiepraam · 2023-10-18T15:08:31Z

I can't test this right now, so if I'm wrong please correct me. I think the way that ls works is that it uses spaces, but replaces every N scapes by a tab basically. If not sure how that works for the separator. If you could explore that a bit and add those test cases to the test suite, that would be great!

My theory is based on: https://www.gnu.org/software/coreutils/manual/html_node/General-output-formatting.html

sylvestre · 2023-10-19T20:02:36Z

the CI isn't super happy :)

allaboutevemirolive · 2023-10-19T20:18:03Z

the CI isn't super happy :)

Will make the necessary corrections. I'm sure my implementation is on the right track.

Here's my summary as references:

[trimed]

To achieve the same output behavior as the ls, I introduced the snippet below, to deal first with,

If the user specifies a tab size of 0 or less, it will print normal spaces.
If the tab size is greater than 0, it will either use the default tab size or the one defined by the user. It calculates how many tabs can be used instead of empty spaces and prints any remaining empty spaces as well.

        let separator = match &self.grid.options.filling {
            Filling::Spaces(n) if self.grid.options.tab_size <= 0 => " ".to_string().repeat(*n),
            Filling::Spaces(n) => {
                let tab_count = n / self.grid.options.tab_size;
                let remaining_spaces = n % self.grid.options.tab_size;
                "\t".repeat(tab_count) + &" ".repeat(remaining_spaces)
            }
            Filling::Text(s) => s.clone(),
        };

sylvestre · 2023-10-19T20:27:58Z

this is GPL code. please don't copy it...
and please remove it from here

allaboutevemirolive · 2023-10-19T21:06:42Z

I simply make it as reference for guidance (no copy pasta, since C code need to be written everything by hands) to deal with tab behaviour/defining tab_size and print tab instead of empty space...

Maybe you can double check to make sure no licensing issues. :)

Edit: Do you mean remove the code in my pull request or remove my previous comment that has GPL code?

sylvestre · 2023-10-20T05:49:04Z

The previous comment with gpl code.
When reimplementing a gpl feature, you are not supposed to read the original code.

sylvestre · 2023-10-20T07:44:06Z

i did it for you :)

sylvestre · 2023-10-20T07:44:50Z

tests/test.rs

+        let mut grid = Grid::new(GridOptions {
+            filling: Filling::Spaces(10),
+            direction: Direction::LeftToRight,
+            tab_size: 0,


could you please add a test with -1 as tab_size ? :)

sylvestre · 2023-10-20T07:46:02Z

tests/test.rs

    let grid = Grid::new(GridOptions {
        direction: Direction::TopToBottom,
        filling: Filling::Spaces(2),
+        ..Default::default()


seems that you are changing the usage of the API by making default mandatory.
I would prefer we don't have to do that.

sylvestre · 2023-10-20T22:01:44Z

tests/test.rs

    let grid = Grid::new(GridOptions {
        direction: Direction::TopToBottom,
        filling: Filling::Spaces(2),
+        tab_size: 8,


why does it has to be mandatory ? :)
thanks

Because it is stated in the documentation under the category '--tabsize=cols'.

In contrast, the statement in the coreutils "comment" claims that

"Assuming cursor is at position FROM, indent up to position TO. Use a TAB character instead of two or more spaces whenever possible."

So, there are two times (probably more, for the sake of efficiency) where tabs will be used instead of normal spaces:

One is that when using ls, it will replace 8 spaces with 1 tab.

Another is that when using ls -CF, it will replace 2 spaces with 1 tab

sorry, I meant in the API :)

Oh, I think I misread your question earlier. Apologize :)

If I understand correctly, you're asking to keep the original API without modifying it.

I think it can be achieved by introducing a new builder pattern or some sort of struct wrapper. This way, the original API can be maintained while giving users the option to modify the tab size using a stuct wrapper or builder pattern, not directly modify the original API.

That's my thought on how to solve tab's problem. If you have any design in mind, please let me know so that I can write the code and learn. :)

Edit: I will update the new code as I am currently unable to do so.

allaboutevemirolive · 2023-10-30T19:19:58Z

TAB behavior summary:

By default, most shells have one tab stop every 8 spaces.
The number of spaces that a tab represents can vary depending on the editor or environment being used.
Output of tabs and spaces is different between the terminal and text files due to the way that tabs and spaces are interpreted by different environments. As a result, when writing to a text file, the file system interprets tabs differently and they may not be displayed as expected.

When I first started working on this, I was confused because I got different results from terminal and texfile when using TAB. That is why I attempted to change the API to ensure everything was in order, but I was mistaken. TAB is interpreted differently on different platforms and text editors, but has the same length on the terminal (8 spaces).

tertsdiepraam · 2023-10-31T09:42:58Z

Looks like it's going in the right direction, but I don't think it's quite right. Here's what GNU is doing:

❯ ls -Cw18 | bat -A                                                                                                                                                                                         10:38:26
───────┬──────────────
       │ STDIN
───────┼──────────────
   1   │ a├─┤·····b␊
   2   │ aaaaaaaaaaa␊
───────┴──────────────

Note that the separator is still spaces, but that it's instead the alignment that uses tabs. The tabs should also still be configurable and it should be possible to turn this behaviour off. Does that make sense?

sylvestre · 2023-11-28T07:49:35Z

@allaboutevemirolive are you still working on it ? thanks :)

allaboutevemirolive · 2023-11-28T16:12:34Z

@allaboutevemirolive are you still working on it ? thanks :)

I believe the issue already fixed by this #27, or am I misunderstand? :)

@sylvestre

Edit: Minimizing

allaboutevemirolive · 2023-12-07T09:28:41Z

Click to expand/collapse

Looks like it's going in the right direction, but I don't think it's quite right. Here's what GNU is doing:
❯ ls -Cw18 | bat -A                                                                                                                                                                                         10:38:26
───────┬──────────────
       │ STDIN
───────┼──────────────
   1   │ a├─┤·····b␊
   2   │ aaaaaaaaaaa␊
───────┴──────────────
Note that the separator is still spaces, but that it's instead the alignment that uses tabs. The tabs should also still be configurable and it should be possible to turn this behaviour off. Does that make sense?

I think I get it. There is a difference in the visual representation for STDIN.

nemesis@nemesis:~/Documents/Github/Focus/test/tab$ ls -Cw18 | batcat -A
───────┬──────────────────────────────────────────────────
       │ STDIN
───────┼──────────────────────────────────────────────────
   1   │ a....├──┤a␊
   2   │ aaaaaaaaaaa␊
   3   │ a├──┤\u{b7}\u{b7}\u{b7}\u{b7}\u{b7}b␊
   4   │ coreutils␊
───────┴──────────────────────────────────────────────────
nemesis@nemesis:~/Documents/Github/Focus/test/tab$ ls -c
 coreutils  'a....'$'\t''a'   aaaaaaaaaaa  'a'$'\t''·····b'
nemesis@nemesis:~/Documents/Github/Focus/test/tab$ ls -CF
'a....'$'\t''a'   aaaaaaaaaaa  'a'$'\t''·····b'   coreutils*

nemesis@nemesis:~/Documents/Github/Focus/test/tab$ ./coreutils ls -Cw18 | batcat -A
───────┬──────────────────────────────────────────────────
       │ STDIN
───────┼──────────────────────────────────────────────────
   1   │ 'a'$'\t''\u{b7}\u{b7}\u{b7}\u{b7}\u{b7}b'␊
   2   │ 'a....'$'\t''a'␊
   3   │ aaaaaaaaaaa␊
   4   │ coreutils␊
───────┴──────────────────────────────────────────────────
nemesis@nemesis:~/Documents/Github/Focus/test/tab$ ./coreutils ls -c
'a'$'\t''·····b'  'a....'$'\t''a'  aaaaaaaaaaa  coreutils
nemesis@nemesis:~/Documents/Github/Focus/test/tab$ ./coreutils ls -CF
'a'$'\t''·····b'  'a....'$'\t''a'  aaaaaaaaaaa  coreutils*

allaboutevemirolive · 2023-12-16T10:41:21Z

Hye, @sylvestre , I tried to investigate this issue and saw your comment below. I wonder if it is reasonable to integrate uutils-term-grid as an internal library instead of external dependencies. Probably can solve silently ignoring -T option.

tertsdiepraam · 2023-12-16T11:11:55Z

I'll try to set some things straight from my perspective, though @sylvestre might disagree with the final point.

The original issue was never about the separator. It was about outputting a tab for every N spaces, regardless of whether they come from the alignment or the separator. In uutils this should be N=8 by default.
The idea of the tabwidth option in ls is to allow the user to set the tabwidth to the tabwidth of their environment. We do not have to guess the right tabwidth, but we can just assume 8 if that option is not given.
This also means that the comment in that screenshot is wrong! But yes, that issue is related!
I don't think there's an easy way to prevent breaking the API just a bit. In my opinion this is fine and I'll talk to the eza people about it. However, it should be possible to use the current behaviour (without replacing spaces with tabs).

allaboutevemirolive · 2023-12-16T11:54:09Z

I see. Thank you for your explanation. Outputting a tab for every N spaces may work. I will try experimenting with Coreutils to see if this approach works.

So, if it works, I assume that this approach will not require any additional uutils-term-grid modification, and I can work directly on ls.rs.

tertsdiepraam · 2023-12-16T12:07:31Z

I think the best way to do this is still in uutils-term-grid, so we don't really have to replace but can write tabs directly instead of writing spaces. If you do it in ls there are weird edge cases such as filenames containing 8 spaces (which should always be printed with spaces).

sylvestre · 2023-12-25T11:54:12Z

any update on this PR ?
thanks :)

allaboutevemirolive · 2024-01-12T05:12:33Z

I will close this PR temporarily. I believe I should go over issue #5396 and this PR to better understand what needs to be addressed, as well as hardcode (for testing purposes) uutils-term-grid to coreutils because this crate is an external dependency. As @tertsdiepraam pointed out, the tab option should be done in uutils-term-grid.

I will create a new PR or open this issue later.

Add Tab option

051f33b

allaboutevemirolive mentioned this pull request Oct 18, 2023

ls -CF should generate tab and not space uutils/coreutils#5396

Closed

Replace space with tab

d22f3bb

Update to pass CI

80c3aea

sylvestre reviewed Oct 20, 2023

View reviewed changes

Refactoring code and fixing API usage

29eb3de

sylvestre reviewed Oct 20, 2023

View reviewed changes

Keep orginal API and auto add tab

9cfb306

allaboutevemirolive mentioned this pull request Dec 1, 2023

API changes #27

Merged

allaboutevemirolive closed this Jan 12, 2024

rm-dr mentioned this pull request Dec 1, 2024

ls: strange --classify output uutils/coreutils#6897

Closed

Add Tab option #24

Add Tab option #24

Uh oh!

Conversation

allaboutevemirolive commented Oct 18, 2023

Uh oh!

codecov bot commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

allaboutevemirolive commented Oct 18, 2023

Uh oh!

tertsdiepraam commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sylvestre commented Oct 19, 2023

Uh oh!

allaboutevemirolive commented Oct 19, 2023 • edited by sylvestre Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sylvestre commented Oct 19, 2023

Uh oh!

allaboutevemirolive commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sylvestre commented Oct 20, 2023

Uh oh!

sylvestre commented Oct 20, 2023

Uh oh!

sylvestre Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

sylvestre Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

sylvestre Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

allaboutevemirolive Oct 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylvestre Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

allaboutevemirolive Oct 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allaboutevemirolive commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tertsdiepraam commented Oct 31, 2023

Uh oh!

sylvestre commented Nov 28, 2023

Uh oh!

allaboutevemirolive commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allaboutevemirolive commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allaboutevemirolive commented Dec 16, 2023

Uh oh!

tertsdiepraam commented Dec 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allaboutevemirolive commented Dec 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tertsdiepraam commented Dec 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sylvestre commented Dec 25, 2023

Uh oh!

allaboutevemirolive commented Jan 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Add `Tab` option #24

Add `Tab` option #24

codecov bot commented Oct 18, 2023 •

edited

Loading

tertsdiepraam commented Oct 18, 2023 •

edited

Loading

allaboutevemirolive commented Oct 19, 2023 •

edited by sylvestre

Loading

allaboutevemirolive commented Oct 19, 2023 •

edited

Loading

allaboutevemirolive Oct 21, 2023 •

edited

Loading

allaboutevemirolive Oct 21, 2023 •

edited

Loading

allaboutevemirolive commented Oct 30, 2023 •

edited

Loading

allaboutevemirolive commented Nov 28, 2023 •

edited

Loading

allaboutevemirolive commented Dec 7, 2023 •

edited

Loading

tertsdiepraam commented Dec 16, 2023 •

edited

Loading

allaboutevemirolive commented Dec 16, 2023 •

edited

Loading

tertsdiepraam commented Dec 16, 2023 •

edited

Loading