Xamarin.forms: [Core] Fix CreateContent usage on DataTemplatesSelector

[numb3r]

Description

-it makes sense to me to open DataTemplateExtension (change the namespace, remove the attribute)
-we should review our own parameterless CreateContent(), I'm thinking about LV.Header and Footer templates.

I agree with @StephaneDelcroix 's recommendations
CreateContent is the public api and it doesn't work so it needs to be fixed or made obsolete for a DataTemplateSelector and the only functional way to do it should be moved out of internals

Original Description

Using DataTemplate, when it tries to create content for ItemTemplate it throw Invalid Operation exception saying "LoadTemplate should not be null".

Version with issue Xamarin 4.10.10.2:

Platform Target Frameworks:
Android: Android SDK 8.3.3.2
Affected Devices: Samsung SM-T377P

Reproduction

DataTemplateDemo.zip

[samhouts]

Can you attach a small project that reproduces this issue? Which version of the Xamarin.Forms nuget package are you using? Thanks!

[GalaxiaGuy]

The message seems to be generated here:

https://github.com/xamarin/Xamarin.Forms/blob/d4480b2e48e4ec85bf0f20b19e3b7b9d6cf1744a/Xamarin.Forms.Core/ElementTemplate.cs#L77

I believe when I saw this problem a while ago, it was because the DataTemplate I was trying to use was really a DataTemplateSelector. If that is the case, then there is already an extension method that checks for this situation and does the right thing:

https://github.com/xamarin/Xamarin.Forms/blob/e3b6e4755eda0e7611694929d6b0529ee2c40d4b/Xamarin.Forms.Core/DataTemplateExtensions.cs#L17

To improve the situation, I think the two checks at the beginning of ElementTemplate.CreateContent should be the other way round (since in the case of a DataTemplateSelector, LoadContent is always null).

[StephaneDelcroix]

I really wonder what you guys are doing to end up with a null there. Please attach a repro so we can fix it.

[numb3r]

@samhouts @StephaneDelcroix
Here's a link to my repo.
Please uncomment section of code where i am using DataTemplate in view.
https://github.com/numb3r/Page-dot-indicator-repo

[StephaneDelcroix]

@samhouts it doesn't look like we have any Xaml test with DTS. We should confirm that it works (DTS + Xaml) with ListView, at the very least.
By looking at our code, I'm also concerned about having DTS (with or without Xaml) for LV header or footers.

[StephaneDelcroix]

@numb3r I went further with the investigation. CreateContent() will never work with a DataTemplateSelector. The correct call would be:

BindableObject bindableObject = (BindableObject) (content = (View) ItemTemplate.CreateContent(item, this));

but that require declaring

using Xamarin.Forms.Internals;

With that modification, your PageDotIndicatorSelector.OnSelectTemplate() is correctly invoked with the right arguments. Calling this.SelectTemplate(...) like you do is wrong and results in a stackoverflow (SelectTemplate calls OnSelectTemplate` already).

With that, you should be good to go.

I'm leaving this issue open for @samhouts to comment on the following points:

• it makes sense to me to open DataTemplateExtension (change the namespace, remove the attribute)
• we should review our own parameterless CreateContent(), I'm thinking about LV.Header and Footer templates.

[andreinitescu]

Following conversation from #4994:

Since the DataTemplateSelector derives from DataTemplate, the DataTemplateSelector should work when it's used as a DataTemplate (Liskov substition principle). Currently it breaks the principle by throwing exception when calling CreateContent() on it and it's creating confusion.

[andreinitescu]

What is the action plan on this? Any blocker to define the plan?

Because it inherits DataTemplate, every instance of DataTemplateSelector creates and holds Bindings and Values dictionaries but the DataTemplateSelector implementation doesn't use these dictionaries. Also, I wonder why is it necessary for DataTemplateSelector implementation to be coupled with ListView, see https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Core/DataTemplateSelector.cs#L14I know removing the inheritance probably is not possible at this moment, but I wonder if there aren't any other options.

I could help with this if there's was a clear plan...

[powerdude]

Just my $0.02, but I would say the team should do the right thing with this now and fix it. There is a major version in beta and this is exactly the type of breaking change to put in that version instead of waiting for a _better_ time. New controls and stuff are nice, but major versions should also be dedicated to fixing old patterns while embracing the new. We should continue to remove the technical debt that has grown in the library as this will help pave the way for future enhancements.

[andreinitescu]

For what is worth: https://github.com/andreinitescu/DataTemplateSelectorFix

[berlamont]

@andreinitescu , Did you forget to push a commit to that repo by chance?

[andreinitescu]

@berlamont not that I know of.

[andreinitescu]

If I could get some little input from the Xamarin Forms team I could work on this. I believe it has some impact on ListView (few tickets open), BindableLayout (decision on #4994 depends on this ticket), CollectionView but also my work on #4269 (see last comment from someone requesting item data template selector property)

[andreinitescu]

@berlamont Please check again, I forgot to push some commits indeed. Too much work, hehe