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

Change the generator to call existing user method with compatible signature #3

Closed
virzak opened this issue Jul 13, 2022 · 11 comments
Closed

Comments

@virzak
Copy link

virzak commented Jul 13, 2022

Previously discussed in #2

Advantages in comparison to the current approach:

  • Doesn't require user code to use partial keyword, hence smoother transition
  • Allows virtual
  • Allows non standard variable names
  • Cleaner code generator, which invokes one specific method non-partial method

Possible disadvantage: slower code generation, but should not be slower than CommunityToolkit/dotnet#8 code generator.

Optionally consider adding an OnChanged property to the attributes which would specify the method to be called: OnChanged = nameof(NonStandardPropertyChanged). This is the equivalent of CanExecute property in the [RelayCommand] attribute in CommunityToolkit.Mvvm library.

@HavenDV
Copy link
Owner

HavenDV commented Jul 28, 2022

I have released an update that allows this. The generator will automatically determine one of the three (four for Attached properties) allowed signatures and will call it in the callback.

[DependencyProperty<string>(""Text"", OnChanged = nameof(OnMyTextChanged))]
public partial class Generatable : FrameworkElement
{
    protected virtual void OnMyTextChanged(string? value)
    {
    }
}

will generate:

//HintName: Generatable.Properties.Text.generated.cs

#nullable enable

namespace H.Generators.IntegrationTests
{
    public partial class Generatable
    {
        /// <summary>
        /// Default value: default(string)
        /// </summary>
        public static readonly global::System.Windows.DependencyProperty TextProperty =
            global::System.Windows.DependencyProperty.Register(
                name: "Text",
                propertyType: typeof(string),
                ownerType: typeof(global::H.Generators.IntegrationTests.Generatable),
                typeMetadata: new global::System.Windows.FrameworkPropertyMetadata(
                    defaultValue: default(string),
                    flags: global::System.Windows.FrameworkPropertyMetadataOptions.None,
                    propertyChangedCallback: static (sender, args) =>
                    {
                        ((global::H.Generators.IntegrationTests.Generatable)sender).OnMyTextChanged(
                            (string?)args.NewValue);
                    },
                    coerceValueCallback: null,
                    isAnimationProhibited: false),
                validateValueCallback: null);

        /// <summary>
        /// Default value: default(string)
        /// </summary>
        public string? Text
        {
            get => (string?)GetValue(TextProperty);
            set => SetValue(TextProperty, value);
        }

    }
}

@virzak
Copy link
Author

virzak commented Jul 28, 2022

I think there's an issue with the nuget package. It doesn't get updated and is marked as deprecated.

https://www.nuget.org/packages/DependencyPropertyGenerator.Core

@HavenDV
Copy link
Owner

HavenDV commented Jul 28, 2022

Remove it
image

@virzak
Copy link
Author

virzak commented Jul 28, 2022

Ok, I gave it a try and it looks good so far.

Lack of OnChanged still calls partial methods. Are there any plans to adopt this new approach when OnChanged isn't specified?

@HavenDV
Copy link
Owner

HavenDV commented Jul 28, 2022

No, the absence of OnChanged will still work with partial methods, allowing the IDE to quickly generate an OnChanged method. I find this to be a more appropriate default behavior than manually guessing the signature.

@virzak
Copy link
Author

virzak commented Jul 28, 2022

Cool, will you be changing it for AttachedDependencyPropertyAttribute? I can specify OnChanged, but it doesn't produce the call to the method. I think it is a bug.

@virzak
Copy link
Author

virzak commented Jul 28, 2022

manually guessing the signature

I don't know if that's a worse solution than the current one, since the code generator is currently guessing 3 different signatures. If the code generator looks into the code, I feel it is less of a guess. Also with the current solution, a user could implement all 3 partial methods and all 3 would get executed.

In any case, this isn't a big deal to me, very happy with the code gen overall.

@HavenDV
Copy link
Owner

HavenDV commented Jul 29, 2022

Cool, will you be changing it for AttachedDependencyPropertyAttribute? I can specify OnChanged, but it doesn't produce the call to the method. I think it is a bug.

I will check and add tests for this case

I don't know if that's a worse solution than the current one, since the code generator is currently guessing 3 different signatures. If the code generator looks into the code, I feel it is less of a guess. Also with the current solution, a user could implement all 3 partial methods and all 3 would get executed.

