[Hosting] Register ImageSource services as KeyedServices by mdh1418 · Pull Request #19641 · dotnet/maui

mdh1418 · 2024-01-01T15:18:42Z

Description of Change

This PR aims to overhaul the ImageSourceServiceProvider-based ImageSource Service registration setup in favor of leveraging Keyed Services introduced in .NET 8 to achieve a more trimmer friendly solution.

ImageSourceServiceProvider was initially introduced to be able to retrieve various derivations of ImageSourceService of which each would interact with its own derivation of an ImageSource (i.e. FileImageSourceService : ImageSourceService, IImageSourceService<IFileImageSource>). Thus, for the ImageSourceServiceProvider to grab these services, it would need to resolve an ImageSource to the corresponding IImagesourceService<> generic that the service had been registered with via trimming unfriendly System.Type.MakeGenericType

maui/src/Core/src/Hosting/ImageSources/ImageSourceServiceProvider.cs

Lines 25 to 59 in 61ca414

    
           public IImageSourceService? GetImageSourceService(Type imageSource) => 
        
           	(IImageSourceService?)GetService(GetImageSourceServiceType(imageSource)); 
        
           public Type GetImageSourceServiceType(Type imageSource) => 
        
           	_serviceCache.GetOrAdd(imageSource, type => 
        
           	{ 
        
           		var genericConcreteType = ImageSourceServiceType.MakeGenericType(type); 
        
           		if (genericConcreteType != null && GetServiceDescriptor(genericConcreteType) != null) 
        
           			return genericConcreteType; 
        
           		return ImageSourceServiceType.MakeGenericType(GetImageSourceType(type)); 
        
           	}); 
        
           public Type GetImageSourceType(Type imageSource) => 
        
           	_imageSourceCache.GetOrAdd(imageSource, CreateImageSourceTypeCacheEntry); 
        
           Type CreateImageSourceTypeCacheEntry(Type type) 
        
           { 
        
           	if (type.IsInterface) 
        
           	{ 
        
           		if (type.GetInterface(ImageSourceInterface) != null) 
        
           			return type; 
        
           	} 
        
           	else 
        
           	{ 
        
           		foreach (var directInterface in type.GetInterfaces()) 
        
           		{ 
        
           			if (directInterface.GetInterface(ImageSourceInterface) != null) 
        
           				return directInterface; 
        
           		} 
        
           	} 
        
           	throw new InvalidOperationException($"Unable to find the image source type because none of the interfaces on {type.Name} were derived from {nameof(IImageSource)}."); 
        
           }

.

Instead, leveraging Keyed Services to register and retrieve each ImageSourceService with a service key such as the underlying ImageSource type IFileImageSource, renders the logic to extrapolate an IImageSourceService<> generic from an ImageSource type unnecessary (one could grab the ImageSourceService via GetRequiredKeyedService<IImageSourceService>(typeof(IFileImageSource))). As a result, the ImageSourceServices can be registered directly through the MauiApp's ServiceCollection, making the ImageSourceServiceProvider class unnecessary.

This PR overhauls the ImageSourceServiceProvider-based ImageSource Service registration setup by:

Registering the various ImageSource Services directly through the MauiApp's ServiceCollection
Replacing ImageSourceServiceProvider API calls with the corresponding GetRequiredKeyedService/GetKeyedService calls
- Ensuring providers used to retrieve ImageSourceServices can be casted to IKeyedServiceProvider
  - Modifying MauiContext's underlying WrappedServiceProvider implements IKeyedServiceProvider
  - Modifying Public API signatures to use IKeyedServiceProvider instead of IServiceProvider
Removing ImageSourceServiceProvider all together
[WIP] Modifying tests to reflect the change in ImageSourceService registration

Issues Fixed

Fixes ImageSourceServiceProvider trimming warnings mentioned in #19397. Once this PR is merged, the number of warnings tracked by PublishNativeAOTCheckWarnings introduced in #19194 should decrease.

ghost · 2024-01-01T15:18:50Z

Hey there @mdh1418! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

simonrozsival · 2024-01-02T12:36:10Z

/azp run

azure-pipelines · 2024-01-02T12:36:32Z

Azure Pipelines successfully started running 3 pipeline(s).

rmarinho · 2024-01-02T15:41:00Z

Can we get a little more context about this on the PR description?

mdh1418 · 2024-01-02T15:45:40Z

