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

[android] fix DI registration for Frame, ListView, TableView #16989

Merged

Conversation

jonathanpeppers
Copy link
Member

This partially reverts b0bba51.

When running:

.\bin\dotnet\dotnet.exe build src\Controls\samples\Controls.Sample\Maui.Controls.Sample.csproj -f net7.0-android -t:Run

The app crashes on the "Compatibility" screen with:

System.InvalidOperationException: No service for type 'Android.Content.Context' has been registered.

These are caused by the change:

--handlersCollection.AddHandler(typeof(Frame), typeof(Handlers.Compatibility.FrameRenderer));
++handlersCollection.AddHandler<Frame>(svc => new Handlers.Compatibility.FrameRenderer(svc.GetRequiredService<Android.Content.Context>()));

It turns out this code path only works because:

  1. Microsoft.Extensions.DI throws a MissingMethodException

  2. MAUI has fallback logic to catch this and call:

return (IElementHandler)Extensions.DependencyInjection.
    ActivatorUtilities.CreateInstance(mauiContext.Services, handlerType, mauiContext.Context);

ActivatorUtilities is part of Microsoft.Extensions.DI.

For now, let's just revert this change for Android for these three types: Frame, ListView, and TableView.

I spend some time trying to "unwind" this and avoid the MissingMethodException on startup. I couldn't solve it yet, but plan to come back to it later.

This partially reverts b0bba51.

When running:

    .\bin\dotnet\dotnet.exe build src\Controls\samples\Controls.Sample\Maui.Controls.Sample.csproj -f net7.0-android -t:Run

The app crashes on the "Compatibility" screen with:

    System.InvalidOperationException: No service for type 'Android.Content.Context' has been registered.

These are caused by the change:

    --handlersCollection.AddHandler(typeof(Frame), typeof(Handlers.Compatibility.FrameRenderer));
    ++handlersCollection.AddHandler<Frame>(svc => new Handlers.Compatibility.FrameRenderer(svc.GetRequiredService<Android.Content.Context>()));

It turns out this code path only works because:

1. Microsoft.Extensions.DI throws a `MissingMethodException`

2. MAUI has fallback logic to catch this and call:

    return (IElementHandler)Extensions.DependencyInjection.
        ActivatorUtilities.CreateInstance(mauiContext.Services, handlerType, mauiContext.Context);

`ActivatorUtilities` is part of Microsoft.Extensions.DI.

For now, let's just revert this change for Android for these three
types: `Frame`, `ListView`, and `TableView`.

I spend some time trying to "unwind" this and avoid the
`MissingMethodException` on startup. I couldn't solve it yet, but plan
to come back to it later.
@PureWeen PureWeen enabled auto-merge (squash) August 24, 2023 19:58
@PureWeen
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

#if ANDROID
svc.GetRequiredService<Android.Content.Context>()
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if you are able to get the maui context, and then get the android contextfrom that. The maui context should be registered in the services?

Copy link
Member

Choose a reason for hiding this comment

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

But I am not sure which maui context it will be - it may be the root app one, or the latest/nearest...

Copy link
Member Author

Choose a reason for hiding this comment

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

svc.GetService<IMauContext>() returns null at this point for some reason?

Copy link
Member

Choose a reason for hiding this comment

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

yea, the service passed in here is just the MauiHandlerFactory that implements IServiceProvider. It doesn't really have any exciting information or access to the relevant WrappedServiceProvider

Currently all these controls are completely broken so just doing this for now to get unbroken.

This is partially why I was hoping we'd shift to a dependency resolution route. If the ToHandler code used ActivatorUtilities then we could look at dependency resolution to make this work.

I think it's still worth seeing if we can modify this factory approach to provide the WrappedServiceProvider inside this code block before we print NET8.

@PureWeen PureWeen merged commit 136f21d into dotnet:main Aug 25, 2023
34 checks passed
PureWeen added a commit that referenced this pull request Aug 29, 2023
PureWeen added a commit that referenced this pull request Aug 30, 2023
* Revert "[core] factory methods for registering services, part 2 (#17004)"

This reverts commit 79a44ff.

* Revert "[android] fix DI registration for Frame, ListView, TableView (#16989)"

This reverts commit 136f21d.

* Revert "[core] use factory methods for registering services (#16741)"

This reverts commit b0bba51.

# Conflicts:
#	src/Controls/src/Xaml/Hosting/AppHostBuilderExtensions.cs
#	src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt

* Revert "Revert "[core] factory methods for registering services, part 2 (#17004)""

This reverts commit ed9a1fc.

# Conflicts:
#	eng/BannedSymbols.txt
#	src/BlazorWebView/src/SharedSource/BlazorWebViewServiceCollectionExtensions.cs

* Restore some changes (so hopefully it builds now?)

Diff is also smaller

---------

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2023
@samhouts samhouts added the fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 platform/android 🤖
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants