unoplatform / uno

Open-source platform for building cross-platform native Mobile, Web, Desktop and Embedded apps quickly. Create rich, C#/XAML, single-codebase apps from any IDE. Hot Reload included! 90m+ NuGet Downloads!!
https://platform.uno
Apache License 2.0
9.01k stars 733 forks source link

[anyplat] MarkupExtension::ProvideValue() is forced into .ToString() by xaml source-gen #14361

Closed Xiaoy312 closed 1 year ago

Xiaoy312 commented 1 year ago

Current behavior

This code below will throw on uno:

public class ResponsiveExtension : MarkupExtension
{
    protected override object? ProvideValue(IXamlServiceProvider serviceProvider) => Orientation.Horizontal;
}

<StackPanel Orientation="{local:Responsive}">
    <TextBlock Text="A" />
    <TextBlock Text="B" />
    <TextBlock Text="C" />
</StackPanel>

Expected behavior

^ should work like on windows.

How to reproduce it (as minimally and precisely as possible)

responsive_value.zip

Workaround

n/a

Works on UWP/WinUI

Yes

Environment

Uno.WinUI / Uno.WinUI.WebAssembly / Uno.WinUI.Skia

NuGet package version(s)

Uno.WinUI@5.0.31

Affected platforms

WebAssembly, Android, iOS, macOS (AppKit), Mac Catalyst, Skia (WPF), Skia (GTK on Linux/macOS/Windows), Skia (Linux Framebuffer), Build tasks

IDE

Visual Studio 2022

IDE version

No response

Relevant plugins

No response

Anything else we need to know?

generated code:

c6.Orientation = 
    (global::Microsoft.UI.Xaml.Controls.Orientation)Convert.ChangeType(
        (((global::Microsoft.UI.Xaml.Markup.IMarkupExtensionOverrides)new global::SampleMarkupExtensions.ResponsiveExtension())
            .ProvideValue(global::Uno.UI.Helpers.MarkupHelper.CreateParserContext(c6, typeof(global::Microsoft.UI.Xaml.Controls.StackPanel), "Orientation", typeof(object))))
            ?.ToString(),
        typeof(global::Microsoft.UI.Xaml.Controls.Orientation)
    );

https://github.com/unoplatform/uno/blob/5.0.19/src/SourceGenerators/Uno.UI.SourceGenerators/XamlGenerator/XamlFileGenerator.cs#L4680-L4683 ^ doesnt seem right.

Since we know on Windows that calling Convert.ChangeType("Horizontal", typeof(Orientation)) will throw. And obiviously, .ToString() and Convert.ChangeType(text, type) isn't a safe way to convert? We could be losing context or performance for complex type. The very least we should be check if the returned value is of the desired type before attempt to "convert":

boxed is T value ? value : Convert.ChangeType(boxed?.ToString(), typeof(T))
Youssef1313 commented 1 year ago

It seems we can just replace Convert.ChangeType with XamlBindingHelper.ConvertValue? We also shouldn't need to call ToString at all.