Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Fix 15052 NullReferenceException when returning null from DataTemplateSelector #15074

Closed
wants to merge 20 commits into from

Conversation

bakerhillpins
Copy link

@bakerhillpins bakerhillpins commented Jan 19, 2022

Description of Change

Allows DataTemplateSelector implementations to return null from DataTemplateSelector.OnSelectTemplate(object item, BindableObject container). This will result in the display of the item/element as if the related DataTemplate were not assigned at all/is null.

Issues Resolved

API Changes

None

Platforms Affected

  • Core/XAML (all platforms)
  • iOS
  • Android
  • UWP

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

I've attempted to add some simple tests to the Core.UnitTests project.

I've also been executing various xamarin_forms_samples projects where I've altered the code so that DataTemplateSelector implementations return null. I'll add those projects as I clean them up.

NOTE: I've needed to run with Microsoft.NETCore.UniversalWindowsPlatform 6.2.13 on my UWP test apps or I'll get ExecutionEngineExceptions when null is returned from the selectors. Once I upgrade that NuGet my changes seem to correct the remaining problems.

Test Projects: (NOTE: these currently reference released Xamarin.Forms and as such will fail)
Selector.zip
FlyoutTest.zip
groupingSampleListView.zip
CollectionViewDemos.zip
TabbedPageDemo.zip

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@bakerhillpins bakerhillpins changed the title Fix 15052 Fix 15052 NullReferenceException when returning null from DataTemplateSelector Jan 19, 2022
@jfversluis
Copy link
Member

Awesome stuff @bakerhillpins thanks so much for your help here! Feel free to ping me when you think this is good to go or you need anything :)

@bakerhillpins
Copy link
Author

bakerhillpins commented Jan 19, 2022

@jfversluis LOL... you asked for it. Might not be the right place but I can't continue testing/fixing without getting this to work and I've spent a bunch of time on it already:

I can't get my local xamarin.forms build (windows machine) of the UAP dll to build/execute in a test project. Android/iOS work fine. When I use a released version of Xamarin.Forms (5.0.0.2291) my UWP project runs with no problems.

When I update to my locally built NuGet I get the following Warning:
warning MSB3277: There was a conflict between "Microsoft.UI.Xaml.Markup, Version=0.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, ContentType=WindowsRuntime" and "Microsoft.UI.Xaml.Markup, Version=10.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35".

Which follows up with a bunch of these errors:

error MSB3030: Could not copy the file "C:\Users\bryan.roth\.nuget\packages\xamarin.forms\9.9.2\lib\uap10.0.16299\Xamarin.Forms.Platform.UAP\FormsAutoSuggestBoxStyle.xbf" because it was not found.

After a bit of poking around it seems that my local build has a dependency built into it that the production Xamarin.Forms.Platform.UAP.dll does not:

Local:
image

Release (5.0.0.2291):
image

Which is in the project but I can't seem to figure out where it comes from???

image

UPDATE - wish I would have stumbled upon this before I pinged you but I seem to have found a way around it...

Short story:

  • Build the solution with VS2022 and I get that extra reference in the assembly.
  • Build using the powershell script and there is no extra reference...

For those of you that arrive here via search... If you add the following two properties to the Xamarin.Forms.Platform.UAP project you can build with VS.. These properties were extracted from the build.cake file BuildForNuget task.

    <DisableEmbeddedXbf>false</DisableEmbeddedXbf>
    <EnableTypeInfoReflection>false</EnableTypeInfoReflection>

@jfversluis
Copy link
Member

Haha glad you figured it out! :D

@bakerhillpins
Copy link
Author

bakerhillpins commented Jan 20, 2022

UPDATE: I'm not sure what I was doing when I was seeing this problem but it is an issue ONLY on UWP.

@jfversluis I'm testing my changes and have a question for you about ItemsView.EmptyViewTemplate. It's not working as described here. The code that's detailed in that section is present in the Xamarin_Forms_Samples CollectionViewDemos project in this file. However, when you search the DataTemplateSelector.OnSelectTemplate is never called.

Search isn't coming up with a bug. Threw me for a loop when I was testing since the DataTemplate wasn't working and it's in the default samples.

So, can you tell me if the documentation details the correct expected behavior? UPDATE: It is Correct.

The code that's responsible for the application of the EmptyViewTemplate for UAP is here. And it's coded to completely ignore the DataTemplate unless the EmptyView is of a type other than string or View.

Other platforms operate the same so I'm unsure if this is expected or not..
[I guess it doesn't really matter, it's not a bug I caused]

@bakerhillpins
Copy link
Author

bakerhillpins commented Jan 21, 2022

@jfversluis Sorry for the earlier distractions.

