[Bug] AppThemeBindingExtension code - ConvertTo will never be invoked

[tonyhallett]

https://github.com/xamarin/Xamarin.Forms/blob/caab66bcf9614aca0c0805d560a34e176d196e17/Xamarin.Forms.Xaml/MarkupExtensions/AppThemeBindingExtension.cs#L111

//.... when converterProvider is null
                       Exception converterException = null;

            if (converterException != null && _haslight)
                binding.Light = Light.ConvertTo(propertyType, minforetriever, serviceProvider, out converterException);
            if (converterException != null && _hasdark)
                binding.Dark = Dark.ConvertTo(propertyType, minforetriever, serviceProvider, out converterException);
            if (converterException != null && _hasdefault)
                binding.Default = Default.ConvertTo(propertyType, minforetriever, serviceProvider, out converterException);

            if (converterException != null)
                throw converterException;

!=null should be replaced with ==null

[jsuarezruiz]

Could you share a sample where reproduce the issue and validate it?

[tonyhallett]

@jsuarezruiz

An example really isn't necessary.

Exception converterException = null;

converterException is null

Given converterException is null how is binding.Light going to be set ? if (converterException != null && _haslight) binding.Light = Light.ConvertTo(propertyType, minforetriever, serviceProvider, out converterException);

and so on.

This is not something that can be easily demonstrated given that the markup extension is to be used in xaml. The IServiceProvider implementation is provided by the framework. To demonstrate, we need the service IValueConverterProvider to be null and we also need to understand what the ConvertTo object extension method does.

In the ( ridiculous ) example below I would expect that if !null == null then the AppThemeBindingExtension Default property value should be set on the AppThemeBinding ( ConvertTo itself ) and the label should have this colour, which of course it does not.

<?xml version="1.0" encoding="utf-8" ?>
<ContentPage xmlns="http://xamarin.com/schemas/2014/forms"
             xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
             x:Class="AppThemeBindingExtensionNonsensePage">
    <ContentPage.Content>
        <StackLayout x:Name="stackLayout">
            <Label Text="Has Green Colour as IServiceProvider provides IValueConverterProvider" TextColor="{AppThemeBinding Default=Green}"/>
        </StackLayout>
    </ContentPage.Content>
</ContentPage>

[XamlCompilation(XamlCompilationOptions.Compile)]
    public partial class AppThemeBindingExtensionNonsensePage : ContentPage,IServiceProvider, IProvideValueTarget, IMultiValueConverter
    {
        public object TargetObject => null;

        public object TargetProperty => BindableProperty.Create("PropertyName", typeof(Color), typeof(AppThemeBindingExtensionNonsensePage));

        public object GetService(Type serviceType)
        {
            if (serviceType == typeof(IProvideValueTarget))
            {
                return this;
            }
            return null;
        }

        public object Convert(object[] values, Type targetType, object parameter, CultureInfo culture)
        {
            var valueFromAppThemeBinding = values[0];
            if(valueFromAppThemeBinding == null)
            {
                return Color.Red;
            }
            throw new Exception("Not hit as AppThemeBinding has not had Light, Dark or Default set");
        }

        public object[] ConvertBack(object value, Type[] targetTypes, object parameter, CultureInfo culture)
        {
            throw new NotImplementedException();
        }

        public AppThemeBindingExtensionNonsensePage()
        {
            InitializeComponent();

            var badMarkupExtension = new AppThemeBindingExtension();
            badMarkupExtension.Default = Color.Green;
            var appThemeBinding = (BindingBase)badMarkupExtension.ProvideValue(this);

            var badLabel = new Label { Text = "Should be green" };
            badLabel.SetBinding(Label.TextColorProperty, appThemeBinding);

            stackLayout.Children.Add(badLabel);

            var multiBinding = new MultiBinding { Bindings = new List<BindingBase> { appThemeBinding}, Converter = this };

            var badLabelWithConverter = new Label { Text = "Should be green" };
            badLabelWithConverter.SetBinding(Label.TextColorProperty, multiBinding);
            stackLayout.Children.Add(badLabelWithConverter);

        }
    }

[tonyhallett]

I would suggest a test be added to https://github.com/xamarin/Xamarin.Forms/blob/5.0.0/Xamarin.Forms.Xaml.UnitTests/OnAppThemeTests.cs

that uses the MarkupExtensionParser and a mock IServiceProvider to test the ConvertTo paths - see https://github.com/xamarin/Xamarin.Forms/blob/caab66bcf9614aca0c0805d560a34e176d196e17/Xamarin.Forms.Xaml.UnitTests/MarkupExtensionTests.cs#L110 for how to proceed.

@StephaneDelcroix perhaps you can fix this - https://github.com/xamarin/Xamarin.Forms/commits/01c71080ae8356275a3570383bf3636d689d4155/Xamarin.Forms.Xaml/MarkupExtensions/AppThemeBindingExtension.cs

Final point. Unless something is setting the Value property with reflection the value will always be null.

[tonyhallett]

@jsuarezruiz

See the difference ?https://github.com/xamarin/Xamarin.Forms/blob/29380ad055851fcd3d889c71d9e32ff1935c6746/Xamarin.Forms.Xaml/MarkupExtensions/AppThemeBindingExtension.cs#L82