Skip to content
This repository has been archived by the owner on Jan 16, 2024. It is now read-only.

Optimized NotifyPropertyChanged support #23

Closed
lizardking opened this issue Sep 19, 2018 · 2 comments
Closed

Optimized NotifyPropertyChanged support #23

lizardking opened this issue Sep 19, 2018 · 2 comments
Labels
as designed As designed clarification Feature needs clarity to make it more user friendly

Comments

@lizardking
Copy link

The current T4 files are generating code that calls OnPropertyChanged whenever the setter of a property is called (even if the value doesnt change). According to the example in How to: Implement the INotifyPropertyChanged Interface OnPropertyChanged is only called when there is a actual change of the value (also my personal preference since it avoids unecessary events).

I'm now using a modifed EFDesigner.ttinclude in my project which generates the following code for the properties:

      #region Property Name of type string
      [Required]
      protected string _Name;

      /// <summary>
      /// Required, Min length = 250
      /// </summary>
      public string Name
      {
         get {return _Name;}
         set {if (_Name != value) {string oldName=_Name; NameChanging(oldName, value);  _Name = value;OnPropertyChanged(nameof(Name)); NameChanged(oldName, value);}}
      }
      partial void NameChanging(string oldName, string newName);
      partial void NameChanged(string oldName, string newName);
      #endregion

Below is the modified method in the modified EFDesigner.ttinclude which I have included in my project. I has the improved OnPropertyChanged handling and it also excludes the ConccurencyToken from calling OnPropertyChanged.

void WritePersistentProperties(ModelClass modelClass)
{
   if (!modelClass.Attributes.Any(x => x.Persistent))
      return;

   Output("// Persistent properties");
   List<string> segments = new List<string>();
   NL();

   foreach (ModelAttribute modelAttribute in modelClass.Attributes.Where(x => x.Persistent))
   {
      string nullable = IsNullable(modelAttribute) ? "?" : "";

      Output($"#region {(modelAttribute.IsConcurrencyToken?"ConcurrencyToken":"Property")} {modelAttribute.Name} of type {modelAttribute.FQPrimitiveType}{nullable}");
      
	  segments.Clear();

      if (modelAttribute.IsIdentity)
         segments.Add("Identity");
      if (modelAttribute.Required || modelAttribute.IsIdentity)
         segments.Add("Required");
      if (modelAttribute.Indexed)
         segments.Add("Indexed");
      if (modelAttribute.MinLength > 0)
         segments.Add($"Min length = {modelAttribute.MinLength}");
      if (modelAttribute.MaxLength > 0)
         segments.Add($"Max length = {modelAttribute.MaxLength}");
      if (!string.IsNullOrEmpty(modelAttribute.InitialValue))
      {
         string quote = modelAttribute.PrimitiveType == "string" ? "\"" : modelAttribute.PrimitiveType == "char" ? "'" : "";
         segments.Add($"Default value = {quote}{FullyQualified(modelClass.ModelRoot, modelAttribute.InitialValue)}{quote}");
      }

     

      if (!modelAttribute.AutoProperty && !modelAttribute.IsConcurrencyToken)
      {
         GenerateClassAnnotations(modelAttribute, "_");
         Output($"protected {modelAttribute.FQPrimitiveType}{nullable} _{modelAttribute.Name};");

         NL();
      }

      if (!string.IsNullOrEmpty(modelAttribute.Summary) || segments.Any())
      {
         Output("/// <summary>");
         if (segments.Any())
            Output($"/// {string.Join(", ", segments)}");
         if (!string.IsNullOrEmpty(modelAttribute.Summary))
            Output("/// {0}", modelAttribute.Summary);
         Output("/// </summary>");
      }
      if (!string.IsNullOrEmpty(modelAttribute.Description))
      {
         Output("/// <remarks>");
         Output("/// {0}", modelAttribute.Description);
         Output("/// </remarks>");
      }

      string setterVisibility = modelAttribute.SetterVisibility == SetterAccessModifier.Protected ? "protected " : modelAttribute.SetterVisibility == SetterAccessModifier.Internal ? "internal " : "";

      if (modelAttribute.AutoProperty || modelAttribute.IsConcurrencyToken)
      {
         GenerateClassAnnotations(modelAttribute);
         Output($"public {modelAttribute.FQPrimitiveType}{nullable} {modelAttribute.Name} {{ get; {setterVisibility}set; }}");
      }
      else
      {

         Output($"public {modelAttribute.FQPrimitiveType}{nullable} {modelAttribute.Name}");
         Output("{");
         Output($"get {{return _{modelAttribute.Name};}}");
         Output($"{setterVisibility}set {{if (_{modelAttribute.Name} != value) {{{modelAttribute.FQPrimitiveType}{nullable} old{modelAttribute.Name}=_{modelAttribute.Name}; {modelAttribute.Name}Changing(old{modelAttribute.Name}, value);  _{modelAttribute.Name} = value;OnPropertyChanged(nameof({modelAttribute.Name})); {modelAttribute.Name}Changed(old{modelAttribute.Name}, value);}}}}");
         Output("}");
      
	     Output($"partial void {modelAttribute.Name}Changing({modelAttribute.FQPrimitiveType}{nullable} old{modelAttribute.Name}, {modelAttribute.FQPrimitiveType}{nullable} new{modelAttribute.Name});");
	     Output($"partial void {modelAttribute.Name}Changed({modelAttribute.FQPrimitiveType}{nullable} old{modelAttribute.Name}, {modelAttribute.FQPrimitiveType}{nullable} new{modelAttribute.Name});");
	  }

	   Output("#endregion");
      NL();
   }

   if (!modelClass.AllAttributes.Any(x => x.IsConcurrencyToken) && 
       (modelClass.Concurrency == ConcurrencyOverride.Optimistic || modelClass.ModelRoot.ConcurrencyDefault == Concurrency.Optimistic))
   {
      Output("/// <summary>");
      Output("/// Concurrency token");
      Output("/// </summary>");
      Output("[Timestamp]");
      Output("public Byte[] Timestamp { get; set; }");
      NL();
   }
}
@msawczyn
Copy link
Owner

