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

[Proposal] BindableProperty generator #542

Open
1 of 10 tasks
pictos opened this issue Aug 13, 2022 · 15 comments · May be fixed by #1321
Open
1 of 10 tasks

[Proposal] BindableProperty generator #542

pictos opened this issue Aug 13, 2022 · 15 comments · May be fixed by #1321
Assignees
Labels
approved This Proposal has been approved and is ready to be added to the Toolkit champion A member of the .NET MAUI Toolkit core team has chosen to champion this feature help wanted This proposal has been approved and is ready to be implemented proposal A fully fleshed out proposal describing a new feature in syntactic and semantic detail

Comments

@pictos
Copy link
Member

pictos commented Aug 13, 2022

[Bindable Property (BP) generator]

  • Proposed
  • Prototype: Not Started
  • Implementation: Not Started
    • iOS Support
    • Android Support
    • macOS Support
    • Windows Support
  • Unit Tests: Not Started
  • Sample: Not Started
  • Documentation: Not Started

Link to Discussion

Summary

This feature will make our lives easier (and in the future the lives of our users) allowing us to create BindableProperties without having to type too much (I hope).

Important, this just covers the BindableProperty.Create method!

This does not cover the BindableProperty.CreateAttached

Motivation

We are lazy devs that don't like to write boilerplate code, so the SG is here to save us! Let's use it

Detailed Design

Right now we don't have a final decision on what the API should look like, and it's fine so far, so I'll post here some proposals that we can vote on. Maybe we can have more than one implemented in order to make the class readable (avoid a lot of attributes at the class level for example). It's very easy to implement an attribute that covers all parameters from the BindableProperty.Create method, so we can do that in our v1.

