Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix truncate of table cells containing ANSI #256

Merged
merged 1 commit into from
Feb 3, 2024

Conversation

mikelorant
Copy link
Contributor

Table cells (including the header) may contain ANSI codes. When determining the width of these cells, ANSI was not excluded.

The reflow package has the same truncate function that is aware of ANSI. This improves cell width calculation and correctly truncates.

@mikelorant
Copy link
Contributor Author

@charmbracelet Can I get a review for this very small bug fix. Thanks.

@mikelorant
Copy link
Contributor Author

@maaslalani Would you be able to do a review of this fix? Thanks.

@maaslalani
Copy link
Contributor

Hey @mikelorant, this looks great! Do you mind adding a test for ANSI truncation so this doesn't regress again?

@mikelorant
Copy link
Contributor Author

Take a look at the tests file and you'lll see why this is going to be a really big problem.

Options:

  • Switching the tests to use golden files in a testdata directory.
  • Accept confusing ANSI codes in the plain text multi line strings.
  • Add a separate test specifically to test strings that use lipgloss.NewStyle(). This gets more difficult with tests are termenv detects no ANSI capability when running tests but can be worked around.

@maaslalani
Copy link
Contributor

maaslalani commented Feb 2, 2024

Take a look at the tests file and you'lll see why this is going to be a really big problem.

Options:

  • Switching the tests to use golden files in a testdata directory.
  • Accept confusing ANSI codes in the plain text multi line strings.
  • Add a separate test specifically to test strings that use lipgloss.NewStyle(). This gets more difficult with tests are termenv detects no ANSI capability when running tests but can be worked around.

I would go with the 3rd option to test strings that have lipgloss.NewStyle or hand code ANSI sequences in the strings (with the help of a few constants from termenv). If we go with the lipgloss.NewStyle() we'll likely have to force the color profile to be set to ANSI256 in one of the tests so the ANSI sequences show up.

Table cells (including the header) may contain ANSI codes. When
determining the width of these cells, ANSI was not excluded.

The `reflow` package has the same truncate function that is aware of
ANSI. This improves cell width calculation and correctly truncates.

Signed-off-by: Michael Lorant <michael.lorant@nine.com.au>
@mikelorant
Copy link
Contributor Author

Had a little fun with the test by making the header an incredibly long ANSI string.

Have confirmed this test fails on the master branch without the fix.

❯ go test ./...
--- FAIL: TestTableANSI (0.00s)
    table_test.go:975: expected:

        ┌───────────┬────────┬──────┐
        │   Fruit   │ Color  │ Code │
        ├───────────┼────────┼──────┤
        │ Apple     │ Red    │ 31   │
        │ Lime      │ Green  │ 32   │
        │ Banana    │ Yellow │ 33   │
        │ Blueberry │ Blue   │ 34   │
        └───────────┴────────┴──────┘

        got:

        ┌───────────┬────────┬──────┐
        │   Fruit   │ Color  │   C│
        ├───────────┼────────┼──────┤
        │ Apple     │ Red    │ 3…   │
        │ Lime      │ Green  │ 3…   │
        │ Banana    │ Yellow │ 3…   │
        │ Blueberry │ Blue   │ 3…   │
        └───────────┴────────┴──────┘
FAIL
FAIL	github.com/charmbracelet/lipgloss/table	0.515s
FAIL

With the fix, test passes:

=== RUN   TestTableANSI
--- PASS: TestTableANSI (0.00s)

@maaslalani
Copy link
Contributor

Nice work on this one @mikelorant, thank you so much!

@maaslalani maaslalani merged commit de46012 into charmbracelet:master Feb 3, 2024
9 checks passed
@mikelorant mikelorant deleted the fix/ansi-truncate branch February 3, 2024 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants