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

[listview] fixes for various null/empty DataTemplate #13146

Merged
merged 2 commits into from
Feb 7, 2023

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Feb 6, 2023

Description of Change

Fixes #11203

The following breaks XAML Hot Reload:

  1. Setup a working ListView and DataTemplate

  2. Delete the DataTemplate's body (as if I'm about to type a completely new view there).

  3. Trigger a XAML Hot Reload. This can happen automatically if you just pause typing.

  4. MAUI crashes at runtime:

in Android.Views.ViewGroup.Layout at /Users/runner/work/1/s/xamarin-android/src/Mono.Android/obj/Release/net7.0/android-33/mcw/Android.Views.ViewGroup.cs:3369,5
in Microsoft.Maui.Controls.Handlers.Compatibility.VisualElementRenderer<Microsoft.Maui.Controls.ListView>.OnLayout at D:\a\_work\1\s\src\Controls\src\Core\Compatibility\Handlers\Android\VisualElementRenderer.cs:54,6
in Microsoft.Maui.Controls.Handlers.Compatibility.ListViewRenderer.OnLayout at D:\a\_work\1\s\src\Controls\src\Core\Compatibility\Handlers\ListView\Android\ListViewRenderer.cs:298,4

You can also create this problem in C#, like I did in a unit test:

listView.ItemTemplate = new DataTemplate(() => /* valid template */);
listView.ItemTemplate = new DataTemplate(); // broken
listView.ItemTemplate = new DataTemplate(() => null); //broken

I could also get a slightly different crash:

System.InvalidCastException: Specified cast is not valid.
at Microsoft.Maui.Controls.Internals.TemplatedItemsList`2[[Microsoft.Maui.Controls.ItemsView`1[[Microsoft.Maui.Controls.Cell, Microsoft.Maui.Controls, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]], Microsoft.Maui.Controls, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null],[Microsoft.Maui.Controls.Cell, Microsoft.Maui.Controls, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].ActivateContent(Int32 index, Object item)

The "invalid cast" appears to be due to the changes in c57858f. It returns a new Label() in some cases, which does not work with a ListView. You need a Cell instead of View in that case.

The fixes appear to be in two places:

  • VisualElementRenderer.OnLayout: use is, also simplifies the code

  • TemplatedItemsList.ActivateContent : use is and call the default data template.

Now my tests pass, and I can't reproduce the issue in XAML Hot Reload either!

Manually testing, I'm able to alternative commenting out the two XAML elements:

image

image

This didn't work before.

Issues Fixed

Fixes: #11203

Fixes: dotnet#11203

The following breaks XAML Hot Reload:

1. Setup a working `ListView` and `DataTemplate`

2. Delete the `DataTemplate`'s body (as if I'm about to type a
   completely new view there).

3. Trigger a XAML Hot Reload. This can happen automatically if you
   just pause typing.

4. MAUI crashes at runtime:

    in Android.Views.ViewGroup.Layout at /Users/runner/work/1/s/xamarin-android/src/Mono.Android/obj/Release/net7.0/android-33/mcw/Android.Views.ViewGroup.cs:3369,5
    in Microsoft.Maui.Controls.Handlers.Compatibility.VisualElementRenderer<Microsoft.Maui.Controls.ListView>.OnLayout at D:\a\_work\1\s\src\Controls\src\Core\Compatibility\Handlers\Android\VisualElementRenderer.cs:54,6
    in Microsoft.Maui.Controls.Handlers.Compatibility.ListViewRenderer.OnLayout at D:\a\_work\1\s\src\Controls\src\Core\Compatibility\Handlers\ListView\Android\ListViewRenderer.cs:298,4

You can also create this problem in C#, like I did in a unit test:

    listView.ItemTemplate = new DataTemplate(() => /* valid template */);
    listView.ItemTemplate = new DataTemplate(); // broken
    listView.ItemTemplate = new DataTemplate(() => null); //broken

I could also get a slightly different crash:

    System.InvalidCastException: Specified cast is not valid.
    at Microsoft.Maui.Controls.Internals.TemplatedItemsList`2[[Microsoft.Maui.Controls.ItemsView`1[[Microsoft.Maui.Controls.Cell, Microsoft.Maui.Controls, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]], Microsoft.Maui.Controls, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null],[Microsoft.Maui.Controls.Cell, Microsoft.Maui.Controls, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].ActivateContent(Int32 index, Object item)

The "invalid cast" appears to be due to the changes in c57858f. It
returns a `new Label()` in some cases, which does not work with a
`ListView`. You need a `Cell` instead of `View` in that case.

The fixes appear to be in two places:

* `VisualElementRenderer.OnLayout`: use `is`, also simplifies the code

* `TemplatedItemsList.ActivateContent` : use `is` and call the default
  data template.

Now my tests pass, and I can't reproduce the issue in XAML Hot Reload
either!
@Eilon Eilon added the area-controls-listview ListView and TableView label Feb 6, 2023
Copy link
Member

@mandel-macaque mandel-macaque left a comment

Choose a reason for hiding this comment

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

Looks good, maybe invert the if (I'm not a UI developer)

src/Controls/src/Core/TemplatedItemsList.cs Outdated Show resolved Hide resolved
Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

Failing test on iOS

NavigatingBackViaBackButtonFiresNavigatedEvent

@jonathanpeppers
Copy link
Member Author

@rmarinho what test is this?

I can't even find it in the repo:

> git grep NavigatingBackViaBackButtonFiresNavigatedEvent
# nothing

I don't see how the latest commit broke it c1133aa, as it should produce identical C# code.

It was green before, so is this broken by a change in main?

@PureWeen
Copy link
Member

PureWeen commented Feb 7, 2023

@rmarinho what test is this?

I can't even find it in the repo:

> git grep NavigatingBackViaBackButtonFiresNavigatedEvent
# nothing

I don't see how the latest commit broke it c1133aa, as it should produce identical C# code.

It was green before, so is this broken by a change in main?

@jonathanpeppers CI merges main before running tests. It looks like main broke with one of the merges from yesterday. Taking a look now

@jonathanpeppers
Copy link
Member Author

@rmarinho so maybe we can merge this one? It seems like it should be an improvement, adding is checks.

@mandel-macaque
Copy link
Member

@rmarinho @PureWeen there is a way (I made it work) to ensure that vsts use the commit of the PR and not the merge with main. It has its pros an cons. But you can do that and then make the merge block if it is not updated with main

@rmarinho rmarinho merged commit 9ab7067 into dotnet:main Feb 7, 2023
@jonathanpeppers jonathanpeppers deleted the ListViewEmptyDataTemplate branch February 7, 2023 19:32
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-listview ListView and TableView fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing <DataTemplate/> contents can give a runtime exception
7 participants