Skip to content

Refactor and improve ScrollbarMargin.create_margin #1517

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ilius
Copy link
Contributor

@ilius ilius commented Oct 30, 2021

This PR refactors and improves the readability of scrollbar rendering code.

And also fixes an issue which is commented in the code

    scrollbar_height = min(
        window_height,
        max(1, int(math.ceil(window_height * fraction_visible))),
    )
    # Without math.ceil, the scrollbar will never reach the end.
    # Unless you increase the scrollbar height by one (like it was done
    # before, implicitly) which makes it less accurate, and may also
    # cause a misconception that there are no items after the last visible
    # item when scrollbar reaches the end before the last item is visible
    scrollbar_top = int(window_height * fraction_above)
    scrollbar_bottom = window_height - scrollbar_height - scrollbar_top

If you test it carefully, you realize that scrollbar height was (usually) bigger by one that what should be (for example if menu height is 16 and you have 32 items in menu, the shown scrollbar height is 9 rather than 8).

@ilius ilius force-pushed the PR-ScrollbarMargin-refactor branch 6 times, most recently from e041c4f to de7b934 Compare October 30, 2021 14:59
@ilius ilius force-pushed the PR-ScrollbarMargin-refactor branch from de7b934 to 7552e13 Compare November 13, 2021 01:45
@ilius ilius force-pushed the PR-ScrollbarMargin-refactor branch from 7552e13 to 50415ee Compare November 25, 2021 18:26
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.

1 participant