-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Handlers] Register handlers and use Microsoft.Extensions for Hosting and Dependency Injection #12460
Conversation
src/Platform.Handlers/samples/Sample/ViewModel/ViewModelBase.cs
Outdated
Show resolved
Hide resolved
/azp run |
No pipelines are associated with this pull request. |
/azp run |
No pipelines are associated with this pull request. |
src/Platform.Handlers/tests/Xamarin.Platform.Handlers.UnitTests/TestClasses/MockViewHadler.cs
Outdated
Show resolved
Hide resolved
b1e3fa0
to
b6ff3f9
Compare
/azp run |
Pull request contains merge conflicts. |
/azp run |
No pipelines are associated with this pull request. |
src/Platform.Handlers/tests/Xamarin.Platform.Handlers.Benchmarks/HandlersBenchmarker.cs
Outdated
Show resolved
Hide resolved
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 took a casual look through some of the major pieces. Just some initial thoughts.
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Xamarin.Platform.Handlers.csproj
Outdated
Show resolved
Hide resolved
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Hosting/Internal/AppHostLifetime.cs
Outdated
Show resolved
Hide resolved
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Hosting/AppBuilder.cs
Outdated
Show resolved
Hide resolved
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Hosting/IStartup.cs
Outdated
Show resolved
Hide resolved
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Hosting/AppBuilder.cs
Outdated
Show resolved
Hide resolved
if (_handlersServiceProvider != null) | ||
servicesCollection.AddSingleton<IHandlerServiceProvider>(_handlersServiceProvider); | ||
|
||
var app = (TApplication)Activator.CreateInstance(typeof(TApplication)); |
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.
Why isn't the Application created through DI
? That way it can participate in DI and get services itself.
if (_hostBuilderContext != null) | ||
AppLoader.ConfigureAppServices<TApplication>(_hostBuilderContext, servicesCollection, app); | ||
|
||
var services = servicesCollection.BuildServiceProvider(); |
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.
Is it intentional to have 2 service containers? This one and the one created in CreateServiceProvider()
?
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.
yes our service container for handlers is registering using a different way, the service doesn't need to implement the interface
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Hosting/Internal/AppHost.cs
Outdated
Show resolved
Hide resolved
04cb214
to
97064d0
Compare
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Xamarin.Platform.Handlers.csproj
Outdated
Show resolved
Hide resolved
2b314b9
to
ddbc51a
Compare
Use the null-forgiving operator (!) to rather swear that we doin' good
Also split the benchmarks into separate classes for better comparison.
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Platform/IMauiContext.cs
Show resolved
Hide resolved
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Core/IWindow.cs
Outdated
Show resolved
Hide resolved
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Platform/IMauiContext.cs
Outdated
Show resolved
Hide resolved
{ | ||
public abstract class MauiApp : App | ||
{ | ||
public abstract IWindow GetWindowFor(IActivationState state); |
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.
Just interested to know what this state will do... How does it affect the place when I have multiple windows or is this just for that first window?
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.
All Windows, state will be platform dependent and can have things like intent, url link, state to restore etc.. everything that will allow the user to return the window that is needed, a new one or a existing one after restoring the state.
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.
Maybe the confusion here is that it's called GetWindowFor
and it should be called CreateNewWindowFor
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.
But if it gets the windows that is already visible...
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.
Right Create seems it's always new, could be a Window already exists ?!
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.
It doesn't get already visible, it's supposed to be when the platform asks you to create a new window for the given state/args ... this is to deal with the fact that on android and iOS you aren't always in control of when a window instantiation is requested. On iOS, as a user, you can use the task manager to request that an app creates a new window for you.
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Hosting/MauiServiceProvider.cs
Show resolved
Hide resolved
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Hosting/Internal/AppHost.cs
Show resolved
Hide resolved
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Hosting/Internal/AppHost.cs
Show resolved
Hide resolved
src/Platform.Handlers/src/Xamarin.Platform.Handlers/Hosting/Internal/ServiceFactoryAdapter.cs
Show resolved
Hide resolved
[Application] | ||
public class MainApplication : MauiApplication<MyApp> | ||
{ | ||
public MainApplication(IntPtr handle, JniHandleOwnership ownerShip) : base(handle, ownerShip) |
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.
@dellis1972 it looks like they are planning on using this for default Maui apps
Is there still some issue with Fast Deployment and a custom application class?
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.
There is but only for Enhanced Fast Deployment. That is where we fast deploy the dex files. It just won't work since we need the app class (java) to be in the classes.dex in the apk. Which it won't be. There is a possible way around it but it would involve allot of work.
Description of Change
Adding the
Host
concept similar to ASPNET , this will allow to use the containers and providers for our handlers as well for services during the application, will open possiblity to use asConfiguration
with appsettings file, or other features ofIHost
like a built-in lifecycle and services, logging extensions etc.Adding the
App
abstract class that allows the basic for working with the AppBuilder. Also we would haveMauiApp
that allows to work with Handlers and Windows.IWindow
Windows must be provided by the user by implementing
IWindow GetWindowFor(IActivationState state)
on there ownMauiApp
class.IAppHostBuilder
Users can register services and configure the default
AppHostBuilder
with there own services or configurations, this can be done by overiding `virtual IAppHostBuilder CreateBuilder()´User must register Windows as
IWindow
, or overrideWindows
on App class.Example:
The AppBuilder might be hidden from the user , but we will allow the user to override it, ant the user can extend by registering native services, configure logging , register new handlers.
Example : Register a custom handler to override the default one, configure native services
Using the Microsoft Dependency Injection to register handlers in a our own
ServiceCollection
implementation based on a dictionary. We assume that we only want 1 handler for a specific type then the use of dictionary. We still allow users to override it by just registering there own factory container ofIServiceProviderFactory<ServiceCollection>
type.Example to use the default ServiceCollection instead of the Xamarin.Forms one
This PR also adds a couple of things
Maui.Controls.Sample that is just the Handlers + Maui.Controls
Add benchmark project to better get a sense of improvements on MAUI non specific platform code.
Add
profile-android.ps1
to help measure startup time on the Android application.usage:
Current baseline to Register and Get Handlers via Dependency Injection system and a basic Dictionary
Current StartupTime using Release/AOT using Google Pixel 2
Issues Resolved
API Changes
Trying to map IApp/MauiApp to Application on Android and AppDelegate. on iOS while Windows will map to Acitivites and SceneDelegates.
The user can register it's own IHandlerServiceProvider and override our handler system. This provider shouldn't validate that implementation implements the required service, since we Register IButton,ButtonHandler, but ButtonHandler doesn't implement IButton, this is just for mapping one type to the other.
Special ServiceCollection based on Dictionary
This might exist for all IFrameworkElements and can be platform specific, will consolidate all the info to needed to generate a renderer from a IView, we might also be adding the Dispatcher here too
A point where we can get handlers our other services, HotReload service could also be retrived via
App.Services
The IMauiContext will move to IFrameworkElement
The IAppHostBuilder implementation, when we call Build(app) we get an IAppHost
Platforms Affected
Behavioral/Visual Changes
None
Before/After Screenshots
Not applicable
Testing Procedure
Unit tests present
PR Checklist