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

Allow a win32 window's restored size to be defined even when it is maximised or minimised #14470

Merged

Conversation

TomEdwardsEnscape
Copy link
Contributor

@TomEdwardsEnscape TomEdwardsEnscape commented Feb 3, 2024

This PR rewrites WindowImpl.Resize to use SetWindowPlacement, which allows us to set the size of the window even when it is minimised or maximised.

Breaking changes

None

Obsoletions / Deprecations

None

Fixed issues

Fixes #13300

@hez2010
Copy link
Contributor

hez2010 commented Feb 4, 2024

I don't think this is the correct fix.
A minimized window is not an actual window but a placeholder without any size information on it.
If we want to change the size before the window shows up, I will rather expect to save the size information and resize it when users restore the window state to normal. But I think this should be handled by the app developer, not the framework itself.

@TomEdwardsEnscape
Copy link
Contributor Author

A minimized window is not an actual window but a placeholder without any size information on it.

It might be a placeholder, but nevertheless the size of a minimised window is stored by Windows and is sent back to the application as a WM_SIZE message when that window is restored.

This is the behaviour that Avalonia expects, so I think that removing the inconsistency between minimised and non-minimised windows is a good approach.

If we want to change the size before the window shows up, I will rather expect to save the size information and resize it when users restore the window state to normal. But I think this should be handled by the app developer, not the framework itself.

This is OK in principle, but it doesn't map well onto the Avalonia API, in which windows have public Width and Height properties that can be read and written at any time. Should we start throwing exceptions when those properties are set while the window is minimised?

@hez2010
Copy link
Contributor

hez2010 commented Feb 4, 2024

This is OK in principle, but it doesn't map well onto the Avalonia API, in which windows have public Width and Height properties that can be read and written at any time. Should we start throwing exceptions when those properties are set while the window is minimised?

The behavior in other win32 applications is just to return the window size before it was last minimized, and any resize operation is ignored after the window was minimized. This is exactly what Avalonia currently doing, so I'm not sure whether this should be fixed.

@TomEdwardsEnscape
Copy link
Contributor Author

Avalonia doesn't ignore the resize. It accepts it the new value for its own window object without transferring it to the OS, which is inconsistent. It should either apply the new value in both places, or in neither (i.e. throw an exception).

@thevortexcloud
Copy link
Contributor

thevortexcloud commented Feb 5, 2024

It should either outright reject the change or buffer it from the sound of things. Quickly making it visible then invisible feels like a hack to me, and I imagine on some slow systems may lead to noticeable flickering. If you do go with rejecting the change, you probably also need to add a new property/method that says if the window can be resized. CanResize is currently settable by user code and indicates if the end user is allowed to resize the window.

@maxkatz6 maxkatz6 requested a review from emmauss February 6, 2024 00:50
@TomEdwardsEnscape
Copy link
Contributor Author

I don't think the window would ever appear, because we are executing on the UI thread and thus blocking it. But yeah, buffering the value is the (more complicated) alternative if we don't like re-minimising.

I'm not a fan of blocking changes to the properties because these limitations may not exist on other platforms.

@emmauss
Copy link
Contributor

emmauss commented Feb 8, 2024

Wouldn't it be better to defer resizing to when the window is restored?

@StefanKoell
Copy link
Contributor

Coming from WinForms, I know there's a concept called "RestoreBounds" on a form:
https://learn.microsoft.com/en-us/dotnet/api/system.windows.window.restorebounds?view=windowsdesktop-8.0

A Rect that specifies the size and location of a window before being either minimized or maximized.

Maybe something like this would be a good approach.

@thevortexcloud
Copy link
Contributor

It sounds like most people would prefer buffering the value until the window is restored.

@jp2masa
Copy link
Contributor

jp2masa commented Feb 8, 2024

Can't this be fixed using SetWindowPlacement?

@TomEdwardsEnscape
Copy link
Contributor Author

TomEdwardsEnscape commented Feb 8, 2024

Can't this be fixed using SetWindowPlacement?

Well well well:

WPF_SETMINPOSITION: The coordinates of the minimized window may be specified. This flag must be specified if the coordinates are set in the ptMinPosition member.

That's our solution, and is apparently also what WPF does to handle this exact scenario.

WPF stands here for "Window Placement Flags". So it's not the silver bullet I had expected. Still worth looking at though.

@TomEdwardsEnscape TomEdwardsEnscape force-pushed the fixes/resize-minimised-win32-window branch from 866c181 to 7633f87 Compare February 9, 2024 08:55
@TomEdwardsEnscape
Copy link
Contributor Author

SetWindowPlacement worked! It supports setting the window size of a minimised window without restoring it.

@@ -1426,6 +1450,23 @@ private static void EnableCloseButton(IntPtr hwnd)
MF_BYCOMMAND | MF_ENABLED);
}

private RECT ClientRectToWindowRect(RECT clientRect, WindowStyles? styleOverride = null, WindowStyles? extendedStyleOverride = null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are several other uses of AdjustWindowRectEx which could be redirected to this method. These other locations currently don't account for per-window DPI, so using this method may fix some bugs. But I decided not to make unnecessary changes in this PR.

@avaloniaui-bot
Copy link

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

@TomEdwardsEnscape TomEdwardsEnscape force-pushed the fixes/resize-minimised-win32-window branch from 797927b to 4defeb2 Compare February 9, 2024 13:31

// do comparison after scaling to avoid rounding issues
if (requestedClientWidth != clientRect.Width || requestedClientHeight != clientRect.Height)
if (_lastWindowState == WindowState.FullScreen)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still can't resize a fullscreen window, but I don't think this will ever be supported for the reasons outlined in the comment here. This is not a regression, it's the same behaviour as on master.

@avaloniaui-bot
Copy link

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

@TomEdwardsEnscape TomEdwardsEnscape force-pushed the fixes/resize-minimised-win32-window branch from 4defeb2 to bc36c4c Compare February 9, 2024 14:55
@avaloniaui-bot
Copy link

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

@TomEdwardsEnscape TomEdwardsEnscape changed the title Allow minimised win32 windows to be resized Allow a win32 window's restored size to be defined even when it is maximised or minimised Feb 10, 2024
@TomEdwardsEnscape
Copy link
Contributor Author

@emmauss this is ready for review again.

@maxkatz6
Copy link
Member

LGTM, just want to have @emmauss opinion as well, as it might affect XPF.

@emmauss
Copy link
Contributor

emmauss commented Feb 20, 2024

Could you rebase your branch?

@TomEdwardsEnscape TomEdwardsEnscape force-pushed the fixes/resize-minimised-win32-window branch from bc36c4c to 92ab0e3 Compare February 20, 2024 08:16
@avaloniaui-bot
Copy link

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

@maxkatz6 maxkatz6 enabled auto-merge February 20, 2024 23:37
@maxkatz6
Copy link
Member

@TomEdwardsEnscape it looks like there are conflicts. It cannot be automatically resolved, and we can't push to your remote branch.

auto-merge was automatically disabled February 21, 2024 08:03

Head branch was pushed to by a user without write access

@TomEdwardsEnscape TomEdwardsEnscape force-pushed the fixes/resize-minimised-win32-window branch from 92ab0e3 to e2f0b68 Compare February 21, 2024 08:03
@avaloniaui-bot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Window does not take into account the specified Height and Width if it is launched in the minimised state
8 participants