fix: scrollbar thumb not visible on long lists

[ThomasMiz]

The problem

When displaying somewhat-long lists, the Scrollbar widget might not display a single thumb character, and only the track will be visible. Here are some GIFs showing the issue:

The code to reproduce this issue was based off the scrollbar example and can be found in this pastebin.

This PR

I fixed this issue by converting the logic in the part_lengths function to use integer calculations instead of floating-point (the only reason for the use of floating point seemed to be the rounding after division, which can be done in integers by replacing (a / b).round() as usize with (a + b/2) / b) and then doing a bit of clamping. The result of the previous test is as shown in these GIFs:

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b0314c5) 91.9% compared to head (ca183b7) 91.9%.

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #959   +/-   ##
=====================================
  Coverage   91.9%   91.9%           
=====================================
  Files         61      61           
  Lines      15723   15729    +6     
=====================================
+ Hits       14456   14462    +6     
  Misses      1267    1267           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ThomasMiz]

A small thing to note about the pastebin code that should probably be fixed in the examples are lines 55-57:

if key.kind == KeyEventKind::Release {
    continue;
}

Otherwise, on Windows terminals, all events get registered twice

[Valentin271]

Yep can confirm I reproduce. Though important to note this happens in large lists and somewhat small terminal sizes. My fullscreen terminal at 191x46 works fine but 80x27 does not.

[ThomasMiz]

Yep can confirm I reproduce. Though important to note this happens in large lists and somewhat small terminal sizes. My fullscreen terminal at 191x46 works fine but 80x27 does not.

My use case, the reason I found this, is because I have a list of up to 64k log events

[Valentin271]

This looks good to me but I'd like another review.

I had a few comments but definitely non-blocking for me.

[joshka]

I also just hit this bug in an app I'm writing.

I'd like to see one or more tests for this that show the problem (make sure they fail on the previous code before the fix is applied)

[ThomasMiz]

I'd like to see one or more tests for this that show the problem (make sure they fail on the previous code before the fix is applied)

With commit 8baebda I've added tests to check that the thumb is visible. I've tested the previous code, which fails the tests due do no thumb rendering in some cases, and the new code from this PR, which passes the tests.

[joshka]

Thank you for the unit test here - that makes it really easy to confirm that the solution works.

I've provided an alternative approach than keeping things as integers as I think it's genuinely easier to understand.

• used clamp instead of min / max as it's easy to mistake x.min(2) for being at least 2 instead of at most
• renamed variables and added a few intermediate variables to help make the calculations more obvious
• cleaned up the comments
• arranged the parts of the method so that each section is a bit more self-cohesive

[joshka]

@Valentin271 can you take another quick look over this please / @kdheepak ?

[Valentin271]

That's fine by me, I don't mind keeping floats and the code is much easier to understand now 👍

[orhun]

LGTM as well