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

Reduce WinUIEx references #2934

Merged
merged 1 commit into from
May 17, 2024
Merged

Reduce WinUIEx references #2934

merged 1 commit into from
May 17, 2024

Conversation

nieubank
Copy link
Contributor

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

@@ -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);
Copy link
Contributor

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.

Copy link
Contributor

@AmelBawa-msft AmelBawa-msft May 16, 2024

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

@dhoehna
Copy link
Contributor

dhoehna commented May 16, 2024

This looks fine to me. Is it possible to check this in after the build snaps for //BUILD?

@nieubank
Copy link
Contributor Author

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.

@krschau
Copy link
Collaborator

krschau commented May 16, 2024

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.

dabhattimsft
dabhattimsft approved these changes May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce unneeded references to WinUIEx
5 participants