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 right padding calculation for horizontal centering #520

Closed
wants to merge 1 commit into from

Conversation

kquick
Copy link
Contributor

@kquick kquick commented Nov 29, 2024

The calculations for right padding on horizontal centering don't seem right:

If availWidth is 30, and rWidth is 10, then leftPaddingAmount will be 10. From this, remainder will be 10 and rightPaddingAmount will be 20, which is too large, giving a total width of 10 + 10 + 20 = 40.

Similarly, if availWidth is 30 and rWidth is 11, then leftPaddingAmount will be 8. From this, remainder will be 14, and rPaddingAmount will be 22, giving a total width of 8 + 11 + 22 = 41.

This patch provides a new calculation for rPaddingAmount, which will be 10 and 12, respectively, both giving a resulting length of 30 which matches availWidth.

This does not produce any visible differences on rendering simple scenarios because vty will apply cropping to the Image generated, but it may provide invalid width values for general calculations if there is a Widget to the left or right of the centered Widget.

It may be that vCenterWith has the same type of error; I did not investigate that function in detail.

jtdaugherty added a commit that referenced this pull request Nov 30, 2024
@jtdaugherty
Copy link
Owner

Looking at the code, I think the fix is even smaller than your patch. The value remainder was intended only to capture 0 or 1 columns left over due to the amount of padding (left + right) not being evenly divisible by 2. So the fix is

-               remainder = max 0 $ c^.availWidthL - (leftPaddingAmount * 2)
+               remainder = max 0 $ c^.availWidthL - (rWidth + (leftPaddingAmount * 2))

which makes the calculation for rightPaddingAmount work out the way I intended: if the left padding times 2 plus rWidth leaves a "remainder" of 1 column, that needs to be added to the right padding.

I'm really surprised I haven't seen this misbehave before!

jtdaugherty added a commit that referenced this pull request Dec 1, 2024
@jtdaugherty
Copy link
Owner

Whoops, pushed the patch to the wrong branch. I just cherry-picked it to master.

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.

2 participants