ReactiveProperty should inherit from ReadOnlyReactiveProperty

[ghost]

It would be useful if the ReactiveProperty class inherited from the ReadOnlyReactiveProperty class. That way, we can have a private ReactiveProperty and a public ReadOnlyReactiveProperty with the same object as their value. This effectively lets us have private setters and public getters, but with a reactive property.

Example

// In this class we have an IsDirty boolean value that can change. We want
// it to be possible to change it from inside the class,
// but impossible to change it from outside the class except by actually
// changing one of the other properties.

public class MountainViewModel
{
    private ReactiveProperty<bool> _isDirty = new ReactiveProperty<bool>();

    // This is cleaner than having a line initializing
    // IsDirty in the constructor
    public ReadOnlyReactiveProperty<bool> IsDirty => _isDirty;

    public ReactiveProperty<string> Name { get; } = new ReactiveProperty<string>();

    public ReactiveProperty<double> HeightInMeters { get; } = new ReactiveProperty<double>();

    public MountainViewModel()
    {
        Name.Subscribe(_ => _isDirty.Value = true);
        HeightInMeters.Subscribe(_ => _isDirty.Value = true);
    }
}

// Example usage of MountainViewModel:

var mvm = new MountainViewModel();

// This line causes a compile error because IsDirty is a
// ReadOnlyReactiveProperty. This is good! We don't want people
// to manually set the dirty bit.
mvm.IsDirty.Value = true;

// The only way to change IsDirty to true is to actually change
// one of the other properties on the view model. This also is good!
mvm.Name = "Kilimanjaro";
Debug.Assert(mvm.IsDirty.Value);
// The assert passes because we changed the mountain's name

As it currently stands, we would have to make the class slightly less readable to accomplish the same effect:

public class MountainViewModel
{
    private ReactiveProperty<bool> _isDirty = new ReactiveProperty<bool>();

    public ReadOnlyReactiveProperty<bool> IsDirty { get; }

    public ReactiveProperty<string> Name { get; } = new ReactiveProperty<string>();

    public ReactiveProperty<double> HeightInMeters { get; } = new ReactiveProperty<double>();

    public MountainViewModel()
    {
        // This line is necessary because ReadOnlyReactiveProperty
        // is not a base class of ReactiveProperty.
        IsDirty = _isDirty.ToReadOnlyReactiveProperty(_isDirty.Value);

        Name.Subscribe(_ => _isDirty.Value = true);
        HeightInMeters.Subscribe(_ => _isDirty.Value = true);
    }
}

Thank you for your time!

[ghost]

As with the other issue, if it's agreed that this is a change this project wants, then I would be happy to contribute the code and do a pull request.

[runceel]

It's very difficult.(ReactiveProperty should inherit from ReadOnlyReactiveProperty)

But I think that this case is follows.

static class MvvmHelpersExtensions
{
    public static IObservable<bool> ChangedAsObservable<T>(this ReactiveProperty<T> self, bool skipFirst = true)
    {
        var result = self.AsObservable();
        if (skipFirst)
        {
            result = result.Skip(1);
        }
        return result.Select(_ => true);
    }
}

class MountainViewModel
{
    private Subject<bool> ResetIsDirtySubject { get; } = new Subject<bool>();
    public ReactiveProperty<string> Name { get; } = new ReactiveProperty<string>();
    public ReactiveProperty<double> HeightInMeters { get; } = new ReactiveProperty<double>();
    public ReadOnlyReactiveProperty<bool> IsDirty { get; }

    public MountainViewModel()
    {
        this.IsDirty = Observable.Merge(
            this.Name.ChangedAsObservable(),
            this.HeightInMeters.ChangedAsObservable(),
            this.ResetIsDirtySubject)
            .ToReadOnlyReactiveProperty(false);
    }

    public void ResetDirty()
    {
        this.ResetIsDirtySubject.OnNext(false);
    }
}

Thank you.

[gayaK]

