charmbracelet / lipgloss

Style definitions for nice terminal layouts 👄
MIT License
7.62k stars 208 forks source link

Bug: Truncation in Table Cells #324

Open AkshayKalose opened 1 week ago

AkshayKalose commented 1 week ago

Describe the bug The last character in a table cell is truncated and replaced with even though it would have fit.

Source Code https://go.dev/play/p/Z_uMf1mdLFr

Actual behavior

╭───┬───┬─────┬────╮
│On…│Tw…│Thre…│Fou…│
├───┼───┼─────┼────┤
│on…│tw…│thre…│fou…│
╰───┴───┴─────┴────╯

Expected behavior

╭───┬───┬─────┬────╮
│One│Two│Three│Four│
├───┼───┼─────┼────┤
│one│two│three│four│
╰───┴───┴─────┴────╯
nivanov-ati commented 5 days ago

I'm also seeing this. The regression happened between 0.9.1 and 0.10.0. My guess is that this PR broke it: https://github.com/charmbracelet/lipgloss/pull/256

nivanov-ati commented 5 days ago

@mikelorant You're the original author of the PR - could you please take a look at this? The Go Playground example above is very small and makes it super easy to rep.

mikelorant commented 5 days ago

@nivanov-ati I will take a look a this over the next few days, shouldn't be too difficult to sort out.

mikelorant commented 5 days ago

@aymanbagabas Need some assistance here, this has highlighted a bug in your ANSI package.

package main

import (
    "fmt"

    "github.com/charmbracelet/x/ansi"
)

func main() {
    str := "one"

    out := ansi.Truncate(str, len(str), ".")
    fmt.Println(out)
}

Expected: one Result: on.

Your explanation of the truncate function deviates from how it works:

// Truncate truncates a string to a given length, adding a tail to the // end if the string is longer than the given length.

While we could hack in a fix for this, it would be the wrong approach when we know the problem is likely impacting many other parts of Lipgloss.

jbcpollak commented 4 days ago

this probably doesn't add much value but I added these two test cases to test_truncate.go in the X repo and they fail as expected. Staring at the code for a few minutes I don't see an obvious quick fix:

    {"short", "one", ".", 3, "one"},
    {"shortemoji", "on👋", ".", 3, "on👋"},

Not sure what Charm's process is here but should an issue on the X repo be made?

meowgorithm commented 4 days ago

Thank you @mikelorant for jumping in here and thank, you @jbcpollak, for the test cases. Please do open up a PR on /x.

Ayman's on holiday right now. We may be able to sort this out beforehand, but worst case we can pick it back up when he's back.

jbcpollak commented 4 days ago

I spent more time trying to fix this but its definitely not something I feel super comfortable with. I left some thoughts on the above PR.