unoplatform / uno

Build Mobile, Desktop and WebAssembly apps with C# and XAML. Today. Open source and professionally supported.
https://platform.uno
Apache License 2.0
8.77k stars 706 forks source link

Add Windows.UI.Xaml.CreateDefaultValueCallback Support #4347

Closed robloo closed 2 weeks ago

robloo commented 3 years ago

What would you like to be added:

Support for Windows.UI.Xaml.CreateDefaultValueCallback

Why is this needed:

When a property is not value-type, suppling a default value should be done using a CreateDefaultValueCallback() delegate to ensure a new instance is created for every DependencyObject.

This is used to implement SelectedItems properties in custom controls (a partial measure since read-only dependency properties are not supported in WinUI).

For which Platform:

Anything else we need to know?

Source location:

robloo commented 3 years ago

I'm curious why this hasn't been implemented yet? Is there an Uno-specific work-around? Is there an architectural issue caused by the way Uno implements dependency properties?

jeromelaban commented 3 years ago

Thanks for the report!

There's isn't any particular issue for that member, it's just never been requested :) Yet it definitely makes sense to implement it.

robloo commented 3 years ago

@jeromelaban Thanks for the feedback. I can imlement this then.

Would the correct place be here in the get accessor: https://github.com/unoplatform/uno/blob/5c53c2f37df48c9d74bd2ef34afe58bc7e43760e/src/Uno.UI/UI/Xaml/PropertyMetadata.cs#L123-L131

If an internal _defaultValueCallback is present (a new field I would add) that would simply be invoked and the value returned. If not, the existing _defaultValue field would be returned.

It looks like changing it here would be enough for all code paths but I'm no where close to familiar with this code base :)

jeromelaban commented 3 years ago

This could be the proper location, though we must be certain that getting that in WinUI property actually invokes CreateDefaultValueCallback every time, and that there's no caching involved.

dr1rrb commented 3 years ago

Be aware that based on my lastest tests and according to the doc:

instead of a fixed constant default value in any case where the default value of a dependency property might be thread-bound

the callback is invoked only once per thread and not for each new instance of the owning control. To my understanding that as been designed to allow usage of DependencyObject as default value in a multi-window app (i.e. multi UI thread / dispatcher).

robloo commented 3 years ago

That's interesting. I wonder how there hasn't been an issue in the past on some custom controls that use this. Perhaps because only one custom control is used at a time... Either I completely misunderstood how this behaves over the years or they changed the docs to make a bug a feature.

I probably won't add this to Uno if that's how it is supposed to work. Instead, I'll set default values in constructors to avoid sharing a reference.

Youssef1313 commented 3 years ago

Will take a look :)