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 division by zero in viewport scrollpercentage calculation #494

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

zMoooooritz
Copy link
Contributor

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
Copy link
Member

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

@zMoooooritz
Copy link
Contributor Author

zMoooooritz commented Mar 10, 2024

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
Copy link
Member

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!

@meowgorithm meowgorithm merged commit 961c081 into charmbracelet:master Mar 11, 2024
9 checks passed
@zMoooooritz zMoooooritz deleted the divisionByZero branch March 11, 2024 21:00
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.

None yet

2 participants