CommunityToolkit / dotnet

.NET Community Toolkit is a collection of helpers and APIs that work for all .NET developers and are agnostic of any specific UI platform. The toolkit is maintained and published by Microsoft, and part of the .NET Foundation.
https://docs.microsoft.com/dotnet/communitytoolkit/?WT.mc_id=dotnet-0000-bramin
Other
3.08k stars 300 forks source link

Usage of ObservableValidator in Windows Template Studio #26

Open mvegaca opened 4 years ago

mvegaca commented 4 years ago

Hi

We are working on a Mockup for WinUI3 Templates in Windows Template Studio that uses the MVVMToolkit libraries. You can find the code at Mockup-WinUI3.

As part of this mockup we're looking at a FormPage that allows users to add a new order to an in-memory SampleOrder Collection. We want to validate the user input and give visual feedback when the user edits any of the fields and when clicking submit.

We initially implemented a custom ValidationObservableRecipient and want to switch to the MVVMToolkit ObservableValidator now. You can find a sample implementation of the old page in FormPage and the new implementation in the FormWCTPage.

We are having some issues in the new FormWCTPage:

Are we missing something or are those scenarios not yet supported?

Screenshots of Form Page

image

Clicking directly on submit button

image

Thank you so much ;)

ghost commented 4 years ago

Hello mvegaca, thank you for your interest in Windows Community Toolkit!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible.. Other community members may also answer the question and provide feedback 🙌

Kyaa-dost commented 4 years ago

@mvegaca Thanks for highlighting this as it does seem a very peculiar behavior because ideally it should work once the requirements are fulfilled or even reset.

@Sergio0694 any insight on this? 🤔

Sergio0694 commented 4 years ago

With respect to these two:

  • We did not find a way to validate and show visual feedback on the fields to the user when clicking submit button (for example: leave all fields empty and click submit)
  • We did not find a way to reset the fields to the default values after submitting

They're not currently supported, that is correct. Those features would be extra additions on top of the standard INotifyDataErrorInfo interface. Not saying they'd be bad, just explaining why they weren't added initially 🙂

As for the issue mentioned here:

  • The OrderId does not always remove the error message when a correct value is entered.

Do you have a minimal repro? Also, have you checked whether the same happens when eg. using UWP WinUI 2.x, or WPF? Just wondering if that might be indirectly caused by WinUI 3.x in particular, or at least if you have a smaller repro that'd be easier to investigate, and also so that I could add a new unit test replicating that issue specifically, if we pinpoint what's happening.

michael-hawker commented 4 years ago

@mvegaca thanks for filing the issue and taking this class for a spin! QQ: Is this for WinUI 3 Desktop or WinUI 3 UWP?

FYI @stevenbrix as well.

mvegaca commented 4 years ago

Hi

We're working with WinUI3 Desktop (Packaged) Preview 2.

I've created a quick repro App to show you the range validation behavior.

image

image

About clicking directly on submit, would be necessary to make the ValidateProperty public or enable ObservableValidator to raise the ErrorsChanged event (like ObservableObject does with OnPropertyChanged method).

This also would be necessary to add a CleanAllErrors method to set a default configuration after submitting the order.

We want to create the template giving a straight forward scenario to developers, a form to submit a new order that user can see on the MasterDetail page before adding it.

sibille commented 4 years ago

They're not currently supported, that is correct. Those features would be extra additions on top of the standard INotifyDataErrorInfo interface. Not saying they'd be bad, just explaining why they weren't added initially 🙂

Are there plans to add this functionality to the MVVMToolkit ObservableValidator or to allow to extend it's functionality in the ViewModel?

Sergio0694 commented 4 years ago

Thanks @mvegaca for the small repro, I'll take a look! Also curious to see if I can repro that on UWP, to check whether that issue is specifically related to the type being used from WinUI 3, or just a bug in the type on its own. Will update when I know more 👍

Are there plans to add this functionality to the MVVMToolkit ObservableValidator or to allow to extend it's functionality in the ViewModel?

We haven't looked at adding them yet for the first public release. I know @stevenbrix mentioned the WinUI team has looked into offering some controls specifically to support the interface, so it'd be interesting to potentially coordinate the work in that direction so that our ObservableValidator can expose the necessary APIs to support those new controls. As for other additions unrelated to that, we're currently still thinking about how to approach that. From what I understand, basically, the request is for:

Is that generally what you had in mind? 🙂

cc. @michael-hawker

mvegaca commented 4 years ago

Exactly, those two features are what we need for our WinUI3 Form Template. Our WinUI3 Templates are still in preview. For now, we are going to generate a custom ObservableValidator class that supports those two features. Once the toolkit supports those methods we will replace it.

