-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Added Shell Page dependency resolution with DataTemplate and Shell global routes… #3375
Conversation
LoadTemplate = () => Activator.CreateInstance(type); | ||
LoadTemplate = () => | ||
{ | ||
if (Application.Current?.Handler?.MauiContext?.Services != null) |
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.
Services need to be retrieved from the context the element is being used in. The services all get scoped differently based on the window/modal they are apart of. This it the part I'm hoping doesn't become too tricky. In theory you can find the correct services via the parent
_parent.FindMauiContext().Services
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.
Yeah, I'm starting to bump into the trickiness here. The ShellContent's ContentTemplate's _parent is null when LoadTemplate() is being called. The ShellContent.Parent does exist though, maybe I can either try to set ContentTemplate.Parent with "this" (ShellContent) when GetOrCreateContent() is being called, or something to that effect? Or is that a bad idea?
This is an example of where, when I put something like (ContentTemplate as IElement).Parent = this;
, if I happen to be using a DataTemplate
inside of a ShellContent
, it will then find the services and LoadTemplate
works.
maui/src/Controls/src/Core/Shell/ShellContent.cs
Lines 53 to 58 in 25c7c2c
Page IShellContentController.GetOrCreateContent() | |
{ | |
var template = ContentTemplate; | |
var content = Content; | |
Page result = null; |
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.
@PureWeen does this seem too hacky to you?
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.
Looking at ElementTemplate
a bit closer I think that it was incorrectly made to implement IElement
eight or so years ago when that structure was first created. AFAICT the parent is never set on ElementTemplate
anywhere ever. Can we utilize the fact that the setter for ElementTemplate.LoadTemplate
is public? Then inside ShellContent we just set our own LoadTemplate
? I think I'm going to walk back my statement about this being a DataTemplate
enhancement :-) I think the use of a ServiceProvider
with a DataTemplate
should probably be isolated to Shell
. The only other cases where we use DataTemplates
are with ListView/CollectionView
which don't really seem to fit the bill here and we probably don't want to add the additional overhead to those creators.
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.
@PureWeen I don't think it's that simple, because of how DataTemplate
in the XAML gets instantiated, in DataTemplateExtension
. The constructor takes a Type
which gets used by LoadTemplate
. We'd have to somehow get the type into whatever we set for Loadtemplate
in ShellContent
.
maui/src/Controls/src/Xaml/MarkupExtensions/DataTemplateExtension.cs
Lines 22 to 25 in 18e0f4e
if (typeResolver.TryResolve(TypeName, out var type)) | |
return new DataTemplate(type); | |
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.
Hunh, that feels like a sort of useless Func :-/ It just uses a closure inside the ctor
👎 What if you just store the type locally and expose it out using an internal? does that give you what you need?
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.
@PureWeen Yes, this seems to work.
Like you say, this is limited to DataTemplate
in ShellContent
, not working with DataTemplate
in general.
src/Controls/src/Core/Routing.cs
Outdated
@@ -143,7 +143,16 @@ public static Element GetOrCreateContent(string route) | |||
// okay maybe its a type, we'll try that just to be nice to the user | |||
var type = Type.GetType(route); | |||
if (type != null) | |||
result = Activator.CreateInstance(type) as Element; | |||
{ | |||
if (Application.Current?.Handler?.MauiContext?.Services != null) |
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.
Same comment here. We'll probably need to add a parameter to this method that takes in an IMauiContext and propagate that into this call from the caller
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.
@PureWeen for global routes, do we need to pass in a context via GoToAsync() then, from where that's being called?
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! you should be able to get the services via
_shell.FindMauiContext().Services
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.
This looks great! I have one suggestion, but that doesn't invalidate this amazing work <3
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
This change has broken a lot of my pages as DI is being used to create the instance of the view and it automatically chooses the constructor with max number of arguments. This change assumes that DI should be used to create the instance of the view. Some assumption given I never registered my views to the service provider, so I would not expect it to be resolved this way, and more to the point I don't want my view resolved this way and it would seem there is no way to opt out of this, which now causes me a headache. Why can't the internal implementation attempt to resolve via the service provider i.e. sp.GetService(type) and if its null revert to using Activator.CreateInstance this would ensure all existing code just works! If I want to opt into DI for view resolution, simple I just register the view, just like I would register everything. I get why developers would want DI resolution of the view and there are benefits, but if its only to inject a view model to set the BindingContext personally I prefer my approach, which is and attachable property that resolves the type from the service provider and sets the BindingContext . This way you are not restricted to the router for DI resolution it can be applied on any BindableObject and better still in Xaml, no code behind, changing constructors etc etc etc. And this is why I don't need or want DI resolution of my views unless I specifically register them. Can you advise is there any way to bypass this feature? |
I agree with PedroCigaw. I like the benefit of constructor injection for view models. All my views have a single viewmodel. Why should I need to register it in a container ? Instead I prefer to have it set by the view itself. So it’s usage is near its declaration. This feature would be much more useful for constructing view models though. but I don’t see how to use it for that ? (without having to register the viewmodel in the container) |
The relevant githug issues: dotnet/maui#3698 dotnet/maui#4072 dotnet/maui#3147 dotnet/maui#3375
…; added unit tests
@PureWeen I've written a couple of unit tests for this, and the device tests worked on Android. I can't get the device tests app (or anything else for that matter, including the Single Project sample app) to run on iOS.
Description of Change
Implements #3147
Additions made
PR Checklist
Does this PR touch anything that might affect accessibility?