charmbracelet / bubbles

TUI components for Bubble Tea 🫧
MIT License
5.52k stars 265 forks source link

Fix division by zero in viewport scrollpercentage calculation #494

Closed zMoooooritz closed 7 months ago

zMoooooritz commented 7 months ago

In the current implementation one gets a division by zero in case len(m.lines) == m.Height + 1, which in turn makes the function return NaN. Removing the -1 does avoid this error and still yields sensible results.

meowgorithm commented 7 months ago

Nice, thanks for this. Just so we can verify this fix, can you share some code that reproduces the issue this solves for?

zMoooooritz commented 7 months ago

It can indeed be easily reproduced by taking the pager example and replacing line 71 with m.viewport = viewport.New(msg.Width, 6) and setting the value of content in the main function to Test0\nTest123\nTest123\nTest123\nTest123\nTest123\nTest9.

code for error reproduction

While further investigating this I have learned more concerning this error:

  1. NaN is returned when 0 / 0 is calculated
  2. On all other X / 0 +Inf or -Inf is returned depending on the sign of X (which causes no problems for us since we are clamping the values to [0, 1])

Therefore the NaN is only returned in the special case of one line more content than the number of lines of the viewport and the scroll is positioned at the start.

meowgorithm commented 7 months ago

Glorious; thanks for the detailed explanation and example. This patch looks good and I was able to verify the fix. Thanks for the contribution, @zMoooooritz!