-
Notifications
You must be signed in to change notification settings - Fork 357
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
Reduce WinUIEx references #2934
Conversation
@@ -135,7 +138,11 @@ public App() | |||
|
|||
// Main window: Allow access to the main window | |||
// from anywhere in the application. | |||
services.AddSingleton<WindowEx>(_ => MainWindow); | |||
services.AddSingleton(_ => MainWindow); |
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.
I have an issue to do exactly this, but I do have some enhancements we can do as well 😊
Because the Window and the DispatcherQueue are sometime referenced/injected in testable classes, we should consider mapping an interface to them.
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.
Something like this:
AddSingleton<IAppWindow>(_ => MainWindow);
AddSingleton<IDispatcherQueue>(_ => MainWindow.DispatcherQueue);
Another option is to add a reference to the MainWindow
and DispatcherQueue
in IApp
so we can do:
GetService<IApp>().MainWindow
This looks fine to me. Is it possible to check this in after the build snaps for //BUILD? |
yep, no rush to get this in. Once I have the all-clear for BUILD changes I'll merge. |
At this point, things have gone in that aren't going into Build, so if we did need to break the glass we'd be doing a cherry-pick anyway and checking this in wouldn't matter. |
Summary of the pull request
WinUIEx is primarily used in Dev Home for easy handling of MinWidth and MinHeight of main and secondary windows and for remembering window placement of the main window across launches (PersistenceId). As WinUI improves these dependencies will be unnecessary and this dependency can eventually be removed since the intention of WinUIEx is to highlight gaps in WinUI rather than replace or extend it.
Most of the usage of WinUIEx in Dev Home is actually using the WindowEx to get access to the DispatcherQueue to queue work on the UI thread. We don't actually need to expose the WindowEx for that since the Microsoft.UI.Xaml.Window has that itself. In fact, most consumers don't even care about the Window so we can easily just register the DispatcherQueue itself to avoid overexposure of the Window and callers can be more explicit. We should in the longer-term reconsider access to the Dispatcher and why so many components need the UI thread... I suspect a lot of this can be further simplified. The goal of this PR is to just reduce the scope of the dependencies.
I also took the liberty of cleaning up some references that were unused as I found them when removing the using WinUIEx statements as well.
The two convenience methods from WinUIEx that were used GetWindowHandle and CenterOnScreen were pulled into WindowExExtensions (didn't rename for easier change tracking, we can rename this to WindowExtensions when fully removing WinUIEx down the road.)
PI coming in brings quite a few new dependencies on WinUIEx but nothing insurmountable. These similar principals should apply to that project as well but given the churn there I did not make many changes to anything in that project.
References and relevant issues
Detailed description of the pull request / Additional comments
Validation steps performed
I've going through the testing scenarios checklist and ensured that secondary windows as well as PI is unaffected.
PR checklist