@rmarinho Yes, sorry it is still a work in progress, and I haven't had time to fill out the description yet.

jonathanpeppers · 2024-01-03T14:41:14Z

Can you target the net9.0 branch instead? main is for .NET 8 servicing right now.

With keyed services introduced in net8.0, register the default IImageSourceServices as keyed services directly through the MauiApp's ServiceProvider's ServiceCollection in preparation to obviate ImageSourceServiceProvider logic.

…r interface

…legate

…ration

Tests leverage ContextStub instead of MauiContext. With ImageSourceServices registered as keyed services, ContextStub needs to implement IKeyedServiceProvider in order for tests to properly grab keyed image sources.

mdh1418 · 2024-01-08T16:40:55Z

/azp run

azure-pipelines · 2024-01-08T16:41:13Z

Azure Pipelines successfully started running 3 pipeline(s).

src/Compatibility/ControlGallery/src/Core/Startup.cs

jonathanpeppers · 2024-01-08T16:47:13Z

src/Controls/src/Core/Platform/Android/BottomNavigationViewUtils.cs

+			if (context.Services is not IKeyedServiceProvider provider)
+				throw new InvalidOperationException($"Context services {nameof(context.Services)} does not implement {nameof(IKeyedServiceProvider)}");


@PureWeen can/should we change context.Services to an IKeyedServiceProvider? (it extends IServiceProvider) Then we wouldn't need to cast/throw here?

https://github.com/dotnet/runtime/blob/0823c5c2814ffad5718f06715899ef91aa84aae2/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/IKeyedServiceProvider.cs#L12

The cast/throw occurs a couple places in this PR.

…ce registration

This reverts commit cb0c115.

This reverts commit f840ac3.

…figureDelegate" This reverts commit d3e0305.

…to keyed services

ghost added the community ✨ Community Contribution label Jan 1, 2024

mdh1418 changed the base branch from main to net9.0 January 2, 2024 21:20

mdh1418 changed the base branch from net9.0 to main January 2, 2024 21:20

jonathanpeppers added legacy-area-perf Startup / Runtime performance perf/app-size Application Size / Trimming (sub: perf) and removed community ✨ Community Contribution labels Jan 3, 2024

jonathanpeppers added this to the .NET 9 Planning milestone Jan 3, 2024

mdh1418 changed the base branch from main to net9.0 January 3, 2024 15:21

mdh1418 force-pushed the register_keyed_image_source_services branch from eb696b4 to d8e7c66 Compare January 3, 2024 15:21

mdh1418 added 15 commits January 5, 2024 18:12

[ImageSourceService] Alias ImageSource handlers to ImageSourceServices

fa5428f

[MauiContext] Extend WrappedServiceProvider with IKeyedServiceProvide…

87962fc

…r interface

[ElementHandler] Add GetKeyedServiceProvider extension method

c4e805d

Migrate UpdateFlyoutIconAsync to Keyed services

caf6590

Migrate UpdateBackgroundImageSourceAsync to Keyed Services

c0cfd4c

Migrate UpdateThumbImageSourceAsync to Keyed Services

32e873f

Migrate UpdateSourceAsync to KeyedServices

1934172

Migrate SetMenuItemIcon to Keyed Services

2fa2575

Migrate GetPlatformImageAsync to Keyed Services

b8b90ad

[ImageSourceService] Deprecate ConfigureImageSources with configureDe…

d3e0305

…legate

Remove remaining references to IImageSourceServiceProvider

f840ac3

Remove ImageSourceServiceProvider

cb0c115

[DeviceTests] Transition ConfigureTestBuilder to Keyed Service regist…

6af310a

…ration

[DeviceTests] Extend ContextStub to implement IKeyedServiceProvider

ca6ffd0

Tests leverage ContextStub instead of MauiContext. With ImageSourceServices registered as keyed services, ContextStub needs to implement IKeyedServiceProvider in order for tests to properly grab keyed image sources.

mdh1418 marked this pull request as ready for review January 8, 2024 16:41

mdh1418 requested a review from a team as a code owner January 8, 2024 16:41

mdh1418 requested review from PureWeen, jonathanpeppers, jsuarezruiz, mattleibow, rmarinho and simonrozsival and removed request for a team January 8, 2024 16:41

jonathanpeppers reviewed Jan 8, 2024

View reviewed changes

mdh1418 added 10 commits January 8, 2024 13:15

Fixup HostBuilderImageSourceTests

410d67e

