-
Notifications
You must be signed in to change notification settings - Fork 692
Child window exception fix #2007
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
base: main
Are you sure you want to change the base?
Child window exception fix #2007
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a crash issue when handling multiple windows by replacing the Win32WindowHelper
with OverlappedPresenter
for setting window minimum size constraints. The change eliminates the use of static fields that caused the crashes and adopts the recommended WinUI approach.
Key Changes
- Replaced
Win32WindowHelper
withOverlappedPresenter
for window size constraints - Removed the entire
Win32WindowHelper
class and its Win32 API dependencies - Updated window minimum size setting in both
MainWindow
andTabViewWindowingSamplePage
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
TabViewWindowingSamplePage.xaml.cs | Updated to use OverlappedPresenter instead of Win32WindowHelper for setting minimum window size |
MainWindow.xaml.cs | Replaced Win32WindowHelper usage with OverlappedPresenter for window constraints |
Win32WindowHelper.cs | Completely removed the helper class that was causing crashes with multiple windows |
(window.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumWidth = 640; | ||
(window.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumHeight = 500; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast to OverlappedPresenter could fail if the presenter is not of that type, potentially causing a NullReferenceException. Consider adding a null check or type check before setting the properties.
(window.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumWidth = 640; | |
(window.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumHeight = 500; | |
if (window.AppWindow.Presenter is OverlappedPresenter overlappedPresenter) | |
{ | |
overlappedPresenter.PreferredMinimumWidth = 640; | |
overlappedPresenter.PreferredMinimumHeight = 500; | |
} | |
else | |
{ | |
// Handle the case where the presenter is not an OverlappedPresenter | |
System.Diagnostics.Debug.WriteLine("The presenter is not of type OverlappedPresenter."); | |
} |
Copilot uses AI. Check for mistakes.
(window.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumWidth = 640; | ||
(window.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumHeight = 500; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast to OverlappedPresenter could fail if the presenter is not of that type, potentially causing a NullReferenceException. Consider adding a null check or type check before setting the properties.
(window.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumWidth = 640; | |
(window.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumHeight = 500; | |
var presenter = window.AppWindow.Presenter as OverlappedPresenter; | |
if (presenter != null) | |
{ | |
presenter.PreferredMinimumWidth = 640; | |
presenter.PreferredMinimumHeight = 500; | |
} |
Copilot uses AI. Check for mistakes.
(this.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumWidth = 640; | ||
(this.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumHeight = 500; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast to OverlappedPresenter could fail if the presenter is not of that type, potentially causing a NullReferenceException. Consider adding a null check or type check before setting the properties.
(this.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumWidth = 640; | |
(this.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumHeight = 500; | |
var overlappedPresenter = this.AppWindow.Presenter as OverlappedPresenter; | |
if (overlappedPresenter != null) | |
{ | |
overlappedPresenter.PreferredMinimumWidth = 640; | |
overlappedPresenter.PreferredMinimumHeight = 500; | |
} |
Copilot uses AI. Check for mistakes.
(this.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumWidth = 640; | ||
(this.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumHeight = 500; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast to OverlappedPresenter could fail if the presenter is not of that type, potentially causing a NullReferenceException. Consider adding a null check or type check before setting the properties.
(this.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumWidth = 640; | |
(this.AppWindow.Presenter as OverlappedPresenter).PreferredMinimumHeight = 500; | |
var presenter = this.AppWindow.Presenter as OverlappedPresenter; | |
if (presenter != null) | |
{ | |
presenter.PreferredMinimumWidth = 640; | |
presenter.PreferredMinimumHeight = 500; | |
} |
Copilot uses AI. Check for mistakes.
@AndrewKeepCoding I believe these APIs do not take into account DPI by default? |
In addition to @niels9001 concern, wouldnt it be easier to have the functions in WindowHelper be and resolve the hwnd by passing in a Window? It doesnt look like we need any of the state the way the functions work right now and keep that little state inside the functions. |
@niels9001 @marcelwgn Thanks for the feedback! Would this work better? We should also consider integrate it to the other helper class internal class Win32WindowHelper
{
private const double DefaultDpi = 96.0f;
public static void SetWindowMinMaxSize(Window window, POINT? minWindowSize = null, POINT? maxWindowSize = null)
{
if (window.AppWindow.Presenter is not OverlappedPresenter presenter)
{
System.Diagnostics.Debug.WriteLine("SetWindowMinMaxSize: AppWindow.Presenter is not OverlappedPresenter");
return;
}
var hWnd = WinRT.Interop.WindowNative.GetWindowHandle(window);
var dpi = Win32.GetDpiForWindow(hWnd);
if (minWindowSize is not null)
{
presenter.PreferredMinimumWidth = (int)(minWindowSize.Value.x * dpi / DefaultDpi);
presenter.PreferredMinimumHeight = (int)(minWindowSize.Value.y * dpi / DefaultDpi);
}
if (maxWindowSize is not null)
{
presenter.PreferredMaximumWidth = (int)(maxWindowSize.Value.x * dpi / DefaultDpi);
presenter.PreferredMaximumHeight = (int)(maxWindowSize.Value.y * dpi / DefaultDpi);
}
}
internal struct POINT
{
public int x;
public int y;
}
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @AndrewKeepCoding ,
I previously worked on DPI awareness using PreferredMinimumWidth
and PreferredMinimumHeight
, and I believe I have a solid workaround. I didn’t implement it earlier because it seemed too complex at the time, and the current implementation appeared sufficient. However, since it’s now breaking the app, I suggest we consider this workaround.
You can find the related code here: DPI-Aware
From my findings, we should focus on two key areas:
- Making window size constraints DPI-aware using
GetDpiForWindow
and applyingscaleFactor = dpi / 96.0
(see this PR: Zakariathr22/DPI-Aware#1) - Handling dynamic DPI changes by intercepting the
WM_DPICHANGED
message (see this PR: Zakariathr22/DPI-Aware#2)
I'd also like to know what @niels9001 and @marcelwgn think about this.
I'm starting to think that we should also consider simplifying this "child window sample" and just use a plain |
I think your proposed approaches are fine (grouping them since they are virtually doing the same) @AndrewKeepCoding and @Zakariathr22. Regarding the DPI changes and grabbing the DPI factor, I think we should use the XAMLRoot.RasterizationScale property so we dont have to rely on calling into Win32 APIs. Since the XAMLRoot also comes with a changed events arg, we would also be able to deal with the DPI changes. |
Description
This PR replaces
Win32WindowHelper
withOverlappedPresenter
to set window max size.Motivation and Context
The
Win32WindowHelper
relied on static fields, which led to app crashes when handling multiple windows. TheOverlappedPresenter
, as demonstrated in the AppWindow sample page, is the recommended approach to setting window size constraints.Note: This PR supersedes #2005, which was closed due to an incomplete approach.
Fixes: #2003
How Has This Been Tested?
Ran the app and verified the changes.
Screenshots (if appropriate):
Types of changes