-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fixed unnecessary window displacement on resize #18004
base: master
Are you sure you want to change the base?
Conversation
b8f6dc3
to
3a5cf26
Compare
You can test this PR using the following package version. |
|
@cla-avalonia agree |
@codecat thanks! Can you try to add integrational test for this? This issue seems like something that could be easily caught by tests: See |
Ah, I missed those! Will do. |
3a5cf26
to
be913e3
Compare
You can test this PR using the following package version. |
Test was failing. Had to set my Windows resolution to 800x600 to reproduce this one; apparently the test driver was not actually clicking the buttons because they didn't fit in the window, which can not be scrolled. As a workaround, I simply wrapped all buttons in a horizontal stack panel so that they fit inside the window. The more elegant solution would probably be to make the view scrollable, but that's assuming that the test driver handles that correctly (though it seems to work for the Window page in the test app). Anyway, hope this works now 🤞 |
I'm not sure why the tests are still failing, they pass on my machine, even on 800x600 resolution. Maybe @maxkatz6 has more insights into this? |
The new integration test passes on Windows, but fails on macOS. |
What's your default screen resolution and scaling? Are you using any non-default Windows style, like compact, or high-contrast? |
…n shifting due to incorrect workspace coordinate usage
1e8dc95
to
53d13f3
Compare
I normally run 3840x1600, at 100% scaling, and default Windows 11 styles. I have not tested with different scaling actually, perhaps that might be causing some tests to fail? |
Are you running the integration tests? |
Yes, but only the test I added, which is passing. I have not tested running all of them, in case that changes anything. |
You run all tests, both unit and integration tests. Your change affects all window positioning tests. |
The actual test that is failing is the one I wrote ( I can't get Avalonia to build on Mac currently, but I'll do my best to figure that out. |
You can test this PR using the following package version. |
It looks like on Mac, the Not sure where the real values are supposed to come from, but I'd be happy to include a fix for that in this PR too, if I can figure it out 😅 |
If the fix and test is specifically for Windows, then exclude Mac from the test. |
That would be a lot easier, but would sweep a bug under the rug. I'll do that and open another issue. |
Check if the bug occurs on Mac then. |
You can test this PR using the following package version. |
What does the pull request do?
This fixes unnecessary window displacement on resize
introduced by #16608. Also related: #17559. (Edit: It was introduced sometime earlier even, probably..)What is the current behavior?
When a window on Windows programmatically resizes, it also sets the position for the window placement in workspace coordinates. The coordinates are however fetched from screen coordinates, which causes the window to shift every time the window resizes when the user's taskbar is located on the left or top side of the screen.
Recording.2025-01-20.144204.mp4
For this video, I added a timer to the application that adds 10px to the window height every second using
Height += 10
.What is the updated/expected behavior with this PR?
The window should not be moving, and/or be using workspace coordinates instead of screen space coordinates when using
SetWindowPlacement
.How was the solution implemented (if it's not obvious)?
The struct that's passed into
SetWindowPlacement
is already fetched fromGetWindowPlacement
which returns workspace coordinates. We don't need to assign this at all, as it should already be assigned.Simply removing the code for left & top assignment is enough to fix this.
Checklist
Added XML documentation to any related classes?Doesn't seem relevant here.Consider submitting a PR to https://github.com/AvaloniaUI/avalonia-docs with user documentationFixed issues
This fixes #16217