feat(viewport): auto-wrap

[kyu08]

Resolves #479
Resolves #644

The issue I experienced

I found that example app of viewport can't render properly if content has very long line.

If the content using in the example's was like below in the toggle, even if the content was scrolled to the bottom, the example can't render all contents. The content of last line(This is the last line.) is not rendered.

Content for reproduce

## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test
## test
test

012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789

This is the last line.

What this PR fixes

This PR fixes the problem to fix height calculation method.

Without this change a very long line that exceeds width is not considered. New method considers it.

[kyu08]

@caarlos0
[Gentle ping]
If you have time to re-review this PR, please review my correction. If commits should be squashed I will do it.
Thank you.

[bashbunni]

Hey @kyu08 thanks so much for this contribution. It looks good and all the tests are passing. Thank you as well for including a test with the changes, makes this PR super easy to review.

I just tried running some bubble tea examples that use viewport and found some issues. If you run the glamour example, then resize to a smaller window and scroll down, you'll see that there's a lot of extra whitespace. If you keep scrolling down you get a runtime error: slice bounds out of range [:41] with capacity 40.

Let me know if you're able to reproduce that issue :)

[kyu08]

Hey @kyu08 thanks so much for this contribution. It looks good and all the tests are passing. Thank you as well for including a test with the changes, makes this PR super easy to review.

Thanks for your kind words! I am also grateful to maintainers like you for making this project available.

Also, thank you for finding the bug.

If you run the glamour example, then resize to a smaller window and scroll down, you'll see that there's a lot of extra whitespace. If you keep scrolling down you get a runtime error: slice bounds out of range [:41] with capacity 40.

I fixed that issue ( eaff7e7 ) and the another issue that percentage counter is not working properly.( 5e1cef4 )

I noticed that we should not use m.lines, but rather the lines that are actually displayed. As I wrote as code comment, if there is a line that is longer than the width, it will be displayed in multiple lines. But I didn't consider it at the start of this PR.

Could you re-review them please? (If my changes are okey, I can squash commits if needed.)

[kyu08]

SetContent also needed to be fixed. eb7b9d5

[kyu08]

@bashbunni
Hi, I would be very appreciate if you rereview my changes when you get a chance. Thank you.

[bashbunni]

Hey @kyu08 will do when I'm able to give some attention to bubbles in the next little bit. Usually I alternate focuses on a weekly basis, so will let you know when I'm on an open source maintenance week :D

[bashbunni]

That said, I'm very eager to include this in v0.20.0, so will definitely be taking a look when I can

[bashbunni]

Hey I just refactored to make the names more clear + wrap words rather than cutting them. I also added a test case to reflect that.

I was getting an out of range error on this if I sized the terminal window very small, scrolled down to the very bottom, then expanded it as large as possible. Was just an issue with the YOffset exceeding its max. Fixed with a WindowSizeMsg check in the Update

[kyu08]

@bashbunni
Thanks so much for fixing naming and updating logics. It looks better than mine.

The tests failing seems like fixed by this:

diff --git a/viewport/viewport.go b/viewport/viewport.go
index 05e93ae..332134b 100644
--- a/viewport/viewport.go
+++ b/viewport/viewport.go
@@ -423,7 +423,7 @@ func wrap(lines []string, width int) []string {
 		// wrap lines (handles lines that could not be word wrapped)
 		wrap := ansi.Hardwrap(wrapWords, width, true)
 		// split string by new lines
-		wrapLines := strings.Split(strings.TrimSpace(wrap), "\n")
+		wrapLines := strings.Split(wrap, "\n")
 
 		out = append(out, wrapLines...)
 	}
diff --git a/viewport/viewport_test.go b/viewport/viewport_test.go
index 0dc7974..3863255 100644
--- a/viewport/viewport_test.go
+++ b/viewport/viewport_test.go
@@ -31,6 +31,11 @@ func TestWrap(t *testing.T) {
 			width: 12,
 			want:  []string{"hello bob, I", "like yogurt", "in the", "mornings."},
 		},
+		"whitespace of head of line is preserved": {
+			lines: []string{" aaa", "bbb", "ccc"},
+			width: 5,
+			want:  []string{" aaa", "bbb", "ccc"},
+		},
 	}

How about deleting strings.TrimSpace(wrap) like above?
I think there is no problem because ansi.Wordwrap and ansi.Hardwrap trims spaces of tail of lines.

[kyu08]

@bashbunni
I pushed a diff I suggested and verify that unit tests have passed on my machine by running go test ./....

If you have a moment, could you please check this pr again? If there are no problems, I would appreciate it if you could merge this PR.
Thank you.

[josebalius]

Running into this when using glamour to render. Looking forward to the fix.

[meowgorithm]

Thanks for your attention to this one @Kyu and for your keen attention to detail @bashbunni.

A couple quick questions:

1. Does this PR wrap long lines at render time?
2. Does manually wrapping long lines outside of this PR solve the issue?

[meowgorithm]

What I’m getting at is that first and foremost, it sounds like this issue can be solved by wrapping or truncating manually with Lip Gloss (iirc, historically this burden was on the user). I think we do indeed want to offer built-in options for the viewport not to accidentally break, however we probably want to offer a few different options as wrapping won’t always be want the user wants.

[meowgorithm]

Another thing to consider with this one is how automatically wrapping will affect existing implementations. Consider that Huh and textarea both depend heavily on viewport and auto-wrapping may break existing functionality (it may not but it's worth checking and considering).

Anyway, to do this one right we'll need to offer a few different wrapping options beyond wrapping as a default

1. Truncate (just chop off the end)
2. Truncate with ellipsis (put a … at the end of the truncated line, the actual character should be configurable)
3. Wrap
4. No automatic correction

To be safe, I'd probably truncate by default so as not to break any existing implementations.

Also, in the short term @kyu08 and @josebalius it's trivial auto-wrap text in your current implementations: see charmbracelet/bubbletea#1185 for an example.

wrapped := lipgloss.NewStyle().Width(yourWidth).Render(yourContent)
viewport.SetContent(wrapped)

[meowgorithm]

This assumes that the width of the viewport is tied to the window width, which will not always be the case: viewports can have a fixed size. Additionally, this requires the user to flow tea.WindowSizeMsgs through a viewport's update. It would be better to simply re-wrap when the width of the viewport changes.

In order to achieve this we'll probably need to deprecate Width and introduce SetWidth() and a less-than-ideal GetWidth(). In v2 we can correct the naming.

[josebalius]

Also, in the short term @kyu08 and @josebalius it's trivial auto-wrap text in your current implementations: see charmbracelet/bubbletea#1185 for an example.

@meowgorithm thank you! ❤️

[bashbunni]

Hey @kyu08 thank you so much for this contribution! Given that viewport is used in a wide variety of cases where it's rendering more than just text, this feature would be breaking for a lot of users.

Given that, we are going to close this PR, but will work on improving the UI of viewport in a non-breaking way. That will likely be with better truncation and potentially horizontal scrolling.

Thank you for your support 🫶

[kyu08]

@meowgorithm
I'm sorry that I didn't reply to your review. I thought I have to take time to investigate scope to influenced by this PR to reply. But I could not take enough time.

@bashbunni
OK. I have no objection to closing this PR. 👍
I understand that this PR may affect to use case like rendering elements other than text.

Thanks for reviewing!