Sergio0694 commented 4 years ago

Tried to run the sample, and that didn't work for me either. Attached the local MVVM Toolkit package so that I could step into it, looks like the call to Validator.TryValidate throws a C++ exception somewhere within WinUI I guess, maybe that's why the UI isn't updated correctly? The code runs fine after that, so the exception is handled fine, but the UI doesn't refresh, as you pointed out.

Tried to run the same exact code in a UWP app, and while the UI doesn't support that, I could verify that the ObservableValidator class itself is indeed working as expected, at least it would seem so by looking at the output:

image

stevenbrix commented 4 years ago

Thanks for all the great work here @mvegaca! A lot of these issues are some of the same things I came across as well, and something that needs to be spec'd out so we can have a default experience that "just works".

Per what @Sergio0694 said, these services would need to be added on top of the existing support, however there are a few known issues with what currently exists mainly, this one:

The OrderId does not always remove the error message when a correct value is entered.

Depending on how you setup your x:Bind, it will only either listen to LostFocus or PropertyChanged events. There is work needed to be done so that it defaults to LostFocus, but then switches to PropertyChanged once the control is invalid.

We did not find a way to validate and show visual feedback on the fields to the user when clicking submit button (for example: leave all fields empty and click submit)

Regarding this one. Early designs/prototypes, had the idea of a ValidationCommand, which you could share across all of your controls that implement the IInputValidationControl interface. The command would then know when each of it's controls was valid/invalid (including Required controls that hadn't been filled out) and would raise the CanExecuteChanged event. This way you could get the behavior you want, without tying to using a specific control.

sibille commented 4 years ago

Thanks for the insights @stevenbrix. Is there an issue on the WinUI3 repo that we can watch for the upcoming changes on InputValidation?

stevenbrix commented 4 years ago

@sibille, you can follow this issue here: https://github.com/microsoft/microsoft-ui-xaml/issues/179

Just to set expectations, there hasn't been much activity, and the amount of attention we'll be able to give before the 3.0 release may be minimal. There is a base set of functionality in the platform currently, and it's likely that is what we'll ship at 3.0.

michael-hawker commented 4 years ago

Yeah, @Sergio0694 and I talked to @stevenbrix this morning.

@sibille he's good with our two teams working together between your templates in WTS and the Toolkit (MVVM and UWP/WinUI 3 sides) in building a working solution (hopefully as close to the interface/platform support as possible, but we can include extra bits if needed). It may be interesting to use WPF as a possible litmus test as well since it has some support for that as well. Is this new template going to work for both WinUI 3 and WPF or just WinUI 3?

Then we can use that to help guide the future direction of the platform later next year when they get back to this space.


Also FYI @sibille and @mvegaca, we've been talking about renaming our CommunityToolkit/WindowsCommunityToolkit#3381 PR to be FormPanel and investigating if we can help explore https://github.com/microsoft/microsoft-ui-xaml/issues/82 more. Could be an interesting thing to explore in 2021 before WinUI 3 ships for your template. Would love to know your thoughts. FYI @vgromfeld.

sibille commented 4 years ago

@sibille he's good with our two teams working together between your templates in WTS and the Toolkit (MVVM and UWP/WinUI 3 sides) in building a working solution (hopefully as close to the interface/platform support as possible, but we can include extra bits if needed).

@michael-hawker, sounds like a great plan! I'll open a tracking issue on our side,

It may be interesting to use WPF as a possible litmus test as well since it has some support for that as well. Is this new template going to work for both WinUI 3 and WPF or just WinUI 3?

We're currently looking at a FormPage for WinUI3, but will sync with my team to see if we can include WPF too (FYI @jeanroca).

Also FYI @sibille and @mvegaca, we've been talking about renaming our CommunityToolkit/WindowsCommunityToolkit#3381 PR to be FormPanel and investigating if we can help explore microsoft/microsoft-ui-xaml#82 more. Could be an interesting thing to explore in 2021 before WinUI 3 ships for your template. Would love to know your thoughts. FYI @vgromfeld.

We'll also have a look at that one to see how it fits in the templates, thanks for the info!

michael-hawker commented 4 years ago

@jamesmcroft FYI for you too in case your UWP validators hook-up the INotifyDataError support for UWP too, then these should be good test cases to compare against for the future.

jamesmcroft commented 4 years ago

@michael-hawker I've been looking into this and I think the implementation I've created for validators is slightly different.

I'm factoring some time in this weekend to have a play with the latest changes for the ObservableValidator and seeing how it might work with what I've built.

Noemata commented 3 years ago

Would much appreciate seeing samples of these validation mechanisms working with UWP, since it's in production. WinUI is months away and having all this spin on the future at the expense of the present needs to be rationalized.

@Sergio0694 , could you please push up a small validation sample for UWP somewhere using your new MVVM bits, thanks.

Sergio0694 commented 3 years ago

Hey @mvegaca and @sibille, all those requested methods are now implemented in ObservableValidator 😄 Here's a comment with the full API brakdown: https://github.com/windows-toolkit/WindowsCommunityToolkit/issues/3428#issuecomment-765688514.

TLDR: ObservableValidator now exposes ValidateAllProperties to automatically validate the whole entity, ValidateProperty to manually validate individual properties, and ClearErrors to clear errors for specific properties or for the whole entity.

If you try it out, let me know how it goes and if ObservableValidator can now properly support your use case scenario! 🙂

mrlacey commented 3 years ago

Tried to run the sample, and that didn't work for me either. Attached the local MVVM Toolkit package so that I could step into it, looks like the call to Validator.TryValidate throws a C++ exception somewhere within WinUI I guess, maybe that's why the UI isn't updated correctly? The code runs fine after that, so the exception is handled fine, but the UI doesn't refresh, as you pointed out.

I know this is going back a bit but was a separate WinUI bug filed for this?

mvegaca commented 3 years ago

Hello @Sergio0694 Thanks for adding these new features to ObservableValidator. It looks that works with some bugs, there are many times in wich an error is not cleaned when the property is set to a valid value. Here is a branch to try this with the mockup that I am working on. https://github.com/mvegaca/WindowsTemplateStudio/tree/Mockup-WinUI3-ObservableValidator

Try with this.

image

image

I hope this helps.

Sergio0694 commented 3 years ago

Thanks for the repro, I'll go take a look! 😄

Sergio0694 commented 3 years ago

@mvegaca The sample seems to work absolutely fine for me, I couldn't repro the issue on my end 🤔

image

I tried twice with the debugger attached, and also by just running the app on its own. I tried quite a few times with all sorts of values, and also to insert a valid text, then deleting, then typing again, etc. All the form cells seem to update correctly for me.

I'm on VS 2019 16.9 Preview 3, .NET 5 SDK 5.0.200-preview.20614.14, and with the updated WinUI 3 VSIX package. Also, I'm on Windows 10 Pro x64 build 19041.782.

Do you consistently repro the issue? Are there any noticeable differences in your setup?

EDIT: I did change the base class to be the current ObservableValidator class from the MVVM Toolkit Previe 5, same behavior.

mvegaca commented 3 years ago

@Sergio0694 thanks for looking into it.

Yes, we can consistently reproduce it on various machines on our team using VS version: 16.9.0 Preview 3.0 and Windows version: 19042.746 and 19041.782.

The Issue is on Form WCT Page. The view model of this page is already extending from the ObservableValidator base class. The Issue is only related to UI feedback, validation is working fine as the item is saved and added to the sample companies collection that is shown in the ContentGrid Page.

Here is a video with repro steps to better explain our test case. https://1drv.ms/v/s!Av0n-YhDRN_3vctFnzwib0OZ-S01hg?e=OCOVN1

Sergio0694 commented 3 years ago

Was looking for a facepalm emoji to react to your message but unfortunately GitHub doesn't have one 😄 I was indeed using the wrong page, sorry about that! Tried the Form WCT page and I can finally repro the issue on my end now, great! I'll add a local reference to the MVVM Toolkit and see if I can figure out what's going on and fix it, thanks for the report! Will post an update as soon as I have any news on this 🙂

Sergio0694 commented 3 years ago

Alright, I've investigated this a bit and I'm not entirely sure this is an actual bug in ObservableValidator. All the internal logic for ObservableValidator itself works fine as far as I can tell and the state is updaated correctly. I'm looking at the generated .g.cs backing file and it seems to me there might be something off here.

Here's my breakdown.

The registration of INotifyDataErrorInfo itself is done correctly right next to INotifyPropertyChanged:

public void UpdateChildListeners_ViewModel(global::WinUIDesktopApp.ViewModels.FormWCTViewModel obj)
{
    if (obj != cache_ViewModel)
    {
        // ...
        if (obj != null)
        {
            cache_ViewModel = obj;
            ((global::System.ComponentModel.INotifyPropertyChanged)obj).PropertyChanged += PropertyChanged_ViewModel;
            ((global::System.ComponentModel.INotifyDataErrorInfo)cache_ViewModel).ErrorsChanged += ErrorsChanged_ViewModel;
        }
    }
}

Now, consider the situation where the bug happens, so when we have a viewmodel with just one remaining field that is then updated, validated again and then marked as valid. The viewmodel now has 0 errors, and it raises ErrorsChanged to notify the UI of this change in errors. We expect the view to clear the errors in that control bound to this property, which is not happening. This is the code that is invoked when ErrorsChanged is raised:

public void ErrorsChanged_ViewModel(object sender, global::System.ComponentModel.DataErrorsChangedEventArgs e) 
{
    FormWCTPage_obj1_Bindings bindings = TryGetBindingObject();
    if (bindings != null)
    {
        string propName = e.PropertyName;
        if (global::System.String.IsNullOrEmpty(propName))
        {
            bindings.UpdateErrors_ViewModel((global::System.ComponentModel.INotifyDataErrorInfo)sender, "OrderID");
            // All other properties here...
        }
        else
        {
            bindings.UpdateErrors_ViewModel((global::System.ComponentModel.INotifyDataErrorInfo)sender, propName);
        }
    }
}

In this case propName is "ShipsTo", so the second branch is taken - we can move on to UpdateErrors_ViewModel:

private void UpdateErrors_ViewModel(global::System.ComponentModel.INotifyDataErrorInfo sender, string propertyName)
{
    if (this.initialized)
    {
        switch (propertyName)
        {
            // All other properties here...
            case "ShipTo":
            {
                UpdateErrors_(obj6, sender, "ShipTo");
                break;
            }
        }
    }
}

Just a wrapper to actually switch the bound property name and the respecting control, this is fine. Now on to the possibly faulty method:

private void UpdateErrors_(global::Microsoft.UI.Xaml.Controls.Control control, global::System.ComponentModel.INotifyDataErrorInfo sender, string propertyName)
{
    if (sender.HasErrors)
    {
        UpdateInputValidationErrors(control, sender.GetErrors(propertyName));
    }
}

Ha, that doesn't look right! sender.HasErrors is false now, because we just validated the last incorrect property. So this method just returns and the UI is not updated at all, which explains why we're seeing that behavior in the last field being modified.

cc. @stevenbrix am I correct here in thinking this might just be a bug in the XAML compiler codegen? It seems to me that there should at least be another branch in UpdateErrors_ that just clears all the errors in the target control without doing other checks in case the linked viewmodel has no errors left anymore, right? 🤔

EDIT: also figured out why the other implementation is working fine, it seems to me it's just a coincident, really. Basically, the errors are cleared before the property is updated, which makes the .g.cs file execute until that UpdateErrors_ bit. Then HasErrors returns true due to a bug in the implementation of that INotifyDataErrorInfo class. Consider this bit:

image

Note: this code above is not from the ObservableValidator classs in the MVVM Toolkit, it's the temporary local implementation in the sample project that was shared in this thread. The one in the MVVM Toolkit does not have this issue.

You can see how HasErrors returns true because the dictionary is not empty, even though all the various list of errors are actually empty - the model has effectively no errors at this time, and HasErrors is returning an incorrect value. But, this causes the XAML generate code to actually execute the logic to update the UI, which fetches the errors for the property (which is an empty list), and consequently the method after that just updates the UI to display no errors. But really this is just a crazy coinvidence at the end of the day 😄

stevenbrix commented 3 years ago

@Sergio0694 I'm trying to get caught up. It sounds like this is a bug in WCT?

michael-hawker commented 3 years ago

@Sergio0694 I'm trying to get caught up. It sounds like this is a bug in WCT?

Nope, @stevenbrix we filed an issue on WinUI here, looks like an issue with the generated binding support for INotifyDataErrorInfo which doesn't signal the UI to update if the validation passes, it's only signaling when there's an error present. Seems like a simple logic update for the code generator?

thomasrea0113 commented 2 years ago

So I encountered a similar issue, and worked around it with some reflection. In a subclass of ObservableValidator, I added this method:

    private const BindingFlags _protectedBindingFlags = BindingFlags.Instance | BindingFlags.NonPublic;

    protected static readonly MethodInfo ValidateMethod =
        typeof(ViewModelBaseWrapper).GetMethod(nameof(ValidateAllProperties), _protectedBindingFlags)
            ?? throw new NullReferenceException();

    public void Validate() => ValidateMethod.Invoke(this, Array.Empty<object>());

which will expose the ValidateAllProperties method so that commands can initiate validation. My use case was similar - I didn't want an empty form to show a bunch of errors, and only wanted validation to run after submit was clicked. You can then create a command like so:

new RelayCommand<ViewModel>(
        options =>
        {
            if (options == null) throw new NullReferenceException();

            options.Validate();

            if (!options.HasErrors)
                EAMClients.Add(options.Key!, options);
        });