I imagine the standard user behavior like this:

  1. User adds [DependencyProperty<string>(""Text"")]
  2. Writes partial void in the class body
  3. Presses space
  4. The IDE offers him a choice of 3 signatures that he can override.
  5. Chooses and the IDE creates an empty method with that signature.
  6. During generation, the callback will not be created if the user has not defined at least one of the partial signatures. Here, up to 4 signature checks pass (because I'm not sure that the compiler is smart enough to cut out all the code inside a method call, like casts) and only those that the user has defined in his code remain.

For me, the main benefit of having partial declarations is that the IDE will offer a choice and create an empty method.

@HavenDV
Copy link
Owner

HavenDV commented Jul 29, 2022

Cool, will you be changing it for AttachedDependencyPropertyAttribute? I can specify OnChanged, but it doesn't produce the call to the method. I think it is a bug.

I couldn't repeat. I used:

public enum Mode
{
    Mode1,
    Mode2,
}

[AttachedDependencyProperty<Mode, TreeView>("Mode", OnChanged = nameof(OnModeChanged))]
public static partial class TreeViewExtensions
{
    private static void OnModeChanged(TreeView sender, Mode oldValue, Mode newValue)
    {
    }
}

Generated code:

//HintName: TreeViewExtensions.AttachedProperties.Mode.generated.cs

#nullable enable

namespace H.Generators.IntegrationTests
{
    public static partial class TreeViewExtensions
    {
        /// <summary>
        /// Default value: default(Mode)
        /// </summary>
        public static readonly global::System.Windows.DependencyProperty ModeProperty =
            global::System.Windows.DependencyProperty.RegisterAttached(
                name: "Mode",
                propertyType: typeof(global::H.Generators.IntegrationTests.Mode),
                ownerType: typeof(global::H.Generators.IntegrationTests.TreeViewExtensions),
                defaultMetadata: new global::System.Windows.FrameworkPropertyMetadata(
                    defaultValue: default(global::H.Generators.IntegrationTests.Mode),
                    flags: global::System.Windows.FrameworkPropertyMetadataOptions.None,
                    propertyChangedCallback: static (sender, args) =>
                    {
                        OnModeChanged(
                            (global::System.Windows.Controls.TreeView)sender,
                            (global::H.Generators.IntegrationTests.Mode)args.OldValue,
                            (global::H.Generators.IntegrationTests.Mode)args.NewValue);
                    },
                    coerceValueCallback: null,
                    isAnimationProhibited: false),
                validateValueCallback: null);

        /// <summary>
        /// Default value: default(Mode)
        /// </summary>
        public static void SetMode(global::System.Windows.DependencyObject element, global::H.Generators.IntegrationTests.Mode value)
        {
            element = element ?? throw new global::System.ArgumentNullException(nameof(element));

            element.SetValue(ModeProperty, value);
        }

        /// <summary>
        /// Default value: default(Mode)
        /// </summary>
        [global::System.Windows.AttachedPropertyBrowsableForType(typeof(global::System.Windows.Controls.TreeView))]
        public static global::H.Generators.IntegrationTests.Mode GetMode(global::System.Windows.DependencyObject element)
        {
            element = element ?? throw new global::System.ArgumentNullException(nameof(element));

            return (global::H.Generators.IntegrationTests.Mode)element.GetValue(ModeProperty);
        }

    }
}

@virzak
Copy link
Author

virzak commented Jul 29, 2022

This doesn't work for me, taken out of https://rachel53461.wordpress.com/2011/09/17/wpf-grids-rowcolumn-count-properties/:

[AttachedDependencyProperty<int, Grid>("RowCount", OnChanged = nameof(OnRowCountChanged), DefaultValue = -1)]
[AttachedDependencyProperty<int, Grid>("ColumnCount", OnChanged = nameof(OnColumnCountChanged), DefaultValue = -1)]
public static partial class GridHelpers
{
    // Change Event - Adds the Rows
    static void OnRowCountChanged(Grid grid, int newValue)
    {
        grid.RowDefinitions.Clear();

        for (int i = 0; i < newValue; i++)
            grid.RowDefinitions.Add(
                new RowDefinition() { Height = GridLength.Auto });
    }

    static void OnColumnCountChanged(Grid grid, int newValue)
    {
        grid.ColumnDefinitions.Clear();

        for (int i = 0; i < newValue; i++)
            grid.ColumnDefinitions.Add(
                new ColumnDefinition() { Width = GridLength.Auto });
    }
}

Also I just noticed that with oldVaue it works.

@HavenDV
Copy link
Owner

HavenDV commented Jul 29, 2022

This doesn't work for me, taken out of https://rachel53461.wordpress.com/2011/09/17/wpf-grids-rowcolumn-count-properties/:

Yes thanks, forgot to add some checks for static method signatures. Fixed.

Also take a look at this: https://github.com/HavenDV/H.XamlExtensions

@HavenDV HavenDV closed this as completed Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants