Bug: output wraps in terminal (with --icons)

[aarondill]

Here's a reproduction:
Note that the line splitting is put in place by me. It is technically outputting a single line, but because it reaches the end, it wraps. When copying the output, I had to manually insert newlines to mimic the output seen on my terminal.

> echo $COLUMNS
168
> mkdir /tmp/test
> for i in {1..11}; do touch /tmp/test/file-number-$i; done
> cargo run --release /tmp/test/
file-number-1  file-number-2  file-number-3  file-number-4  file-number-5  file-number-6  file-number-7  file-number-8  file-number-9  file-number-10  file-number-11
> cargo run --release /tmp/test/ --icons
 file-number-1   file-number-2   file-number-3   file-number-4   file-number-5   file-number-6   file-number-7   file-number-8   file-number-9   file-number-1
0   file-number-11
> touch /tmp/test/file-number-12
> cargo run --release /tmp/test/
file-number-1  file-number-3  file-number-5  file-number-7  file-number-9   file-number-11
file-number-2  file-number-4  file-number-6  file-number-8  file-number-10  file-number-12
> cargo run --release /tmp/test/  --icons
 file-number-1   file-number-3   file-number-5   file-number-7   file-number-9    file-number-11
 file-number-2   file-number-4   file-number-6   file-number-8   file-number-10   file-number-12
> rm /tmp/test/file-number-* && rmdir /tmp/test # Don't forget to clean up :)

Versioning

Git commit: 5f29705

> cat /etc/os-release
PRETTY_NAME="Ubuntu 22.04.2 LTS"
NAME="Ubuntu"
VERSION_ID="22.04"
VERSION="22.04.2 LTS (Jammy Jellyfish)"
VERSION_CODENAME=jammy
ID=ubuntu
ID_LIKE=debian
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
UBUNTU_CODENAME=jammy
> rustc --version
rustc 1.73.0-nightly (db7ff98a7 2023-07-31)

Does this have something to do with #66 / #83?

Note: the same procedure with exa v0.10.1 [-git]:

> echo $COLUMNS
168
> mkdir /tmp/test
> for i in {1..11}; do touch /tmp/test/file-number-$i; done
> exa /tmp/test/
file-number-1  file-number-2  file-number-3  file-number-4  file-number-5  file-number-6  file-number-7  file-number-8  file-number-9  file-number-10  file-number-11
> exa /tmp/test/ --icons
 file-number-1   file-number-3   file-number-5   file-number-7   file-number-9    file-number-11
 file-number-2   file-number-4   file-number-6   file-number-8   file-number-10  
> touch /tmp/test/file-number-12
> exa /tmp/test/
file-number-1  file-number-3  file-number-5  file-number-7  file-number-9   file-number-11
file-number-2  file-number-4  file-number-6  file-number-8  file-number-10  file-number-12
> exa /tmp/test/  --icons
 file-number-1   file-number-3   file-number-5   file-number-7   file-number-9    file-number-11
 file-number-2   file-number-4   file-number-6   file-number-8   file-number-10   file-number-12
> rm /tmp/test/* && rmdir /tmp/test

[cafkafk]

I'll look at this more tomorrow, what --version of eza are you on?

[aarondill]

@cafkafk ive compiled eza from source (see got commit in version info, HEAD as of issue creation)

[cafkafk]

I was able to reproduce this

[cafkafk]

I'm gonna try to get #83 done first, hopefully it will magically solve this.

[aarondill]

I think the root cause is here in 'src/output/grid.rs`, where we give the contents as the contents, but the file name as the width, leading to a mismatch with icons enabled.

According to the comment, escapes stop us from using the content's width, so perhaps a workaround would be to just add one if --icons are enabled (can we always assume an icon will be shown if enabled?)

            grid.add(tg::Cell {
                contents:  contents.strings().to_string(),
                // with hyperlink escape sequences,
                // the actual *contents.width() is larger than actually needed, so we take only the filename
                width:     filename.bare_width(),
            });

[aarondill]

Back in exa v0.10.1, the code looked like this:

            grid.add(tg::Cell {
                contents:  filename.strings().to_string(),
                width:     *filename.width(),
            });
        }

[aarondill]

Actually, exa's HEAD still uses *filename.width(), so this is a change made here

[cafkafk]

Then https://github.com/cafkafk/eza/pull/17/files#diff-0507c61e0f30892ff1a00883c4669b4a13c41e9377ae9bdf8d3818f38506eb63 is the probable culprit I suppose

[aarondill]

The blame shows this change being in 8196d52. I'm gonna try before this commit to see if I can reproduce the issue

[aarondill]

Checking out before doesn't quite work because the grid was still completely messed up before that, but reverting filename.bare_width() to *contents.width() does seem to solve the issue (though potentially causes other issues?)

[cafkafk]

Checking out before doesn't quite work because the grid was still completely messed up before that, but reverting filename.bare_width() to *contents.width() does seem to solve the issue (though potentially causes other issues?)

It would likely break --hyperlinks?

[aarondill]

very much so.
Before:

> cargo run --release -- --hyperlink
build.rs    Cargo.toml    cliff.toml          completions  file        flake.nix  Justfile  man        rust-toolchain.toml  snap  target       Vagrantfile
Cargo.lock  CHANGELOG.md  CODE_OF_CONDUCT.md  devtools     flake.lock  heloo?h\\  LICENCE   README.md  screenshots.png      src   treefmt.nix  xtests

After:

> cargo run --release -- --hyperlink
build.rs                      file                         rust-toolchain.toml
Cargo.lock       flake.lock  screenshots.png
Cargo.toml       flake.nix    snap
CHANGELOG.md              heloo?h\\               src
cliff.toml                  Justfile      target
CODE_OF_CONDUCT.md  LICENCE                   treefmt.nix
completions       man                  Vagrantfile
devtools             README.md    xtests

[aarondill]

I think the options here would be:

1. Keep track of the length of the file before adding escapes, and add one (/length of the icon) when adding an icon
   • Upside: allows for multi character icons in the future. Allows for other characters/icons to be added/displayed
   • Downside: Additional memory to keep track of the length. Additional developer effort to keep track of the length
2. Pass the current --icons status to the function (somehow), and add one if true
   • Upside: Easy (I assume)
   • Downside: Doesn't allow for future changes to rendered filenames/icons. feels hacky. Could be source of a bug later on?
3. Idk 🤷, open to alternatives 😄

[cafkafk]

Keep track of the length of the file before adding escapes, and add one (/length of the icon) when adding an icon

This might be the preferred solution, but I think it would require a bit of a refactor.

Pass the current --icons status to the function (somehow), and add one if true

I've got something a bit like this in mind (reacting to hyperlink being present in filename.options as well as icons being present there). Perhaps this is the short term solution to go for first. I've got a sketch for a solution for this, I'll create a PR for it in a moment.

[cafkafk]

I think I got this working now