Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dansiegel opened this issue Jun 23, 2021 · 7 comments
Open

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

dansiegel opened this issue Jun 23, 2021 · 7 comments
Labels
area-controls-general General issues that span multiple controls, or common base classes such as View or Element partner Issue or Request from a partner team t/enhancement ☀️ New feature or request
Milestone

Comments

@dansiegel
Copy link
Contributor

dansiegel commented Jun 23, 2021

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).

@jsuarezruiz jsuarezruiz added the t/enhancement ☀️ New feature or request label Oct 25, 2021
@mattleibow mattleibow added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Nov 12, 2021
@mattleibow
Copy link
Member

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
Copy link
Contributor

Clancey commented Nov 12, 2021

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.

@Eilon Eilon added the area-controls-general General issues that span multiple controls, or common base classes such as View or Element label Feb 11, 2022
@samhouts samhouts added the partner Issue or Request from a partner team label Jul 20, 2022
@jfversluis jfversluis added this to the Backlog milestone Aug 12, 2022
@ghost
Copy link

ghost commented Aug 12, 2022

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
Copy link
Member

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

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Mar 7, 2024

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

readonly Dictionary<BindableProperty, BindablePropertyContext> _properties = new Dictionary<BindableProperty, BindablePropertyContext>(4);

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.

@mattleibow
Copy link
Member

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
Copy link
Member

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.

@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-general General issues that span multiple controls, or common base classes such as View or Element partner Issue or Request from a partner team t/enhancement ☀️ New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants