[Samples] ToolkitSampleMultiChoiceOption should take a converter method parameter/Action

[michael-hawker]

Problem Statement

@Arlodotexe I was just creating a sample for Expander and was using ToolkitSampleMultiChoiceOption for the expand direction of 'Down' or 'Up':

Labs-Windows/labs/Expander/samples/Expander.Sample/ExpanderFirstSamplePage.xaml.cs

Line 31 in 3b9d3c6

[ToolkitSampleMultiChoiceOption("ExpandDirection", title: "Expand Direction", "Down", "Up")]

However, these are strings... so I needed to add a long unwieldy converter that most folks wouldn't need to the sample:

Labs-Windows/labs/Expander/samples/Expander.Sample/ExpanderFirstSamplePage.xaml

Line 11 in 3b9d3c6

ExpandDirection="{x:Bind local:ExpanderFirstSamplePage.ConvertStringToDirection(ExpandDirection), Mode=OneWay}"

Labs-Windows/labs/Expander/samples/Expander.Sample/ExpanderFirstSamplePage.xaml.cs

Lines 40 to 45 in 3b9d3c6

	public static MUXC.ExpandDirection ConvertStringToDirection(string direction) => direction switch
	{
	"Down" => MUXC.ExpandDirection.Down,
	"Up" => MUXC.ExpandDirection.Up,
	_ => throw new System.NotImplementedException(),
	};

Suggested Solution

It'd be nice if instead, we could have this be part of the Labs sample source generation process here to simplify they desired output for these scenarios:

ExpandDirection="{x:Bind ExpandDirection, Mode=OneWay}" 

As in an app scenario, the developer is probably just using the enum type directly, this is a sample implementation detail in the current setup.

Ideally, we could specify the method/action for converting in the attribute (as an Action I guess or delegate, that should mean llambda or method are supported I think):

[ToolkitSampleMultiChoiceOption("ExpandDirection", title: "Expand Direction", converter:nameof(ConvertStringToDirection), "Down", "Up")] 
...

public static MUXC.ExpandDirection ConvertStringToDirection(string direction) => direction switch 
 { 
     "Down" => MUXC.ExpandDirection.Down, 
     "Up" => MUXC.ExpandDirection.Up, 
     _ => throw new System.NotImplementedException(), 
 }; 

(I'm not sure how the current implementation works, so this may require two properties linked together, one bound to the generated UI that uses the string/radio connection, and another one which listens to that, passes through the function, and is the one bound by the sample.)

But the idea is, this would generate the ExpandDirection with the return type of the converter function instead of string and then when the user selects the option and updates the value would pass it through the action specified first.

Also, if a method is tagged in the attribute this way, we should hide it in the C# code view for displaying the sample code.

Other Considerations

Or maybe we want it to be based off a bool for the sample:

ExpandDirection="{x:Bind local:ExpanderFirstSamplePage.ConvertBoolToDirection(IsExpanderDown), Mode=OneWay}" 

In this case the sample class would actually have two converters, one that converts our string to the bool being expected by the sample for IsExpanderDown, and then the one the sample is actually using to demonstrate mapping that to an enum.

[ToolkitSampleMultiChoiceOption("ExpandDirection", title: "Expand Direction", converter:nameof(ConvertStringToBool), "Down", "Up")] 
...

// hidden from C# view
public static bool ConvertStringToBool(string direction) => direction switch 
 { 
     "Down" => true
     "Up" => false, 
     _ => throw new System.NotImplementedException(), 
 }; 

// Still seen, as in use by example
public static MUXC.ExpandDirection ConvertBoolToDirection(bool direction) => direction switch 
 { 
     true => MUXC.ExpandDirection.Down, 
     false => MUXC.ExpandDirection.Up, 
     _ => throw new System.NotImplementedException(), 
 }; 

Other Notes

The Sample Author may want to just show a converter, and that should be fine. This is more for cases with enums or other complex types where you want a simple selection choice and a simple binding (as would appear in an app).

[michael-hawker]

Another pitfall I just encountered. @Arlodotexe we should have a generator analysis to make sure you don't pick a name for a common XAML enum like Orientation as then it generates a property on the class named Orientation and with global usings it gets confused with the enum for Orientation from WUX/MUX.

E.g. I need to do this:

[ToolkitSampleMultiChoiceOption("LayoutOrientation", title: "Orientation", "Horizontal", "Vertical")]

And not this:

[ToolkitSampleMultiChoiceOption("Orientation", title: "Orientation", "Horizontal", "Vertical")]

As this'll cause a cryptic error:

1>        H:\code\Labs-UWP\template\lab\samples\ProjectTemplate.Sample\ProjectTemplateCustomSample.xaml.cs(23,23,23,34): error CS0120: An object reference is required for the non-static field, method, or property 'ProjectTemplateCustomSample.Orientation'

Took me a minute to realize ProjectTemplateCustomSample.Orientation was because we're generating the new property named Orientation if I setup the option that way.

[michael-hawker]

Realized we probably need to use nameof to refer to a function call in the class, eh @Sergio0694? That could make the params at the end tricky, though maybe if we order it since we're using title: explicitly, then we can make it work the same as it does now?

[michael-hawker]

Was thinking, we could just accept an Enum type as well as a potential overload or separate attribute as well and use that to fill out all the options, right? (Also see #207)

[Arlodotexe]

Another pitfall I just encountered. @Arlodotexe we should have a generator analysis to make sure you don't pick a name for a common XAML enum like Orientation as then it generates a property on the class named Orientation and with global usings it gets confused with the enum for Orientation from WUX/MUX.

E.g. I need to do this:

[ToolkitSampleMultiChoiceOption("LayoutOrientation", title: "Orientation", "Horizontal", "Vertical")]

And not this:

[ToolkitSampleMultiChoiceOption("Orientation", title: "Orientation", "Horizontal", "Vertical")]

As this'll cause a cryptic error:

1>        H:\code\Labs-UWP\template\lab\samples\ProjectTemplate.Sample\ProjectTemplateCustomSample.xaml.cs(23,23,23,34): error CS0120: An object reference is required for the non-static field, method, or property 'ProjectTemplateCustomSample.Orientation'

Took me a minute to realize ProjectTemplateCustomSample.Orientation was because we're generating the new property named Orientation if I setup the option that way.

Curious if we could use the GeneratorDriver (used in our tests) inside of a running source generator to catch when the name conflicts with a using imported type, and change the error message.