comfy-table icon indicating copy to clipboard operation
comfy-table copied to clipboard

Add support for cells with ansi codes.

Open thvdveld opened this issue 2 years ago • 9 comments

I wanted to use comfy-table with cells that contain ANSI escape characters, generated with the colored crate. However, when calculating the width of a cell, the ANSI escape characters were counted as well, resulting in funny looking tables.

By using textwrap's function to calculate the display width of a String, the tables have the correct cells and display correctly.

thvdveld avatar Jun 20 '22 12:06 thvdveld

This isn't planned, as Cells already support coloring itself. Working with already colored code would significantly increase the crate's complexity and I don't really see the need for this, as we already support coloring.

To support this behavior, we would have to:

  1. Add a ANSI escape code parser to our library
  2. Add a parser for other environments (windows shells, which use different escape codes as far as I understand). This would be the first platform dependent code in our library, if we cannot find a crate that supports all platforms.
  3. Always parse all text, even though this will probably only happen in <1% of all usecases. This will slow everything down significantly.

Why can't you just use comfy-table's built-in coloring mechanism?

Nukesor avatar Jun 21 '22 07:06 Nukesor

I needed it because I had to give different colours and styling to different parts of the text in one Cell, instead of one style to the whole Cell.

thvdveld avatar Jun 21 '22 08:06 thvdveld

Codecov Report

Merging #77 (dba5b55) into main (c32a346) will increase coverage by 0.25%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
+ Coverage   97.22%   97.48%   +0.25%     
==========================================
  Files          14       14              
  Lines         794      794              
==========================================
+ Hits          772      774       +2     
+ Misses         22       20       -2     
Impacted Files Coverage Δ
src/row.rs 100.00% <100.00%> (ø)
src/utils/arrangement/dynamic.rs 100.00% <100.00%> (+1.49%) :arrow_up:
src/utils/formatting/content_format.rs 100.00% <100.00%> (ø)
src/utils/formatting/content_split.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 73ab7cf...dba5b55. Read the comment docs.

codecov-commenter avatar Jun 22 '22 23:06 codecov-commenter

I'm pretty sure your current approach wont' work yet, as we have to consider the dynamic content adjustment mode.

-> In that case, any string can be split at any point and distributed along multiple lines.

For this to work, we would have to:

  • Split each string at the correct word depending on the sub-string length (whilst ignoring ANSI escape sequences)
  • ANSI escape sequences would have to be closed early for this line. This means that we have to track the current state of styling, depending on a given position in the string.
  • That styling would then have to be continued on the next line up to the point it should end.

I would really like to see several tests on this. Try to come up with weird edge-cases, such as

[Start black color] some text [start red background] other text, [blinking style] continue text [reset color] continue text [reset background]

Then go ahead and try to split this at every possible position and make sure that the styling is still as it should be. There're probably a handful of other weird edge-cases that one can come up with arbitrary ANSI escape codes.

Nukesor avatar Jun 22 '22 23:06 Nukesor

Right, I forgot to write about that part. Indeed, that is still an issue with this PR. But for now, one liners will work. I will think about how to handle other cases.

thvdveld avatar Jun 23 '22 06:06 thvdveld

Depending on the code-complexity for the solution you find, I would be ok with merging this feature.

However, if this significantly complicates the current code, I would be very much against it. The current optimization logic for text layout is already super complex and I wouldn't feel comfortable maintaining it, if it got even more complex :laughing:

Nukesor avatar Jun 23 '22 10:06 Nukesor

I don't really have time to work on this. However, I think these changes are useable for people that only have data on one line (and thus not spanning over multiple lines, which is the part that really complicates it). I agree this cannot be merged, but maybe you could point out this PR to other people that need it for just one line.

thvdveld avatar Sep 02 '22 07:09 thvdveld

Just came across this looking for a way to color individual words inside a cell. Is there another option?

jannschu avatar Sep 04 '22 14:09 jannschu

Sadly not yet. If you don't need the feature of dynamically arranging your content to a given width, you could check out alternative table implementations, which are linked at the bottom of the readme :)

Nukesor avatar Sep 04 '22 18:09 Nukesor

split_long_words would also have to be changed. https://github.com/Nukesor/comfy-table/commit/9c01db6c0b318b6e1d8619fdb8022688ea4fca65

blueforesticarus avatar Sep 26 '22 16:09 blueforesticarus

I've been thinking about this, and what is the rational for the library supporting styling at all?

There are lots of libraries which provide that functionality (console, termion, crossterm, etc.), and using those to style the text being put in the cell doesn't seem any more difficult than styling the cell. If this library handled ansi codes properly, it might simplify this library alot to only focus on alignment / layout / border style. That is if you were okay breaking backward compatibility.

blueforesticarus avatar Sep 27 '22 14:09 blueforesticarus

@Nukesor Independent of my comment above, I'm willing to try to get a pull request for this ready. A) Is the console dependency acceptable to you? (it has an iterator for escape codes, which textwrap does not ) B) Do you want this logic (and the dependency) behind a feature? C) Would you be willing to merge this with only ANSI escape code support? (Assuming it didn't break existing functionality on windows shells)

My patch seems to break alot of tests, so I'll have to fix those cases too.

blueforesticarus avatar Sep 27 '22 15:09 blueforesticarus

've been thinking about this, and what is the rational for the library supporting styling at all?

There are lots of libraries which provide that functionality (console, termion, crossterm, etc.), and using those to style the text being put in the cell doesn't seem any more difficult than styling the cell. If this library handled ansi codes properly, it might simplify this library alot to only focus on alignment / layout / border style. That is if you were okay breaking backward compatibility.

The reason why comfy-table does its own styling, is because it's pretty hard to do the dynamic arrangement with arbitrary styling on top of it. The text arrangement is done based on text length and splitting by delimiters. ANSI escape sequences make this pretty tricky, as they need to be ignored when doing all those computations.

One has to format the text as if there aren't any escape sequences, including splitting the string at any point without breaking the formatting.

I simply didn't have this requirement in my planned feature set when starting to build this library. This only came up recently via feature requests. It doesn't seem to be common to do varying styling in a single cell.

A) Is the console dependency acceptable to you? (it has an iterator for escape codes, which textwrap does not )

I don't mind adding another dependency, if it works :D

B) Do you want this logic (and the dependency) behind a feature?

I don't see the need to hide this behind a feature flag. As I said above, I just don't want to further significantly complicate the "dynamic content adjustment", as it's already complex as hell. I recently wrote a few tests for some edge-cases and immediately found two bugs, which tells me that this is already too complex...

C) Would you be willing to merge this with only ANSI escape code support? (Assuming it didn't break existing functionality on windows shells)

I guess? As long as it simply doesn't work on Windows and doesn't break it over there, we should probably be fine :D.

Nukesor avatar Oct 02 '22 12:10 Nukesor

Superseded by efforts in #93

Nukesor avatar Nov 28 '22 23:11 Nukesor