[controls] Brush.Foo should return immutable instances

[jonathanpeppers]

Description of Change

When profiling a dotnet new maui app, with this package:

https://github.com/jonathanpeppers/Mono.Profiler.Android

The alloc report shows:

Allocation summary
Bytes      Count  Average Type name
39984        147 2 72     Microsoft.Maui.Controls.SolidColorBrush

Stack trace:

38352 bytes from:
(wrapper runtime-invoke) object:runtime_invoke_void (object,intptr,intptr,intptr)
Microsoft.Maui.Controls.VisualElement:.cctor ()
(wrapper runtime-invoke) object:runtime_invoke_void (object,intptr,intptr,intptr)
Microsoft.Maui.Controls.Brush:.cctor ()

Reviewing the Brush class, there are indeed 147 SolidColorBrush
created on startup that are stored in fields.

But what is weird about this, is that SolidColorBrush is mutable!

public Color Color
{
    get => (Color)GetValue(ColorProperty);
    set => SetValue(ColorProperty, value);
}

So I could literally write code like:

Brush.Blue.Color = Colors.Red;

Blue is red! (insert evil laughter?)

I think the appropriate fix here is that all of these static readonly fields should just be properties that return a new
ImmutableBrush. We can cache the values in fields on demand. Then
someone can't do something evil like change Blue to Red?

I reviewed WPF source code to check what they do, and they took a
similar approach:

https://github.com/dotnet/wpf/blob/5e8187344b2b561ef08b9ca2735cd89cbdd3c11e/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/brushes.cs#L33-L1586

We should make this API change now before MAUI is stable, and we have
the side benefit to save 39984 bytes of memory on startup?

I added tests for these scenarios, and discovered 3 typos for Brush
colors that listed the wrong color.

Additions made

• static readonly fields in Brush are now properties that return a new instance.

PR Checklist

• Targets the correct branch
• Tests are passing (or failures are unrelated)
• Targets a single property for a single control (or intertwined few properties)
• Adds the property to the appropriate interface
• Avoids any changes not essential to the handler property
• Adds the mapping to the PropertyMapper in the handler
• Adds the mapping method to the WinUI, Android, iOS, and Standard aspects of the handler
• Implements the actual property updates (usually in extension methods in the Platform section of Core)
• Tags ported renderer methods with [PortHandler]
• Adds an example of the property to the sample project (MainPage)
• Adds the property to the stub class
• Implements basic property tests in DeviceTests

Does this PR touch anything that might affect accessibility?

No

[PureWeen]

@jonathanpeppers is there any value here with storing these on access into a local Dictionary or variable so they don't get allocated on every call? I realize there's the issue of the mutability but in theory we could account for that by subclassing the brush and throwing if you try to change the color.

Just thinking if we can get best of both worlds

[jonathanpeppers]

@PureWeen we could subclass and make the Color property setter throw.

But you could still do:

Brush.Blue.SetValue(SolidColorBrush.ColorProperty, Color.Red);

Is there a way to "extend" a BindableProperty and make it readonly?

WPF calls DependencyObject.Freeze() on the cached value they have:

https://github.com/dotnet/wpf/blob/5e8187344b2b561ef08b9ca2735cd89cbdd3c11e/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/Knowncolors.cs#L212-L220

I don't there there is a concept of "freeze" in MAUI?

[PureWeen]

@jonathanpeppers we have readonly keys you can use

maui/src/Controls/src/Core/CompressedLayout.cs

Line 28 in b1866fd

static readonly BindablePropertyKey HeadlessOffsetPropertyKey =

but not sure if we can make that work in this scenario.

My initial "semi-ok" approach here would be to do it via an OnPropertyChanged handler on the bindable property. Have that call into the concrete implementation and then you could override that to throw on the ImmutableBrush

[jonathanpeppers]

If I see typos like this, it's probably not on purpose?

[jonathanpeppers]

I found 3 like this after adding a test to check for it.

[PureWeen]

yea, definitely not on purpose but it's been like that since XF :-/

https://github.com/xamarin/Xamarin.Forms/blob/ba709a180f1ac2218a1c9fa8b0566bd92e96e215/Xamarin.Forms.Core/Brush.cs#L78

So probably been wrong for a bit now