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

First pass at observing nested property changes #156

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

michaelstonis
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

neuecc commented Mar 3, 2024

  • I want to know the difference between propertyChanger.InnerPropertyChanged.ObservePropertyChanged(x => x.Value) and its use cases.
  • The behavior of Switch is a bit heavy; this is because it repeats the same content subscription, so I don't think an implementation like Switch is appropriate.

@michaelstonis
Copy link
Contributor Author

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
Copy link
Member

neuecc commented Mar 4, 2024

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.

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.
The ObservePropertyChanged and ObservePropertyChanging methods now support multiple property selectors, allowing for nested observation of property changes. Corresponding unit tests have also been added.
@michaelstonis michaelstonis force-pushed the feature/nested-prop-obs branch from 558affc to 996bc83 Compare March 4, 2024 16:40
These are dependent on `PropertyChanged` until the end of the chain where we will switch to `PropertyChanging`, so these have been updated to leverage the `ObservePropertyChanged` methods.
@michaelstonis
Copy link
Contributor Author

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

  • For nested properties, I check if the value coming in is null and return an empty observable to bypass any additional object creation.
  • The ObservePropertyChanging methods that use nested properties utilize the ObservePropertyChanged extension methods with the pushCurrentValueOnSubscribe set to true for all but the last property in the chain. This is because we need to know when the parent properties have been updated and when a new value is available. The last property in the chain will observe the PropertyChanging event and honor the value of pushCurrentValueOnSubscribe set by the user.

@michaelstonis michaelstonis marked this pull request as ready for review March 4, 2024 19:11
@neuecc
Copy link
Member

neuecc commented Mar 5, 2024

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

@neuecc neuecc merged commit 7aeb80b into Cysharp:main Mar 5, 2024
1 check passed
@hsytkm
Copy link
Contributor

hsytkm commented Mar 5, 2024

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
Copy link
Member

neuecc commented Mar 5, 2024

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
Copy link
Contributor Author

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

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

Successfully merging this pull request may close these issues.

3 participants