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(table): correctly truncate escaped cells #603

Conversation

Broderick-Westrope
Copy link

@Broderick-Westrope Broderick-Westrope commented Aug 28, 2024

Fixes #502.

This pull request introduces new utility functions for handling terminal escape sequences and updates the table rendering logic to use these utilities. It also includes comprehensive unit tests for the new functions. Much of the new utility functions is derived from here (I made the assumption that this project is aiming for minimal dependencies).

New Utility Functions for Escape Sequences:

  • table/escape_utils.go: Added functions to handle terminal escape sequences, including truncate, lengthWithoutEscapeSequences, extractEscapeSequences, and applyEscapeSequences.

Tests for New Utility Functions:

  • table/escape_utils_test.go: Added tests for truncate, lengthWithoutEscapeSequences, extractEscapeSequences, and applyEscapeSequences to ensure they work correctly with various types of strings, including those with escape sequences, emojis, and combining characters.

Updates to Table Rendering Logic:

  • table/table.go: Replaced runewidth.Truncate with the new truncate function in the headersView and renderRow methods to properly handle escape sequences. [1] [2]

Assumptions:

  • When truncating a string an ellipsis (...) is used. When truncating, any escape sequences from the cut-off portion of the string are instead placed at the end of the truncated string. Closing these is import. The assumption I've made is that when the ellipsis is appended to the truncated string it will not be placed within these closing ellipsis. The consequences this are:
    • If for example the original string was to be colored red. The truncated string will still be colored red however the ellipsis will be the default text color (likely either white or black depending on terminal theme).
    • If the user has used a nonsensical combination of escape sequences which prevents the terminal from rendering the string, the ellipsis will still be shown. This is only true if the user included closing sequences (which is the users' responsibility).

@Broderick-Westrope Broderick-Westrope marked this pull request as ready for review August 28, 2024 22:19
@bashbunni bashbunni self-requested a review September 10, 2024 13:00
@Broderick-Westrope
Copy link
Author

May be redundant due to #246
Needs further investigation.

@bashbunni
Copy link
Member

bashbunni commented Sep 20, 2024

Hey @Broderick-Westrope, thank you so much for your help in improving bubbles. This functionality already exists in x/ansi, but can probably be improved with the additional test cases you wrote. If you're interested in contributing these tests over there, that would be nice! I do think that text truncation shouldn't be in the scope of the bubbles library. #502 should be fixed when we migrate the bubbles table to use the Lip Gloss table for rendering. If it's not, then we would likely want to fix that in Lip Gloss with x/ansi

@Broderick-Westrope
Copy link
Author

Broderick-Westrope commented Sep 20, 2024

Thanks @bashbunni, I'll raise a new PR with some test cases in the ANSI package. I'd be happy to help with the migration towards using lipgloss table, if you need the extra hands.

EDIT: After looking at the existing x/ansi tests I don't think my tests would add any value.

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.

How to define the color of a specified cell in a table
2 participants