[Enhancement] Add a way to get properties from the Core

dansiegel commented 3 years ago

Summary

The Handlers each have a static dictionary that will allow you to specify a Property name to add a Mapper of something to do when the value has been updated on a given control. However when working with these mappers you ultimately are dealing with an ILabel, IButton, etc. As a result you cannot access attached properties on the control from the core level. As a result in order to make use of this a Library developer would have to duplicate logic across Comet and Maui to support both design patterns. Since both Maui and Comet ultimately contain a dictionary on the controls, this should be easily achievable by exposing some method on a base class to access values for any given property name from a Core context.

API Changes

public interface IPropertyCollection
{
    object GetValue(string key);
}

With this implemented say on BindableObject, you could then update handlers like:

<Label foo:MyClass.MyAttachedProperty="FooBar" />

LabelHandler.LabelMapper["MyAttachedProperty"] => (LabelHandler handler, ILabel label)
{
    if(label is IPropertyCollection pc)
    {
        var value = pc.GetValue("MyAttachedProperty");
        // value = "FooBar"
    }
};

Intended Use Case

With something (and no I'm not tied to the name) like IPropertyCollection implemented on BindableObject, this would provide library authors the ability to write Property Mappers one time within a Core support library. This reduces the need for first implementing an Interface, and then providing a custom control that implements the interface, which then means that it would never be supported on Out-of-Box controls.

Note that currently in order to achieve the desired effect extensions would need to be duplicated and cast the ILabel (or whatever) as the concrete implementation for Maui / Comet. Since both Comet and Maui controls are effectively Dictionaries, this approach would work very well, while providing flexibility to not work the same if some new implementation were introduced where all of the controls were records or something that might require reflection (per @Clancey).

mattleibow commented 2 years ago

This might be a bit hard to do since BindableObject only lives in the Controls layer, so it will only be able to exist there. Comet does not use Controls so I am not sure how this could be implemented at a Core layer.

If we have it add Core, does Comet have a property bag that we can just pull from? If so, then it might work.

But, another issue is that with Controls, we use BindableProperty - which appears to be string based, but is actually an object instance. Se we can't do myControl.GetValue("Name") because that method actually requires a BindableProperty.

Clancey commented 2 years ago

Comet already has this, you can just do cometView.GetEnvironment<Foo>() So not a problem. It's how you do any property that is not required.

ghost commented 1 year ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

mattleibow commented 4 months ago

@jonathanpeppers before we even think this, I feel this will be even more string-y... Any trimming/AOT thoughts on this one?

jonathanpeppers commented 4 months ago

If BindableObject implemented this new IPropertyCollection interface, it would be O(N) for operations, as the backing store is:

https://github.com/dotnet/maui/blob/7639a2cf24ac3e5a17a08fc1eb0103883659d57d/src/Controls/src/Core/BindableObject.cs#L38

It would have to iterate over the keys until it found a match. This overhead doesn't seem like a great plan to me.

Overall, I'm not a fan of the handlers, in general:

The API isn't great:
- They are static. If this was meant for an application-wide configuration setting, we should have used the "app builder" pattern as that would be closer to what ASP.NET does.
We added a Dictionary<string,> lookup for operations
- BP's would be ok if they were just an order of magnitude worse than a plain C# property. But I think setting a BP currently goes through two dictionaries before calling the platform side. 👀
All properties are set on every control, even for default values. Every View with no TranslationX/Y, RotationX/Y stills sets all these to 0.
- In some cases it's even multiple times:
- https://github.com/dotnet/maui/issues/17303

mattleibow commented 4 months ago

Didn't they make a fast read only dictionary we could maybe switch to...

https://learn.microsoft.com/en-us/dotnet/api/system.collections.frozen.frozendictionary-2?view=net-8.0

Also, this may be restricted to attached properties as there is not really a way to get things since it is not actually part of the view.

jonathanpeppers commented 4 months ago

Frozen collections are slower to create but faster to access. You'd need to do the startup cost / runtime benefit, to see if that would help handlers.

dotnet / maui