Fixup ImageSourceServiceTests

5012d60

[DeviceTests] Transition Controls ConfigureTestBuilder to Keyed Servi…

c2e825b

…ce registration

[DeviceTests] Alias FileImageSource for Windows tests

7ac909e

Revert "Remove ImageSourceServiceProvider"

aeb9612

This reverts commit cb0c115.

Revert "Remove remaining references to IImageSourceServiceProvider"

f8402a8

This reverts commit f840ac3.

Revert "[ImageSourceService] Deprecate ConfigureImageSources with con…

485e00d

…figureDelegate" This reverts commit d3e0305.

[ImageSourceServices] Transition ConfigureImageSources with delegate …

142fd89

…to keyed services

Transition AddServices to keyed services registration

9a15a9f

Allow ConfigureImageSources calls to opt into keyed service transition

084192b

mdh1418 marked this pull request as draft January 16, 2024 16:00

simonrozsival mentioned this pull request Jan 22, 2024

[Trimming] Fix trimming warnings related to image source service provider #20058

Merged

rmarinho deleted the branch dotnet:net9.0 January 31, 2024 11:32

rmarinho closed this Jan 31, 2024

github-actions bot locked and limited conversation to collaborators Mar 2, 2024

Eilon added area-publishing Issues with the app packaging/publishing process (ipk/apk/msix/trimming) perf/general The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Hosting] Register ImageSource services as KeyedServices#19641

[Hosting] Register ImageSource services as KeyedServices#19641
mdh1418 wants to merge 33 commits intodotnet:net9.0from
mdh1418:register_keyed_image_source_services

mdh1418 commented Jan 1, 2024 •

edited

Loading

Uh oh!

ghost commented Jan 1, 2024

Uh oh!

simonrozsival commented Jan 2, 2024

Uh oh!

azure-pipelines bot commented Jan 2, 2024

Uh oh!

rmarinho commented Jan 2, 2024

Uh oh!

mdh1418 commented Jan 2, 2024

Uh oh!

jonathanpeppers commented Jan 3, 2024

Uh oh!

mdh1418 commented Jan 8, 2024

Uh oh!

azure-pipelines bot commented Jan 8, 2024

Uh oh!

Uh oh!

jonathanpeppers Jan 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	public IImageSourceService? GetImageSourceService(Type imageSource) =>
	(IImageSourceService?)GetService(GetImageSourceServiceType(imageSource));

	public Type GetImageSourceServiceType(Type imageSource) =>
	_serviceCache.GetOrAdd(imageSource, type =>
	{
	var genericConcreteType = ImageSourceServiceType.MakeGenericType(type);

	if (genericConcreteType != null && GetServiceDescriptor(genericConcreteType) != null)
	return genericConcreteType;

	return ImageSourceServiceType.MakeGenericType(GetImageSourceType(type));
	});

	public Type GetImageSourceType(Type imageSource) =>
	_imageSourceCache.GetOrAdd(imageSource, CreateImageSourceTypeCacheEntry);

	Type CreateImageSourceTypeCacheEntry(Type type)
	{
	if (type.IsInterface)
	{
	if (type.GetInterface(ImageSourceInterface) != null)
	return type;
	}
	else
	{
	foreach (var directInterface in type.GetInterfaces())
	{
	if (directInterface.GetInterface(ImageSourceInterface) != null)
	return directInterface;
	}
	}

	throw new InvalidOperationException($"Unable to find the image source type because none of the interfaces on {type.Name} were derived from {nameof(IImageSource)}.");
	}

		if (context.Services is not IKeyedServiceProvider provider)
		throw new InvalidOperationException($"Context services {nameof(context.Services)} does not implement {nameof(IKeyedServiceProvider)}");

Conversation

mdh1418 commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Issues Fixed

Uh oh!

ghost commented Jan 1, 2024

Uh oh!

simonrozsival commented Jan 2, 2024

Uh oh!

azure-pipelines bot commented Jan 2, 2024

Uh oh!

rmarinho commented Jan 2, 2024

Uh oh!

mdh1418 commented Jan 2, 2024

Uh oh!

jonathanpeppers commented Jan 3, 2024

Uh oh!

mdh1418 commented Jan 8, 2024

Uh oh!

azure-pipelines bot commented Jan 8, 2024

Uh oh!

Uh oh!

jonathanpeppers Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mdh1418 commented Jan 1, 2024 •

edited

Loading