xamarin / Xamarin.Forms

Xamarin.Forms is no longer supported. Migrate your apps to .NET MAUI.
https://aka.ms/xamarin-upgrade
Other
5.62k stars 1.87k forks source link

[Bug] Converter is called 2 times more than the number of items on the list #12183

Closed TheBlueSky closed 4 years ago

TheBlueSky commented 4 years ago

Description

Converter is called 2 times more than the number of items on the list.

Steps to Reproduce

  1. Create a new Xamarin Form project
  2. Create a Converted like StringCaseConverter below
  3. Place a breakpoint on var stringValue = value?.ToString() ?? string.Empty;
  4. Register the Converter
  5. Replace the code in MainPage.xaml with the code below
  6. Start debugging the application

StringCaseConverter.cs

internal sealed class StringCaseConverter : IValueConverter
{
    public bool IsUpperCase { private get; set; }

    public object Convert(object value, Type targetType, object parameter, CultureInfo language)
    {
        var stringValue = value?.ToString() ?? string.Empty;

        return this.IsUpperCase ? stringValue.ToUpperInvariant() : stringValue.ToLowerInvariant();
    }

    public object ConvertBack(object value, Type targetType, object parameter, CultureInfo language)
    {
        return value;
    }
}

App.xaml

<Application.Resources>
    <ResourceDictionary>
        <converters:StringCaseConverter x:Key="StringToLowerCaseConverter" IsUpperCase="False"/>
        <converters:StringCaseConverter x:Key="StringToUpperCaseConverter" IsUpperCase="True"/>
    </ResourceDictionary>
</Application.Resources>

MainPage.xaml

<?xml version="1.0" encoding="utf-8" ?>
<ContentPage
    x:Class="App1.MainPage"
    xmlns="http://xamarin.com/schemas/2014/forms"
    xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml">

    <StackLayout Orientation="Horizontal" HorizontalOptions="Center">
        <CollectionView HorizontalOptions="Center">
            <CollectionView.ItemsSource>
                <x:Array Type="{x:Type x:String}">
                    <x:String>A</x:String>
                    <x:String>B</x:String>
                    <x:String>C</x:String>
                    <x:String>D</x:String>
                    <x:String>E</x:String>
                    <x:String>F</x:String>
                    <x:String>G</x:String>
                    <x:String>H</x:String>
                </x:Array>
            </CollectionView.ItemsSource>

            <CollectionView.ItemsLayout>
                <LinearItemsLayout Orientation="Horizontal" ItemSpacing="20"/>
            </CollectionView.ItemsLayout>

            <CollectionView.ItemTemplate>
                <DataTemplate>
                    <StackLayout Orientation="Horizontal" HorizontalOptions="End" VerticalOptions="Center">
                        <Label Text="{Binding ., Converter={StaticResource StringToLowerCaseConverter}}" HorizontalOptions="Center"/>
                    </StackLayout>
                </DataTemplate>
            </CollectionView.ItemTemplate>
        </CollectionView>
    </StackLayout>
</ContentPage>

Expected Behavior

The breakpoint is hit 8 times.

Actual Behavior

The breakpoint is hit 24 times.

Basic Information

Screenshots

N/A

Reproduction Link

See above.

Workaround

None that I know of.

StephaneDelcroix commented 4 years ago

This is normal.

1/ the DataTemplate is inflated, so the view (stack layout) is created, and the bindings are evaluated 2/ the StackLayout is parented, and as such, the bindingContext (potentially) changes, so bindings are applied 3/ the context is set (to the letter a to h), so bindings are once again evaluated.

your code shouldn't rely on the number of times a converter is invoked...

We could enhance this a little bit, by swapping steps 2 and 3 (bindings won't be reapplied when parented) (/cc @hartez is this possible), but we would keep the double evaluation, like all bindings created by XAML nowadays.

There's an enhancement for dotnetMaui to change the way contexts are resolved that could make this a one-stepper, but nothing we can change right now.

TheBlueSky commented 4 years ago

This is normal.

@StephaneDelcroix, I do not think this is entirely accurate though.

Let us make a tiny modification to the code I have in my bug reports. Let us change {Binding ., Converter={StaticResource StringToLowerCaseConverter}} to {Binding Length, Converter={StaticResource StringToLowerCaseConverter}}; in other words, instead of binding to the item itself, we bind to a property of an item, Length in this case.

Now when I debug the code I have the StringCaseConverter.Convet() called exactly 8 times, as expected.

StephaneDelcroix commented 4 years ago

I do not think this is entirely accurate though.

It is still accurate. when specifying a Path, the Binding is still evaluated as often as mentioned earlier, but as the Path can't be resolved (BindingContext is null) the converter isn't evaluated.

My advice again: make sure your converters are cheap to execute, and with no side effects.

TheBlueSky commented 4 years ago

My converter is cheap and has no side effect. I was mainly wondering about the behaviour that does not seem intuitive at first, especially coming from a previous experience with WPF, Windows Phone, and UWP.

I still think this can be enhanced so that the code behaviour is as it looks, but thank a lot for clarifying it.

Tommigun1980 commented 4 years ago

I've also seen my converters being invoked a lot more than I would have assumed. If there's any optimisations that can be done in regard to the number of times bindings are evaluated I think that'd be a big performance win, especially if WPF works differently. Thanks.