[BUG] this.RaiseAndSetIfChanged does not trigger PropertyChanged event for generic IReactiveObject

[timothylcooke]

Describe the bug
If a custom IReactiveObject (in this example, IReactiveObjectFilter) calls this.RaiseAndSetIfChanged(ref _field, value), then RaisePropertyChanging and RaisePropertyChanged methods are never called, and therefore PropertyChanging and PropertyChanged events are not triggered.

While IReactiveObjectFilter.WhenAnyValue(x => x.ShouldFilter) receives notifications when this.RaiseAndSetIfChanged is called, SourceList<IReactiveObjectFilter>.Connect().AutoRefresh(x => x.ShouldFilter) does not receive notifications, since PropertyChanged is never called.

Steps To Reproduce
https://github.com/timothylcooke/IReactiveObjectExample.git demonstrates the behavior. This is a WPF project that shows a window with a TabControl. Both Tabs shows a MyUserControl. Each MyUserControl has a binding to a MyViewModel, which has a SourceList<IFilter> of filters (toggled with the CheckBoxes). In the first tab, the IFilter objects are ReactiveObjectFilter objects, and in the second tab, the IFilter objects are IReactiveObjectFilter objects. Whether the IFilter extends ReactiveObject or implements IReactiveObject determines if the PropertyChanged event is triggered, and therefore if the SourceList<IFilter>.Connect().AutoRefresh(x => x.ShouldFilter) triggers updates.

Expected behavior
If I implement IReactiveObject and call this.RaiseAndSetIfChanged(), it should call RaisePropertyChanged.

Screenshots
In the screenshots below, the key difference is that in the first screenshot, where the IFilter objects inherit from ReactiveObject, we see the line Filter… PropertyChanged("ShouldFilter") fired. This ripples down and ends up triggering filtersSource.Connect().AutoRefresh(x => x.ShouldFilter), and in turn updates the currentFilters collection. In the second screenshot, where the IFilter objects implement IReactiveObject, PropertyChanged is never called on the filter, and therefore filtersSource.Connect().AutoRefresh(x => x.ShouldFilter) is never fired. This leaves our currentFilters collection unmodified, and therefore does not filter any items in the Filtered Names list.

Environment

• OS: Windows 10 Pro x64
• Version ReactiveUI 12.1.5
• Device: Dell XPS 13 9360

Additional context
N/A

[glennawatson]

I'll have a look at this one after I get my massive .net 5 branch sorted.

Will do it as part of a Fody refactor I did a while back and get that in.

[glennawatson]

Also thanks for providing the sample, that always helps.

[timothylcooke]

I just added screenshots of the sample app. I figured that's easier to see than cloning and debugging.

Also thanks for providing the sample, that always helps.

You're welcome. I can't tell you how many "bugs" I've identified in various libraries where I try to make a sample project to reproduce it, find that I can't, and eventually realize it's not a bug and I'm doing something stupid in my main project. I always feel better about reporting something if I can reproduce it in a simple project.

P.S. I'm still fairly new to Dynamic Data, so I hope you'll forgive me if some of my observables are less than optimal.

[bruzkovsky]

Hi, I have the same problem after upgrading to 11.5.35 from 10.5.43

[bruzkovsky]

@glennawatson @RLittlesII can I be of any help with fixing this?

This issue is pressing for us, because after updating to ReactiveUI 13.3.1 none of the Xamarin Forms bindings that go directly to the PageViewModel are working. This is because we use MvvmCross, and they require us to extend MvxViewModel, so we had to implement IReactiveObject in those view models ourselves. Now, it looks like you are requiring us to extend ReactiveObject too, which is not possible (no multiple inheritance in C#). So the only thing that works is using Rx bindings where there are no PropertyChanged events fired (interestingly, that works). But that's not a permanent solution, I think you'd agree.

When I look at the ReactiveObject implementation, it seems the following line is needed to enable propagation of PropertyChanged events - if I read the execution flow correctly.

ReactiveUI/src/ReactiveUI/ReactiveObject/ReactiveObject.cs

Line 45 in 92df299

this.SubscribePropertyChangedEvents();

This is the implementation of the extension method:

ReactiveUI/src/ReactiveUI/ReactiveObject/IReactiveObjectExtensions.cs

Lines 215 to 221 in 92df299

	internal static void SubscribePropertyChangedEvents<TSender>(this TSender reactiveObject)
	where TSender : IReactiveObject
	{
	var s = state.GetValue(reactiveObject, _ => (IExtensionState<IReactiveObject>)new ExtensionState<TSender>(reactiveObject));
	s.SubscribePropertyChangedEvents();
	}

Here is the implementation in IReactiveObjectExtensions.ExtensionState (called in line 220):

ReactiveUI/src/ReactiveUI/ReactiveObject/IReactiveObjectExtensions.cs

Line 365 in 92df299

public void SubscribePropertyChangedEvents() => _ = _propertyChanged.Value;

And finally, where the event should be propagated (line 375 checks if the Lazy was created with SubscribePropertyChangedEvents):

ReactiveUI/src/ReactiveUI/ReactiveObject/IReactiveObjectExtensions.cs

Lines 367 to 385 in 92df299

	public void RaisePropertyChanged(string propertyName)
	{
	if (!AreChangeNotificationsEnabled())
	{
	return;
	}
	var changed = new ReactivePropertyChangedEventArgs<TSender>(_sender, propertyName);
	if (_propertyChanged.IsValueCreated)
	{
	// Do not use NotifyObservable because event exceptions shouldn't be put in ThrownExceptions
	_propertyChanged.Value.OnNext(changed);
	}
	if (_changed.IsValueCreated)
	{
	NotifyObservable(_sender, changed, _changed.Value.subject);
	}
	}

I assume that simply calling SubscribePropertyChangedEvents would be enough, but it's internal. What was the idea behind that decision / can we make an easy fix and turn this method public?

[glennawatson]

Looking through it you should be able to make those public. The rationale was due to it accessing a internal state object but it should abstract it for you.

It's a breaking change but we have some changes to binding coming that are breaking so now would be a good time for you to make these changes.

[bruzkovsky]

Thanks for the quick reply, I'll fire up a PR!

[timothylcooke]

For the benefit of anybody who stumbles across this issue in the future, this was fixed in version Version 14.1.1. From the commit notes for commit 4f94c6b, we have:

This change enables consumers to implement IReactiveObject and get PropertyChanged and PropertyChanging notifications (which regressed in version 11.4.1) to get the notifications, one must call this.SubscribePropertyChangedEvents() or this.SubscribePropertyChangingEvents() on the IReactiveObject instance

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.