Nice! Thanks!

@msawczyn msawczyn added enhancement New feature request investigating Looking into this as designed As designed clarification Feature needs clarity to make it more user friendly and removed investigating Looking into this enhancement New feature request labels Sep 19, 2018
@msawczyn
Copy link
Owner

I've given this a deep look and made the decision not to add the OnChainging and OnChanged handlers. The partials that exist so the programmer can intervene in the value that's set serve the same purpose, in the general sense, so the same codebase can dual wield for both use cases.

The nice thing, as you've discovered, is that you can change it to suit your needs. I'll be focused on the most generally useful thing (usually following the 80/20 rule as much as possible), but everyone's able and welcome to morph it to their heart's content. I did add a few fixes and removed some dead code, and those are in the v1.2.6.7 release.

I will be adding more documentation on this feature when I get a moment. I've put a placeholder in the docs and a note in the TODO list so I don't forget. You're right that this needs more explanation that I've currently given it (read: hardly any!), so I'll try to get to that as soon as time and my wife permit.

Thanks for your help in this!

msawczyn added a commit that referenced this issue Sep 24, 2018
   - An entity's concurrency token property is no longer a required parameter in its constructor (#24)
   - Simplified cascade delete settings in property editor for associations
   - Fixed bad code generation in EFCore for cascade delete overrides (#22)
   - Missing files when generating code for .NET Core projects fixed
   - Tightened up and swatted some bugs in INotifyPropertyChanged handling. Added documentation to doc site for this feature (following up on #23)
   - Ensured multiline editing was available in property window for those properties that made sense
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
as designed As designed clarification Feature needs clarity to make it more user friendly
Projects
None yet
Development

No branches or pull requests

2 participants