I'm at the point where I think I've got it all completed. Few things:

  1. I don't really know how to test the Tizen code. Never even looked at that stuff before.
  2. I've tried all the collection based code I can find. See attached test projects. I'm reviewing the the remainder of the system to see what else there is but I'm really just searching. Do you know of any other support for DataTemplateSelectors outside of collection based controls?
  3. I could add Issue tests to the Xamarin source but I have no idea how to validate they work (build/run these tests). Suggestions or should we leave it be?

See below, I updated the default template for the empty page
4. WRT MultiPage default content. It's just a Page/ContentPage with the title set. When this renders it looks empty. If we databound a simple view it would at least print out the Type.Name on the screen.

After further review, this appears to be a UWP only issue.
5. WRT UWP - the default template is rather simplistic and has results in the Header View being clipped a bit with CollectionViews. I didn't really concern myself with this since it's basically operating on no definition from the user rather than crashing. Do we need to adjust this?
image

At the moment that's it. Any comments? Should I move this out of draft?

@bakerhillpins
Copy link
Author

bakerhillpins commented Jan 24, 2022

@jfversluis - I think It's time to get this into review. What do you say?

4. WRT MultiPage default content. It's just a Page/ContentPage with the title set. When this renders it looks empty. If we databound a simple view it would at least print out the Type.Name on the screen.

Ok, For the above I changed the default template in these cases to carry simple label content bound to the object to get something visual on the page (object.ToString). This way when folks drop a template something's displayed rather than just blank.

At the moment I'm unable to resolve what's going on in this method: OnViewTemplateChanged. There are several things I can't make sense out of:

  1. The view data currentViewData is being cast to a View? I'd expect this to be Item/context data not a View object.
  2. The CreateContent extension method is being called with newViewTemplate as the item parameter rather than the currentViewData object?
  3. If you search for where this method is called, it's actually called from the BindableProperty's OnDataChanging method? Yet this method is named OnChanged.

I think this is what the method really should be:

{
	View newContentView  = null;

	DataTemplate template = newViewTemplate?.SelectDataTemplate(currentViewData, shell);

	if (template != null)
	{
		newContentView = (View)template.CreateContent();
	}

	SetView(ref localViewRef,
		newContentView,
		OnChildRemoved,
		OnChildAdded);
}

@bakerhillpins
Copy link
Author

Bindable Layout EmptyDataTemplate fixed but found #15093

@jfversluis - I've not heard from you with regard to my questions so I'm just going to put this into review.

I've corrected the logic so it can handle a null from a DataTemplateSelector, but I've not corrected the logic that's using the template as a parameter for template selection.
@bakerhillpins bakerhillpins marked this pull request as ready for review January 26, 2022 13:03
@bakerhillpins
Copy link
Author

I'd like to understand how to validate platform tests before I commit them.

@jfversluis
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jfversluis
Copy link
Member

I'd like to understand how to validate platform tests before I commit them.

Not sure what you mean by this? I ran the build and all the unit tests seem green? :)

@bakerhillpins
Copy link
Author

I'd like to understand how to validate platform tests before I commit them.

Not sure what you mean by this? I ran the build and all the unit tests seem green? :)

@jfversluis Sorry for the confusion. I did put in some Xamarin.Forms.Core.UnitTests which can be run on windows.

I also authored one or two tests in the Platform.UnitTests space (e.g. Xamarin.Forms.Platform.Android.UnitTests) since I figured you'd prefer to have some included. Just a few to test the process, but I didn't commit them because I don't have instructions on how to execute the Platform based tests => The runner fails because I don't have Mono Android. This isn't surprising as I'm on a windows box but I wasn't sure if there was a way to do this besides checking them in and hoping that the Azure Pipelines don't fail.

@jfversluis
Copy link
Member

I see the new and old tests popping up here and succeeding: https://dev.azure.com/xamarin/public/_build/results?buildId=53229&view=ms.vss-test-web.build-test-results-tab

@bakerhillpins
Copy link
Author

I'm sorry, I'm not doing a good job of explaining myself I guess.

Is there any way to run Platform tests locally on my laptop?

@jfversluis
Copy link
Member

Ah sorry about that! :)

I think they are being ran as part of the UI tests. In the UITest project for Android you can see the PlatformUnitTest project is referenced. You can run them locally, but it might be easier to just commit them and see what it does in the pipeline :)

@jfversluis
Copy link
Member

Now that we're so close to the sunsetting of Xamarin.Forms unfortunately we won't be able to take this in anymore, we're really sorry about that. Nevertheless, thank you so much for your time and effort that you have put into this PR.

Please have a look at the evolution of Xamarin.Forms, .NET MAUI. A lot of development has been going on there. Hopefully this issue was already fixed in that codebase. If not, feel free to port this over to there.

Again, thank you so much for being a contributor and Xamarin.Forms user!

@jfversluis jfversluis closed this Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] NullReferenceException when returning null from DataTemplateSelector.OnSelectTemplate
2 participants