Fix WinForms Icon Button Size Handling #4051
Conversation
Adding the test first, as per usual TDD. Using CI to run it since my machine is a bit loaded right now.
|
FYI -- pushing an unrelated trivial fix because it's been coming up in testbed runs as a bit of a noise, and is easy to repair. Ready for review iff CI passes. |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Looks good to me. I've fixed one minor markup issue, removed the extra test (as it should be covered by existing tests), and modified the example app to demonstrate (or not, as the case may be) the problem.
More generally, we need to formalise this sort of "get me a 32x icon" API; at present, it's being done ad-hoc through each backend, which isn't ideal. However, we can address that as follow up work.
changes/4051.bugfix.2.md
Outdated
| @@ -0,0 +1 @@ | |||
| ``Screen.as_image`` now properly accounts for HiDPI scaling on Winforms. | |||
There was a problem hiding this comment.
Markdown only requires a single quote
| ``Screen.as_image`` now properly accounts for HiDPI scaling on Winforms. | |
| `Screen.as_image` now properly accounts for HiDPI scaling on Winforms. |
testbed/tests/widgets/test_button.py
Outdated
| image""" | ||
| widget.icon = "resources/icons/huge" | ||
| await probe.redraw("Button now has a high-resolution image as icon") | ||
| probe.assert_icon_size() |
There was a problem hiding this comment.
AFAICT, this test shouldn't be needed - red.png is already 72x72 px, and L81 of this file is asserting that the icon is 32x32. With the fix to the assertion to correct for scaling, that test should be sufficient.
There was a problem hiding this comment.
@freakboy3742 Surprisingly when ran on my machine, the red.png thing actually passed. It think it was better to have a guarantee, since the .ico lookup issue on Linux Fedora shows us that the icon lookup order isn't what we think it is sometimes.
2 participants
WinForms icons in buttons aren't scaled to 32x32 like all other platforms are.
Adds a test for this by adding an intentionally high-res image and making sure icon size is still right.
Also includes unrelated fix.
PR Checklist: