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

Use muesli/reflow to compute str width & truncate #68

Merged
merged 2 commits into from
May 2, 2022
Merged

Use muesli/reflow to compute str width & truncate #68

merged 2 commits into from
May 2, 2022

Conversation

greglanthier
Copy link
Contributor

PR related to #66.

Based on this comment I looked in the muesli/reflow library for utilities to compute string width & do truncation while accounting for ANSI control sequences.

I would be happy to resubmit this PR using lipgloss instead if you'd prefer.

Based on [this comment](#66 (comment)) I looked in
the muesli/reflow library for utilities to compute string width &
do truncation while accounting for ANSI control sequences.

I would be happy to resubmit this PR using lipgloss instead if you'd
prefer.
@Evertras
Copy link
Owner

Evertras commented May 2, 2022

I'm fine with truncate instead of lipgloss and in fact prefer this version as I think it's more focused, but I want to confirm a test case where the string has an ANSI reset sequence at the end. Will this properly include the ANSI reset sequence? My concern is that, for example, a string is turned bright red and reset, but truncating it will remove the reset so the following border and text is turned bright red. It appears that the truncate package will handle this properly, but can we add a test case to confirm this works as intended and to demonstrate this behavior for any future refactors/changes?

@greglanthier
Copy link
Contributor Author

You raise an interesting point about truncate and how it might treat ANSI reset sequences.

Is this the behaviour you're expecting?

output := limitStr("\x1b[31;41mte\x1b[0mst", 3)
// output == "\x1b[31;41mte\x1b[0m…"   // note that the `\x1b[0m` sequence is preserved...

The muesli/reflow packages appear to behave as one would expect. So long as the ANSI reset sequence appears prior to the first printable rune where the truncation is to occur the rest of the table (borders and so on) will render with the expected style.

I'll note that lipgloss.NewStyle().SetString(str).MaxWidth(maxLen-1).String() + "…" appears to inject an ANSI reset control sequence on its own before the "…" rune if str contains ANSI colour control sequences. This is another reason I dug around in the mesli/reflow package for an alternative. At the time I wasn't sure if it was desirable to rely on that behaviour to preserve styling (even though that is the purpose of lipgloss as far as I understand) - but the option of letting lipgloss deal with the ANSI sequences exists.

I updated the tests in the original PR. Do those changes provide enough coverage of the intended behaviour? I would be happy to add some other test cases as well - perhaps in areas related to rendering cell borders - but I wanted to keep the PR confined as much as possible.

@Evertras
Copy link
Owner

Evertras commented May 2, 2022

I think in this particular case, if the user is embedding their own ANSI codes, it's up to them to also provide the reset. Either:

  • They're trying to do something very particular by avoiding the reset code, in which case I would rather avoid different behavior when truncating versus not
  • They've forgotten to insert the reset code, in which case that's their own fault and they'll see it regardless of whether the string was truncated or not so it'll be fixed quicker :)

I think this solution properly keeps the intent of the user for whatever final reset codes they wanted to provide while also keeping the total truncation proper from a visible width perspective. The only questionable thing remaining is whether the final ... should try and use the last escape code seen in the string or always be reset... but honestly I think this is fine for default behavior for now, particularly in such an edge case. If there are desires to use the last escape code that can be a future PR.

Thanks a lot for the contribution and thorough discussion! I'll never say no to more test cases, but I think this is great for now.

@Evertras Evertras merged commit af9e08b into Evertras:main May 2, 2022
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