1. Using attributes at the class level

 [BindableProperty(PropertyName = "CardTitle", ReturnType = typeof(string), OwnerType = typeof(CardView), DefaultValue = string.Empty, PropertyChangedMethodName = "Invalidate")]
 [BindableProperty(PropertyName = "CardDescription", ReturnType = typeof(string)]
public partial class CardView : ContentView 
{
       public static void Invalidate() { }  // OnPropertyChanged method
}

// SG will generate
public partial class CardView
{
      public static readonly BindableProperty CardTitleProperty = BindableProperty.Create("CardTitle", typeof(string), typeof(CardView), string.Empty, propertyChaged: Invalidate);
    public string CardTitle
    {
         get => (string)GetValue(CardTitleProperty);
         set => SetValue(CardTitleProperty, value);
    }

     public static readonly BindableProperty CardDescriptionProperty = BindableProperty.Create("CardDescription", typeof(string), typeof(CardView));

    public string CardDescription
    {
         get => (string)GetValue(CardDescriptionProperty);
         set => SetValue(CardDescriptionProperty, value);
    }
}

The CardTitle shows how will be a more complete implementation if the user needs it and the CardDescription shows how will a simple implementation when there's no need to specify all the parameters. Also, in the CardDescription we don't need to provide the OwnerType, the SG can infer that will the class.

2. Using attributes at the ctor level

public partial class CardView : ContentView 
{

       [BindableProperty(PropertyName = "CardTitle", ReturnType = typeof(string), OwnerType = typeof(CardView), DefaultValue = string.Empty, PropertyChangedMethodName = "Invalidate")]
       [BindableProperty(PropertyName = "CardDescription", ReturnType = typeof(string)]
       public CardView() { }

       public static void Invalidate() { }  // OnPropertyChanged method
}

// SG will generate
public partial class CardView
{
      public static readonly BindableProperty CardTitleProperty = BindableProperty.Create("CardTitle", typeof(string), typeof(CardView), string.Empty, propertyChaged: Invalidate);
    public string CardTitle
    {
         get => (string)GetValue(CardTitleProperty);
         set => SetValue(CardTitleProperty, value);
    }

     public static readonly BindableProperty CardDescriptionProperty = BindableProperty.Create("CardDescription", typeof(string), typeof(CardView));

    public string CardDescription
    {
         get => (string)GetValue(CardDescriptionProperty);
         set => SetValue(CardDescriptionProperty, value);
    }
}

The same as the 1 but now we decorate the ctor. If we think this will look better
But this can be used in a method as well, like if the user creates the propertyChanged method it can be used to create the BP using the attribute.

3 Using the Property

[BindablePropertyGenerator]
public partial class CardView : ContentView 
{
    [BindableProperty(PropertyName = "CustomName", OwnerType = typeof(CardView), DefaultValue = string.Empty, PropertyChangedMethodName = "Invalidate")]
    public string CardTitle
    {
         get => (string)GetValue(CustomNameProperty);
         set => SetValue(CustomNameProperty, value);
    }
    public static void Invalidate() { }  // OnPropertyChanged method

    [BindableProperty]
    public string CardDescription
    {
         get => (string)GetValue(CardDescriptionProperty);
         set => SetValue(CardDescriptionProperty, value);
    }
}

// SG will generate
public partial class CardView
{
      public static readonly BindableProperty CustomNameProperty = BindableProperty.Create("CardTitle", typeof(string), typeof(CardView), string.Empty, propertyChaged: Invalidate);

     public static readonly BindableProperty CardDescriptionProperty = BindableProperty.Create("CardDescription", typeof(string), typeof(CardView));

}

Here we can just mark a global attribute to SG lookup and then we use the properties to hold the attribute. With this, we can infer the return type and the Bindable Property name. The user will need to implement the getter and setter, but if this is in the language some day we can implement the getter and setter.

4 Using the BindableProperty field

//User code

public partial class CardView  : ContentView 
{
     [BindableProperty(PropertyName = "CardTitle", ReturnType = typeof(string), OwnerType = typeof(CardView), DefaultValue = string.Empty, PropertyChangedMethodName = "Invalidate")]
    public static readonly BindableProperty CardTitleProperty;

    partial void InitializeStatic()
    {
       lbl.GesturesRecognizer.Add(new GestureRecognizer());
    }

    public static void Invalidate() { }  // OnPropertyChanged method
}

// SG will generate

public partial class CardView
{
    static CardView()
    {
        CardTitleProperty = BindableProperty.Create("CardTitle", typeof(string), typeof(CardView), string.Empty, propertyChaged: Invalidate);
        InitializeStatic();
    }

    static partial void InitializeStatic();

    public string CardDescription
    {
         get => (string)GetValue(CardDescriptionProperty);
         set => SetValue(CardDescriptionProperty, value);
    }

}

Here we will use the BindableProperty field to hold the attribute and generate the rest of the code.

Usage Syntax

I'll fill this out when we define an API design.

Drawbacks

When C#11 arrives with generic in attributes we can make it cleaner, so will be breaking changes in the future (IMHO we should do breaking change instead of maintaining one more pattern to declare the BP, but this is a discussion for the future).

the docs say that will be in C#10 but it was changed.

Alternatives

None.

Unresolved Questions

@pictos pictos added new proposal A fully fleshed out proposal describing a new feature in syntactic and semantic detail labels Aug 13, 2022
@pictos pictos self-assigned this Aug 13, 2022
@ghost ghost added this to Proposal Submitted in New Feature Proposals Aug 13, 2022
@VladislavAntonyuk
Copy link
Collaborator

I like the option 1.
Do we need OwnerType property? It is the same as typeof(Class)

@VladislavAntonyuk
Copy link
Collaborator

We can include this feature with .NET 7 release and C# 11 later this year.

@bijington
Copy link
Contributor

bijington commented Aug 14, 2022

Is it possible to enhance option 1 to allow us to annotate our methods for things like PropertyChanged handlers?

[BindableProperty(
    PropertyName = "CardTitle",
    ReturnType = typeof(string),
    OwnerType = typeof(CardView),
    DefaultValue = string.Empty)]
public partial class CardView : ContentView 
{
     [PropertyChangedFor(nameof(CardTitle))]
     public static void OnCardTitleChanged(BindableObject sender, object oldValue, object newValue)
     {
     }
}

// SG will generate
public partial class CardView
{
    public static readonly BindableProperty CardTitleProperty =
        BindableProperty.Create(
            "CardTitle",
            typeof(string),
            typeof(CardView),
            string.Empty,
            propertyChaged: OnCardTitleChanged);

    public string CardTitle
    {
         get => (string)GetValue(CardTitleProperty);
         set => SetValue(CardTitleProperty, value);
    }
}

@pictos
Copy link
Member Author

pictos commented Aug 14, 2022

I like the option 1. Do we need OwnerType property? It is the same as typeof(Class)

@VladislavAntonyuk, it's possible to infer but is good to expose if the user wants to specify another owner class (for example the base class). I showed that in the CardDescription sample.

Is it possible to enhance option 1 to allow us to annotate our methods for things like PropertyChanged handlers?

@bijington, yes that should be possible

@brminnick
Copy link
Collaborator

I vote for Option 4.

I love the idea of aligning to the MVVM Toolkit and putting the attribute on the Field.

We’d want to include an Analyzer to provide a helpful error message to devs who maybe try to implement a static constructor themselves, letting them know to use the partial method instead. However, I don’t imagine many developers are using static constructors on their views.

And if we proceed with Option 4, we can leverage @Sergio0694’s expertise and the awesome code he’s already written in the MVVM Toolkit!

@pictos
Copy link
Member Author

pictos commented Aug 16, 2022

We’d want to include an Analyzer to provide a helpful error message to devs who maybe try to implement a static constructor themselves, letting them know to use the partial method instead. However, I don’t imagine many developers are using static constructors on their views.

we can do this at the SG level

@bijington
Copy link
Contributor

For Option 4 if we could somehow guarantee that the field name ends in Property (not sure we can) then we wouldn't need the dev to provide the name for the property.

@bijington
Copy link
Contributor

bijington commented Aug 16, 2022

I think I still prefer Option 1. While Option 4 might be slightly more aligned with the MVVM toolkits approach, it is only by the fact that devs place an attribute on the backing field. The field itself doesn't contain anything useful in terms of type information needed, this leads to an additional line of code for developers which provides no value to them and also the need to workaround the scenario where developers might have chosen to define a static constructor.

I do agree it would be nicer to place the attribute to closest related context in the class but I can't decide what that is.

@Sergio0694
Copy link
Member

Sergio0694 commented Aug 16, 2022

FWIW, I plan to implement this (for DependencyProperty) for UWP/WinUI 3, but I'll wait for C# 12 and hopefully partial properties. While other approaches would also work, I don't personally like them (especially using fields over a class, with the property name as a string), as they're just not idiomatic enough. You might want to consider when you plan to ship this generator as a stable release. If it's something that might be done in like a couple months, it's one thing, but if this is something that could reach a stable release in a year, then it might just be worth it to wait for C# 12 directly and do that in a consistent way with the Windows Community Toolkit as well. Just my two cents here 🙂

As in:

[DependencyProperty(...)]
public partial string Name { get; set; }

If/when we do get partial properties I also plan to switch to them for the MVVM Toolkit, and eventually deprecate annotating fields. That's really just a temporary workaround for now, and I don't really like it. It's just the best we could do for now. If this is something the MAUI Community Toolkit team was interested in, we could use this as another use case scenario in the C# language proposal we're working on, to try to get more support for this.

Either way, I'm available in case you have any source generator questions you think I could help with 😄

@brminnick
Copy link
Collaborator

brminnick commented Aug 16, 2022

Thanks Sergio!!

The field itself doesn't contain anything useful in terms of type information needed

Just one quick note: The Field provides us the declaring type of the BindableProperty and its name. For example public static readonly string LabelTextProperty; tells us that we are generating code for a string called public string LabelText { get; set; }.

Edit: Whoops - I misread the spec!

@pictos
Copy link
Member Author

pictos commented Aug 16, 2022

@brminnick but the field will not be of string or any other type but BindableProperty

@bijington
Copy link
Contributor

Thanks Sergio!!

The field itself doesn't contain anything useful in terms of type information needed

Just one quick note: The Field provides us the declaring type of the BindableProperty and its name. For example public static readonly string LabelTextProperty; tells us that we are generating code for a string called public string LabelText { get; set; }.

Edit: Whoops - I misread the spec!

Oh yes sorry I didn't give much detail on that. The other issue is we would be relying on the developer to add the Property suffix which could be prone to typos, etc.

@egvijayanand
Copy link

I still feel too many keystrokes are involved in the options mentioned as the user has to define those attributes at some level (field/class). Rather this can be addressed with code snippets that will automate it as a template and once inserted, modify the necessary ones.

@pictos pictos added the needs discussion Discuss it on the next Monthly standup label Sep 30, 2022
@brminnick brminnick moved this from Proposal Submitted to Proposal Approved in New Feature Proposals Oct 6, 2022
@ghost ghost added approved This Proposal has been approved and is ready to be added to the Toolkit help wanted This proposal has been approved and is ready to be implemented and removed new labels Oct 6, 2022
@brminnick brminnick added the champion A member of the .NET MAUI Toolkit core team has chosen to champion this feature label Oct 6, 2022
@VladislavAntonyuk VladislavAntonyuk removed the needs discussion Discuss it on the next Monthly standup label Oct 6, 2022
@brminnick brminnick linked a pull request Aug 14, 2023 that will close this issue
@tranb3r
Copy link

tranb3r commented Dec 15, 2023

The PR seems to be for an internal SG.
Is this going to be public (and replace M.BindableProperty.Generator)?

@VladislavAntonyuk
Copy link
Collaborator

@tranb3r for now it will be internal. We check and apply it for our library (possibly find and fix issues). When we see and stable enough, we'll make it public.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This Proposal has been approved and is ready to be added to the Toolkit champion A member of the .NET MAUI Toolkit core team has chosen to champion this feature help wanted This proposal has been approved and is ready to be implemented proposal A fully fleshed out proposal describing a new feature in syntactic and semantic detail
Projects
New Feature Proposals
Proposal Approved
Development

Successfully merging a pull request may close this issue.

7 participants