How is this solution?

    public interface IReadOnlyReactiveProperty
    {
        object Value { get; }
        IObservable<IEnumerable> ObserveErrorChanged { get; }
        IObservable<bool> ObserveHasErrors { get; }
    }

    public interface IReactiveProperty : IReadOnlyReactiveProperty
    {
        new object Value { get; set; }
    }

    public interface IReadOnlyReactiveProperty<T> : IReadOnlyReactiveProperty, IObservable<T>, IDisposable, INotifyPropertyChanged
    {
        new T Value { get; }
    }

    public interface IReactiveProperty<T> : IReactiveProperty, IReadOnlyReactiveProperty<T>, INotifyDataErrorInfo
    {
        new T Value { get; set; }
    }

    public class ReactiveProperty<T> : IReactiveProperty<T> {}
    public class ReadOnlyReactiveProperty<T> : IReadOnlyReactiveProperty<T> {}

[ghost]

Thanks for the quick replies!

I prefer a solution that doesn't use new to hide properties. Here's a possible alternative:

public interface IHasErrors
{
    IObservable<IEnumerable> ObserveErrorChanged { get; }
    IObservable<bool> ObserveHasErrors { get; }
}

public interface IReadOnlyReactiveProperty<T> : IObservable<T>, IDisposable, INotifyPropertyChanged,
    INotifyDataErrorInfo, IHasErrors
{
    T Value { get; }
}

public interface IReactiveProperty<T> : IObservable<T>, IDisposable, INotifyPropertyChanged, INotifyDataErrorInfo,
    IHasErrors
{
    T Value { get; set; }
    IReadOnlyReactiveProperty<T> AsReadOnly();
}

public class ReactiveProperty<T> : IReactiveProperty<T>, IReadOnlyReactiveProperty<T>
{
    public T Value { get; set; }

    public IDisposable Subscribe(IObserver<T> observer)
    {
        throw new NotImplementedException();
    }

    public void Dispose()
    {
        throw new NotImplementedException();
    }

    public event PropertyChangedEventHandler PropertyChanged;

    public IEnumerable GetErrors(string propertyName)
    {
        throw new NotImplementedException();
    }

    public bool HasErrors { get; }
    public event EventHandler<DataErrorsChangedEventArgs> ErrorsChanged;
    public IObservable<IEnumerable> ObserveErrorChanged { get; }
    public IObservable<bool> ObserveHasErrors { get; }

    public IReadOnlyReactiveProperty<T> AsReadOnly()
    {
        return this;
    }
}

public abstract class ReadOnlyReactiveProperty<T> : IReadOnlyReactiveProperty<T>
{
    public IDisposable Subscribe(IObserver<T> observer)
    {
        throw new NotImplementedException();
    }

    public void Dispose()
    {
        throw new NotImplementedException();
    }

    public event PropertyChangedEventHandler PropertyChanged;

    public IEnumerable GetErrors(string propertyName)
    {
        throw new NotImplementedException();
    }

    public bool HasErrors { get; }
    public event EventHandler<DataErrorsChangedEventArgs> ErrorsChanged;
    public IObservable<IEnumerable> ObserveErrorChanged { get; }
    public IObservable<bool> ObserveHasErrors { get; }
    public T Value { get; private set; }
}

The definition of AsReadOnly negates the need for IReadOnlyReactiveProperty to inherit from IReactiveProperty.

[runceel]

I agree to follows step.

public interface IHasErrors {}
public interface IReadOnlyReactiveProperty<T> : IHasErrors {}

public interface IReactiveProperty<T> : IHasErrors {}

But I shouldn't inherit IReactiveProperty from IReadOnlyReactiveProperty. Because this relation isn't "is a".

[gayaK]

I understood about this inheritance!

[ghost]

@runceel thank you for reading my code. What do you think about ReactiveProperty (the class, not the interface) inheriting IReadOnlyReactiveProperty? Is that acceptable?

I think it's a valid pattern to have code like this:

public interface IReadOnlyPerson
{
    string Name { get; }
}

public interface IReadWritePerson
{
    string Name { get; set; }
}

public class Person : IReadWritePerson, IReadOnlyPerson
{
    public string Name { get; set; }
}

That way, we can use interfaces to ensure that some code can change a person's name and other code can't. This is basically the pattern I wrote up in my previous comment.

[runceel]

I wrote code. Please review.

[ghost]

That looks perfect. Thank you so much!

[runceel]

I released ReactiveProperty v2.2.8. Please check.

[ghost]

    private readonly ReactiveProperty<int> _counter = new ReactiveProperty<int>();

    public IReadOnlyReactiveProperty<int> Counter => _counter;

It works perfectly! Thanks!