handle ansi style in cell content

[blueforesticarus]

intends to supersede #77
would close #91

[blueforesticarus]

@Nukesor could you approve the CI workflow?

[codecov-commenter]

Codecov Report

Merging #93 (2d53b1b) into main (dce08d6) will decrease coverage by 0.03%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##             main      #93      +/-   ##
==========================================
- Coverage   96.05%   96.02%   -0.04%     
==========================================
  Files          17       17              
  Lines        1547     1610      +63     
==========================================
+ Hits         1486     1546      +60     
- Misses         61       64       +3     

Impacted Files	Coverage Δ
src/utils/mod.rs	100.00% <ø> (ø)
src/utils/formatting/content_split.rs	98.48% <96.05%> (-1.52%)	⬇️
src/cell.rs	98.48% <100.00%> (+0.04%)	⬆️
src/row.rs	100.00% <100.00%> (ø)
src/utils/formatting/content_format.rs	99.36% <100.00%> (+0.02%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Nukesor]

@blueforesticarus You can also test the CI on your fork by enabling Actions over there :)

[blueforesticarus]

@Nukesor What do you want the logic to be when the tty feature is not enabled?
Currently I am using crossterm to identify the reset ansi code, but it looking at the crossterm source it appears to be constant.

Edit: I just hard coded the constant so I wouldn't need cross-term. That fixes tests and linter problems.
The CI on my fork snagged on coverage checks but it looks the test failed to initialize, guess I'd need to setup codecov for my fork?

[Nukesor]

No need to setup codecov :)
As long as the other Actions succeed, you should be good to go.

Regarding the tty question, did you introduce any new libraries that contain unsafe code? If so, this could be a problem, as people rely on the current dependency tree to be fully safe and non-panicable if tty is disabled.

Otherwise, I don't see a big problem with including it in the default feature set.

Did you try the build_large_tables benchmark? If the new logic significantly increases the runtime of this benchmark, we should probably think about hiding this logic behind a feature flag (if possible).

[blueforesticarus]

Looks like console has a few unsafe blocks, but its all in term support and nothing I am using touches it.

[blueforesticarus]

     Running benches/build_large_table.rs (target/release/deps/build_large_table-81383246e255f825)
Huge table              time:   [18.715 ms 18.734 ms 18.755 ms]
                        change: [+24.626% +25.179% +25.712%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

     Running benches/build_tables.rs (target/release/deps/build_tables-b79d9b1e4fa9749e)
Readme table            time:   [27.808 µs 27.830 µs 27.856 µs]
                        change: [+22.215% +22.616% +23.027%] (p = 0.00 < 0.05)
                        Performance has regressed.

Big table               time:   [454.56 µs 455.41 µs 456.49 µs]
                        change: [+8.5173% +8.9623% +9.3673%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 19 outliers among 100 measurements (19.00%)
  7 (7.00%) high mild
  12 (12.00%) high severe

[blueforesticarus]

@Nukesor let me know if that bench result is acceptable or if you want to make a feature flag.

[Nukesor]

Sorry for being so slow to respond 😓

I'm currently in the middle of a job transition and really don't have a lot of time to work on my open source projects.
I'll try to get to it as soon as possible :)

[Nukesor]

A ~22% performance regression for a feature that isn't being used by many users isn't that nice.

I would definitely opt to hide this behind a feature flag!
That way, we can also mark this as experimental and prevent any backward compatibility breakages.

In general, this doesn't look too bad, but it definitely needs a lot of tests :D

[Nukesor]

Early continue for better readability

[Nukesor]

Please write proper inline documentation on what this algorithm does.
It's by no means trivial and quite hard to read/understand right now when one is not familiar with how Ansi escape sequences work.

Try to chunk the algorithm into logical blocks and explain what each block is doing in a sentence or two.

Also, please write docs on what these variables are for.

[blueforesticarus]

@Nukesor I think I addressed everything

[Nukesor]

Nice :)

I'll review it as soon as I find some time to do so!

[Nukesor]

First of all, thanks for your work and time!

I'm going to merge this soon, but there were still some open issues I wanted to address first.
Since this already took so long (due to me), I took the liberty to build upon your MR and create a second MR with a few commits on top. I didn't touch any of your commits though!

Feel free to check out my additions over here:
#108