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 alignment of number pad buttons #2240

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

Conversation

leduyquang753
Copy link

@leduyquang753 leduyquang753 commented Oct 6, 2024

Fixes another instance of #127

Description of the changes

  • Add ExtraColumns property to NumberPad control.
  • Add an extra column to NumberPad control instances in standard, scientific and programmer button panels.

Additional information

The NumberPad control is placed as sort of a "subgrid" within the button panels' grids, on a subset of columns. This causes slight differences in how the column widths are computed in the outer button panel and in the number pad control (see CGrid::DistributeStarSpace) and makes the number pad buttons slightly misaligned with the surrounding buttons. Adding extra empty columns to the right of the NumberPad so that it reaches the right side of the button panel fixes this inconsistency.

How changes were validated

Manual testing.

@leduyquang753
Copy link
Author

@microsoft-github-policy-service agree

@tian-lt
Copy link
Contributor

tian-lt commented Oct 11, 2024

Hi @leduyquang753, thank you for starting this PR. I've seen that the problem of misalignment buttons is being mitigated a lot in your source branch. However, there are still some cases can reproduce the issue. For example:

Screenshot 2024-10-11 112515
image

Do you have any ideas to improve the fix?
Thanks again.

Copy link
Contributor

@tian-lt tian-lt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest improving the fix since the bug can still be reproduced.

@tian-lt
Copy link
Contributor

tian-lt commented Oct 31, 2024

Thank you for the suggestion, @Will-J-Bailey.
I agree with you that this PR can be considered as a partial fix. We can have it merged if there's no regressions.
The root cause of the alignment problem may be related to the cascaded visual structure of button groups. Fixing the problem may require a non-trivial refactoring from the numpad to the very top-level view - Calculator.
@hanzhang54, could you take a look at this PR as well?

Copy link
Contributor

@tian-lt tian-lt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as a temporary solution.

@hanzhang54
Copy link
Contributor

Thank you all for reporting the issue and working on the fix.

Actually, the buttons appear to be aligned as expected on my device. I'm not sure if the discrepancy stems from operating system differences, but it appears more like a WinUI issue rather than a problem with the Calculator. This workaround looks pretty "complex" for simple UI like this. I'd suggest addressing this issue within WinUI rather than making changes to the Calculator.

Calculator version: 11.2409.0.0
image
image

@leduyquang753
Copy link
Author

leduyquang753 commented Oct 31, 2024

It seems that the author of the pull request has been inactive for several days

I have been very busy lately, but I did spend an amount of time looking further into this and unfortunately I had yet to find the deeper cause.

Actually, the buttons appear to be aligned as expected on my device. I'm not sure if the discrepancy stems from operating system differences, but it appears more like a WinUI issue rather than a problem with the calculator. This workaround looks pretty "complex" for simple UI like this. I'd suggest addressing this issue within WinUI rather than making changes to the calculator.

So indeed this is really something from WinUI's layout system itself, specifically the rounding to the nearest screen pixel part. At the current state of this fix, the misalignment seems to only manifest itself when Windows scaling is at non-multiples of 100% (so 125%; 150%;...). Therefore I think there are three paths we can take:

  1. Investigate the exact mechanics with which WinUI rounds the positions of components, then continue to refine the workaround (probably not the most preferrable, I initially thought this workaround was sufficient but turns out there's a much deeper rabbit hole).
  2. Rearchitecture the button pad so buttons are directly laid out into the primary grid (may sacrifice some reusability).
  3. Wait for an improvement within the WinUI layout system itself so that a pixel-perfect layout can be feasibly achieved (may take forever).

@hanzhang54
Copy link
Contributor

Your investigation and effort are really appreciated. Unfortunately, I still cannot repro it at both 125 and 150 scale. I'd prefer to report this to WinUI at first as the root cause is unclear and the mitigation is kind of heavy.

@tian-lt What's your opinion?

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.

3 participants