Skip to content

Add INotifyPropertyChanged to Builder classes#79

Merged
AArnott merged 5 commits intomasterfrom
fix78
Jun 28, 2016
Merged

Add INotifyPropertyChanged to Builder classes#79
AArnott merged 5 commits intomasterfrom
fix78

Conversation

@AArnott
Copy link
Copy Markdown
Owner

@AArnott AArnott commented Jun 26, 2016

Fixes #78

@johannesegger
Copy link
Copy Markdown

Looks very good, thanks.
However, the equality check is more general if you used System.Collections.Generic.EqualityComparer<T>.Default.Equals(this.fieldName, value).

I also started working on it because I finally got it to build on my home machine, but I didn't add any tests. You can take a look at the equality check for example. I also inlined OnPropertyChanged and used a string literal for the property name (because it's generated code I thought it doesn't matter).
But I obviously didn't consider everything because I don't know the code base as good as you do.

@AArnott
Copy link
Copy Markdown
Owner Author

AArnott commented Jun 26, 2016

Great. I was uncertain my equality check would be adequate. There's multiple things to consider. I'll brainstorm here with you later... gotta run for now.

@AArnott
Copy link
Copy Markdown
Owner Author

AArnott commented Jun 27, 2016

I think when the property value has a reference type, it'll be crucial that the field is updated and the event raised any time the reference changes, regardless of value equality between the two references. The reason is consumers such as WPF data binding can hook the value itself for its own property changes if it is a reference type, and if the reference changes, WPF has to reset all its subscriptions.
The == operator defaults to reference equality for reference types. But this operator can be overloaded so using object.ReferenceEquals would be safer.

When the property is over a value type, the setter should either always assign the value and raise the event, or have an equality comparer that really works. An overridden Equals method would do the trick, as the EqualityComparer<T>.Default.Equals would then call it. But do we trust it, or do we always raise it for value types instead?
Given that I'm pretty sure different folks would give different answers as to what the right behavior should be, I wish there was a way to turn it over to the consumer to decide. I'm thinking through various ways to do that.

@AArnott
Copy link
Copy Markdown
Owner Author

AArnott commented Jun 27, 2016

Looking more into this, I see the immutable types' WithFactory method already use the != operator to decide whether a value is significantly different enough to allocate a new object. I wonder if there are bugs there...

@johannesegger
Copy link
Copy Markdown

I proposed using EqualityComparer because I thought it's the standard way of how MVVM libraries are doing it. However I did a (very quick) research and found that there is no consensus:

Your argument about WPF having to reset its bindings is very interesting. I wonder how MVVM libraries are dealing with this or if this is a known problem at all.

@AArnott
Copy link
Copy Markdown
Owner Author

AArnott commented Jun 27, 2016

I'm willing to bet that these other libraries may not be adequately tested enough to notice the bugs that they incur when using reference types in their fields with types that implement INPC themselves.
I'm going to go with the != operator, and test during code gen whether the operator is defined and if not, skip the test (and assume the value is different) which will make structs 'just work' and if folks want to be more precise about it, they can overload the operators to define them, quite possibly in terms of the default EqualityComparer.

@AArnott
Copy link
Copy Markdown
Owner Author

AArnott commented Jun 28, 2016

OK, in its final form, property setters have at least 3 unique sets of conditions (plus no condition at all) based on the type of field the property setter is for.
I feel comfortable with the != operator.

@eggapauli if you're interested in pursuing the EqualityComparer question with the other MVVM frameworks you found, I'll describe a repro for a bug that I suspect they all have if you want to verify it and file bugs against them.

@AArnott AArnott merged commit 3f8c164 into master Jun 28, 2016
@AArnott AArnott deleted the fix78 branch June 28, 2016 01:05
@johannesegger
Copy link
Copy Markdown

Ok, super, thanks for implementing this.
!= will be fine for us, we can always filter out duplicates using Rx.

I'll see if I can reproduce it myself, thanks for the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants