Cysharp / R3

The new future of dotnet/reactive and UniRx.
MIT License
1.72k stars 70 forks source link

First pass at observing nested property changes #156

Closed michaelstonis closed 4 months ago

michaelstonis commented 4 months ago

Added a new method to observe changes in nested properties of INotifyPropertyChanged objects. This allows for more granular observation of property changes, particularly useful when dealing with complex objects. Also updated the test suite to cover this new functionality.

michaelstonis commented 4 months ago

This is just a WIP of how we can do inner property changes. I am not sure if this setup would be acceptable enough or if we would want to change the overall implementation. I would be glad to get some feedback on it. If this is appropriate, I can expand it to allow three levels of nesting and will implement the INotifyPropertyChanging variant.

neuecc commented 4 months ago
michaelstonis commented 4 months ago

The need for this is common when view models have late-bound and replaced properties. In the case of propertyChanger.InnerPropertyChanged.ObservePropertyChanged(x => x.Value) that works when the InnerPropertyChanged object is already set and does not change.

For example, if there is a user interface that has a main view model with a selection function, you can end up needing to listen to that property and respond when it changes. Below is an example of a view model that would be used to display a list of players and the current selected player. When a player is selected, we will want to show that player's score which will dynamically update.

MainViewModel : INotifyPropertyChanged
 - Players: Items<Player>
 - SelectedPlayer : Player?

Player : INotifyPropertyChanged
 - Score: int

In this example, viewModel.SelectedPlayer.ObservePropertyChanged(x => x.Score) would have potential issues. If SelectedPlayer is null, you can't start listening to those value changes. That means you need to intercept the property setter or similar to be able to listen to that property being set to establish ObservePropertyChanged and listen for inner property changes. With a setup like viewModel.ObservePropertyChanged(x => SelectedPlayer, x => x.Score) you can have a single observable that would allow for chained change notifications to SelectedPlayer and Score.

Let me know if that helps explain the use case for this. It is usual that a depth of 2 is sufficient for these kinds of listeners. A depth of 3 (e.g. vm.ObservePropertyChanged(x => x.Value1, x => x.Value2, x => Value3)) would likely cover nearly any use case.

neuecc commented 4 months ago

Thank you, that example was very clear, and I can see how it would indeed be beneficial! Indeed, it seems like Switch is appropriate. I can't think of anything else besides what you've suggested in terms of the API, so it looks good as it is.

michaelstonis commented 4 months ago

This PR contains extensions for ObservePropertyChanged and ObservePropertyChanging that allow for up to two levels of nested property change detection (i.e. x.PropertyOne.PropertyTwo.PropertyThree). I feel like this amount of depth will cover a vast majority of the use cases for nested properties.

Below are a few implementation notes

neuecc commented 4 months ago

Thank you, I think it's very well considered! I'll merge and release it!

hsytkm commented 4 months ago

I tried this feature, which was added in Ver1.1.6, but it seems that I cannot resolve the nullable warning. If SelectedPlayer is nullable, shouldn't TProperty1 also be nullable?

If there is a solution, I would like to know. Here is the code I tried:

#nullable enable
using System.ComponentModel;
using R3;

MainViewModel vm = new();

vm.ObservePropertyChanged(x => x.SelectedPlayer, x => x.Score)
    .Subscribe(Console.WriteLine);

class MainViewModel : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler? PropertyChanged;
    public IReadOnlyList<Player> Players { get; } = [new Player(1), new Player(2)];
    public Player? SelectedPlayer { get; set; }
}

class Player(int score) : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler? PropertyChanged;
    public int Score { get; } = score;
}

image

neuecc commented 4 months ago

If only to remove the warning, ObservePropertyChanged(x => x.SelectedPlayer!, x => x.Score) seems to be a good way to do it.

However, this looks like it would be better to improve the operator definition. How about a change to Func<T, TProperty1?> propertySelector1? It may be more natural since the subsequent Value is originally handled with the expectation that null may come. @michaelstonis

michaelstonis commented 4 months ago

I think this is a good idea and I flip-flopped on this a little when writing it. I have a new PR #158 which provides better support for nullable properties and eliminates the warnings. @neuecc @hsytkm