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

Fixed unnecessary window displacement on resize #18004

Merged
merged 5 commits into from
Feb 1, 2025

Conversation

codecat
Copy link
Contributor

@codecat codecat commented Jan 20, 2025

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 from GetWindowPlacement 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

Fixed issues

This fixes #16217

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054345-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@cla-avalonia
Copy link
Collaborator

cla-avalonia commented Jan 20, 2025

  • All contributors have signed the CLA.

@codecat
Copy link
Contributor Author

codecat commented Jan 20, 2025

@cla-avalonia agree

@maxkatz6 maxkatz6 requested a review from emmauss January 20, 2025 15:13
@codecat
Copy link
Contributor Author

codecat commented Jan 20, 2025

Ah, I missed those! Will do.

@codecat codecat force-pushed the fix/resize-workspace-position branch from 3a5cf26 to be913e3 Compare January 21, 2025 09:53
@codecat
Copy link
Contributor Author

codecat commented Jan 21, 2025

I've added a new integration test now, and force pushed this PR so the commits are matching the contributor guidelines:

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054375-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@codecat
Copy link
Contributor Author

codecat commented Jan 21, 2025

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 🤞

@codecat
Copy link
Contributor Author

codecat commented Jan 21, 2025

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?

@MrJul
Copy link
Member

MrJul commented Jan 23, 2025

I'm not sure why the tests are still failing, they pass on my machine, even on 800x600 resolution.

The new integration test passes on Windows, but fails on macOS.
Either the bug is also present on that platform (I'll check), or we need to wait a bit for the new position to be properly reported.

@emmauss
Copy link
Contributor

emmauss commented Jan 27, 2025

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?

What's your default screen resolution and scaling? Are you using any non-default Windows style, like compact, or high-contrast?
Also could you rebase so the tests run again.

@codecat codecat force-pushed the fix/resize-workspace-position branch from 1e8dc95 to 53d13f3 Compare January 27, 2025 08:54
@codecat
Copy link
Contributor Author

codecat commented Jan 27, 2025

What's your default screen resolution and scaling?

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?

@emmauss
Copy link
Contributor

emmauss commented Jan 27, 2025

What's your default screen resolution and scaling?

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?

@codecat
Copy link
Contributor Author

codecat commented Jan 27, 2025

Yes, but only the test I added, which is passing. I have not tested running all of them, in case that changes anything.

@emmauss
Copy link
Contributor

emmauss commented Jan 27, 2025

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.

@codecat
Copy link
Contributor Author

codecat commented Jan 27, 2025

The actual test that is failing is the one I wrote (Changing_Size_Should_Not_Change_Position), specifically on Mac, which I have not ran the tests on locally yet.

I can't get Avalonia to build on Mac currently, but I'll do my best to figure that out.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054560-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@codecat
Copy link
Contributor Author

codecat commented Jan 27, 2025

It looks like on Mac, the Width and Height getters are returning default values rather than the actual window size (from _effectiveValues). In this case, it is even returning NaN.

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 😅

@emmauss
Copy link
Contributor

emmauss commented Jan 27, 2025

If the fix and test is specifically for Windows, then exclude Mac from the test.

@codecat
Copy link
Contributor Author

codecat commented Jan 27, 2025

That would be a lot easier, but would sweep a bug under the rug. I'll do that and open another issue.

@emmauss
Copy link
Contributor

emmauss commented Jan 27, 2025

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.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054562-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@codecat
Copy link
Contributor Author

codecat commented Jan 27, 2025

Check if the bug occurs on Mac then.

Sorry, I was talking about the Mac-specific bug that I filed in #18060 now 😄

If you're wondering about the issue that this PR fixes, no, it is not reproducible on Mac.

@codecat codecat force-pushed the fix/resize-workspace-position branch from 670229f to 423339b Compare January 30, 2025 08:13
@codecat
Copy link
Contributor Author

codecat commented Jan 30, 2025

To match the notes raised in #18060, I've slightly updated the integration app code.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054635-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

I've checked that #13300 (which initially introduced the bug) is still working after this fix.
Nice work adding an integration test.

LGTM, thank you!

@MrJul MrJul added this pull request to the merge queue Feb 1, 2025
Merged via the queue into AvaloniaUI:master with commit 29fbaaa Feb 1, 2025
10 checks passed
@codecat codecat deleted the fix/resize-workspace-position branch February 1, 2025 15:44
@MrJul MrJul added the backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch label Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch bug os-windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Window position shifts when size changes with taskbar on left or top
6 participants