dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.02k stars 1.73k forks source link

[Spec] ViewModel base class #357

Open Redth opened 3 years ago

Redth commented 3 years ago

Provide concrete implementation of INotifyPropertyChanged

MAUI should provide an out of the box base, concrete implementation of the INotifyPropertyChanged pattern with a minimally useful and prescriptive but not overly opinionated set of convenience methods.

This is useful for building view models, and other base models which use the INPC pattern. We currently have nothing out of the box, and pretty much any sample we ship or app we see in the wild has to implement this themselves. Adding something out of the box is very low cost and provides a better experience for new developers.

Xamarin Community Toolkit provides the type ObservableObject which we could choose to use as is:

API

public abstract class ObservableObject: INotifyPropertyChanged
{
    protected virtual bool SetProperty<T>(
            ref T backingStore,
            T value,
            [CallerMemberName] string propertyName = "",
            Action? onChanging = null,
            Action? onChanged = null,
            Func<T, T, bool>? validateValue = null)
    {
        // ...
    }

    public event PropertyChangedEventHandler PropertyChanged;

    public void NotifyPropertyChanged([CallerMemberName]string propertyName)
        => /* ... */
}

Scenarios

C# Example

public class MyViewModel : Microsoft.Maui.Controls.ObservableObject
{
    string name;

    public string Name
    {
        get => name;
        set => Set(ref name, value);
    }
}

Difficulty : low

Questions?

dansiegel commented 3 years ago

A couple of things I would say...

It's often more than the ViewModel itself that should implement INotifyPropertyChanged. While your ViewModel itself would typically be attached to the Page, you may have a model that is bound directly to within the View (for instance in the CollectionView), and it would need to implement INPC...

Also I'm not sure what the plans are for the Community Toolkit with the release of MAUI but I would assume that will still be around, and there is already the ObservableObject in there that satisfies the requirement... I suppose if it's something that really is desired in MAUI directly then my personal recommendation is to just bring that implementation over.

Redth commented 3 years ago

Linking to that implementation: https://github.com/xamarin/XamarinCommunityToolkit/blob/main/src/CommunityToolkit/Xamarin.CommunityToolkit/ObjectModel/ObservableObject.shared.cs

I'm not opposed to this approach either. I'm just a bit hesitant of the 'Set' helpers. Is this too opinionated? Not enough? We obviously want to strike a good balance between usefully prescriptive and overly opinionated.

We may still also want a ViewModel at some point to help prescribe some convention around Hot Reload in the future - @StephaneDelcroix may have some additional thoughts around that.

dansiegel commented 3 years ago

I would tend to say those helpers are pretty standard. Prism, MvvmCross, MvvmLight, & ReactiveUI all use similar methods for example. While you might say they are all "Opinionated" MVVM is an opinionated design pattern and major frameworks that people use for it all seem to settle on a similar methodology.

Beyond an implementation of INPC if MAUI is providing an explicit ViewModel class for Hot Reload that is very opinionated and I would be concerned that may lead the development of the tooling down a road that makes it incompatible or at least less stable with apps using various frameworks.

Redth commented 3 years ago

I've updated the spec to propose this path. I agree, MVVM itself is opinionated. At the end of the day we want a better experience especially for new developers. Today even most of our samples contain some version of this type of implementation when it should just be baked in.

The hot reload context is something that's still in design, and yes ideally we would not require any sort of base class here, we're just exploring how to guide developers into the best experience for this scenario.

dansiegel commented 3 years ago

I suggest if something really does need to be done for a better hot reload experience, let's ensure it's provided via interface... that way even if there is ultimately some base implementation out of the box, it doesn't leave anyone out because they're doing something else :)

AmrAlSayed0 commented 3 years ago

Since other classes and methods like this one might be included, would it be better to separate those in a separate package? Maui.Mvvm maybe? I would actually like it if ALL of the other MAUI controls that inherit from BindableObject (and in MAUI also inherit from IView/IFrameworkElement) be into a separate package. And maybe controls implemented for MVU that inherit from some other object (I'm not very familiar with MVU 😅) into an Maui.Mvu package. Opinionated patterns in their own opinionated packages.

Redth commented 3 years ago

MAUI is not going to be a traditional nuget package, rather an optional workload component for the dotnet SDK which is a bit different. There is a separation of MAUI core things and BindableObject is not considered a part of that, but rather part of MAUI Controls (and yes you will be able to tell your project you only want Core and not Controls referenced). This type of base class would exist in Controls, not core, as INPC is more closely tied to XAML/MVVM/DataBinding concepts that MAUI Controls uses. Things like Comet/MVU don't necessarily require or make use of INPC so this doesn't make sense in that layer.

codeaphex commented 2 years ago

Hi, is there something stopping you from using the approach from the WindowsCommunityToolkit which would align the api and features. Shouldn't it run without modifications since it has no deps on .NET 5+ https://www.nuget.org/packages/Microsoft.Toolkit.Mvvm? Their ObservableObject Implementation can be found in the 7.1.0 branch: https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/rel/7.1.0/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs

ghost commented 2 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.