The new MVVM-Toolkit got some new bugs with the latest updated.

[timunie]

Describe the bug

I am not sure if it is one bug or two. For that reason I will open only one ticket by today. If you think I should divide it I will open a second issue.

Steps to Reproduce

Steps to reproduce the behavior:

Issue 1:

1. Create a new Project and use ObservableValidator in your ViewModel
2. Add two different Properties which are required. Make use of The field {0} is required and let the validator replace {0} by the DisplayName.
3. Notice that both fields get the same message

Issue 2;

1. Create a new Project and use ObservableValidator in your ViewModel
2. Do a class-level valididation
3. The user will get every issue twice.

Expected behavior

The messages should not be reused and every field should get its own message as it was in preview4. Moreover if you perform a class-level-validation every message should be displayed only one time.

Screenshots

Issue 1

• Field 1:
  

• Field 2:
  

Issue 2:

Environment

NuGet Package(s): 

Id                                  Versions                                 ProjectName                                                                                                                                                                                                                        
--                                  --------                                 -----------                                                                                                                                                                                                                        
MahApps.Metro                       {2.4.3}                                  MvvmToolkitValidationSample                                                                                                                                                                                                        
Microsoft.Toolkit.Mvvm              {7.0.0-build.848}                        MvvmToolkitValidationSample     

Package Version(s): 

Windows 10 Build Number:
- [ ] Fall Creators Update (16299)
- [ ] April 2018 Update (17134)
- [ ] October 2018 Update (17763)
- [ ] May 2019 Update (18362)
- [ ] May 2020 Update (19042)
- [x] 20H2 (19042.804)
- [ ] Insider Build (build number: )

App min and target version:
- [ ] Fall Creators Update (16299)
- [ ] April 2018 Update (17134)
- [ ] October 2018 Update (17763)
- [ ] May 2019 Update (18362)
- [ ] May 2020 Update (19041)
- [ ] Insider Build (xxxxx)
- [X] WPF 

Device form factor:
- [X] Desktop
- [ ] Xbox
- [ ] Surface Hub
- [ ] IoT

Visual Studio 
- [ ] 2017 (version: )
- [X] 2019 (version: 16.8.5) 
- [ ] 2019 Preview (version: )

Additional context

The shown sample is available here: https://github.com/timunie/MvvmToolkitValidationSample

/cc @Sergio0694

[ghost]

Hello timunie, thank you for opening an issue with us!

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 look into the issue and provide feedback 🙌

[Sergio0694]

Hey @timunie, thanks for the report! This is interesting, I'll take a look!
Thank you for providing a sample, that helps! 👍

[Sergio0694]

Small update, I've identified and fixed the first issue. Here's the breakdown of what was causing it, for future reference.
In #3562, I've added the ability to pass an external ValidationContext to ObservableValidator, which was requested in #3729. That caused the first issue due to the fact that the ValidationContext.DisplayName property is not re-evaluated when MemberName is modified (not really sure why that is case, can't think of a reason why you'd want this). This caused ValidateProperty to pass the updated property name but with the old display name instead, which was displayed in the UI.

The fix is to just load and cache the DisplayAttribute instances in ObservableValidator and to replicate the logic to retrieve the display name that ValidationContext has, with the difference that doing this ourselves lets us reuse the same ValidationContext as requested in the other issue (and also to skip some extra allocations while we're at it).

Still investigating the other issue...

[Sergio0694]

Hey @timunie, I'm a bit confused with respect to issue 2 and need some more info to investigate this 🙂

I can repro that with Preview 4 as well, it doesn't seem to be something new in the new version.
And I'm not even sure it's actually a bug in the MVVM Toolkit per se, as things look ok from what I can see 🤔

This is the internal state of the viewmodel right after the call to ValidateAllProperties. You can see that there are (as expected) a total of 2 errors, one for each of those two properties, and yet the TextBox displays each one two times in the popup.

[timunie]

Hi @Sergio0694 ,

The second one is indeed very strange. It still counts two times and if I change the error-template it also tells me two errors where I see in the source only one. Maybe this is a bug in WPF itself.

Let me ask @punker76 if he knows this issue in wpf.

Anyway thank you for your support and have a nice weekend.

Tim

[timunie]

Maybe related: https://www.telerik.com/forums/validation-tooltip-showing-errors-twice#9NYisgnYeE63kagYquLmMg

[Sergio0694]

Yeah that issue is weird, curious to see if anyone else has experienced that on WPF with another INDEI implementation.
Anyway since that repros with the MVVM Toolkit Preview 4 at least we know it's not a regression with the latest build 🙂
And the new PR did fix the first error, so at least that one's covered, thank you for the report!

[timunie]

My old implementation has the same issue. I just never did classlevel validation before so I never noticed that.

If you want to close the issue you are free to do so.

[Sergio0694]

Ah, thanks for the additional info!

No need to close this issue, it will automatically be closed when #3764 is merged, since that fixes issue 1 here 😄

[timunie]

Hi @Sergio0694

I think I found a solution for issue 2. But unfortunately it cannot be implemented in my App 😒 . I have either to use my own MVVM library again or you need to help me another time 🙏 .

What I found out is, that WPF does call GetErrors two times.

1. It passes string.Empty as a parameter
2. It passes null as a parameter

Since the check is string.IsNullOrEmpty(propertyName) it gets executed twice. So I modified the source to just call it on string.Empty and not for null.

/// <inheritdoc cref="INotifyDataErrorInfo.GetErrors(string)"/>
[Pure]
public IEnumerable<ValidationResult> GetErrors(string? propertyName = null)
{
    // Get entity-level errors when the target property is null or empty
    if (propertyName == string.Empty) // modified this line
    {
        // Local function to gather all the entity-level errors
        [Pure]
        [MethodImpl(MethodImplOptions.NoInlining)]
        IEnumerable<ValidationResult> GetAllErrors()
        {
            return this.errors.Values.SelectMany(static errors => errors);
        }

        return GetAllErrors();
    }

    // Property-level errors, if any
    if (propertyName is not null && this.errors.TryGetValue(propertyName!, out List<ValidationResult>? errors)) // we need to perform additional null-check here.
    {
        return errors;
    }

    // The INotifyDataErrorInfo.GetErrors method doesn't specify exactly what to
    // return when the input property name is invalid, but given that the return
    // type is marked as a non-nullable reference type, here we're returning an
    // empty array to respect the contract. This also matches the behavior of
    // this method whenever errors for a valid properties are retrieved.
    return Array.Empty<ValidationResult>();
}

Here is the updated example: https://github.com/timunie/MvvmToolkitValidationSample/tree/Issue2/Solution-1

How would you like to track this issue?

• Leave issue 2 unattended as it is just related to WPF and different for WinUI (I have no idea about WinUI yet)
• I could open another ticket as it is a separate issue I think.
• I can also send a PR if you want me to

Happy coding
Tim

[Sergio0694]

What I found out is, that WPF does call GetErrors two times.

• It passes string.Empty as a parameter
• It passes null as a parameter

*sigh*
But why 😭

Thanks @timunie for looking into this and confirming the issue is in fact on the WPF side!
My main problem with the workaround here is that it explictly goes against the INotifyDataErrorInfo specification, and since the MVVM Toolkit is built to be platform agnostic as one of its core principles, I don't want to have a specific bug in a UI framework in particular (not even just some detail, but an actual incorrect implementation) leak into the Toolkit 😕

If you look at the official MS docs for the INotifyDataErrorInfo.GetErrors(string) method (here), you can see this:

"The name of the property to retrieve validation errors for; or null or Empty, to retrieve entity-level errors."

I highlighted in bold the part that interests us here. The spec explicitly says that both null and an empty string should be accepted values that result in the entity-level errors being returned, while the proposed workaround essentially breaks the spec when null is passed. That could cause all sorts of other issues on other frameworks (eg. Xamarin, or Blazor, or whatever), if those instead just rely on that spec and pass null to get all errors. Not really sure why WPF is doing this (maybe just an oversight, maybe a workaround for other implementations only handling either null or an empty string, not really sure), but I'm not sure that inserting a workaround on our own specifically for that in the MVVM Toolkit is the right solution here.

I absolutely recommend opening an issue for this in the WPF repo, or just comment in an existing one if there's one already. Please include a link to this issue if you do, so we can track that as well or just refer to it should this come up again in the future.

As for WinUI (and UWP XAML), from what I can tell this is properly handled already since this specific issue hasn't come up before (not that I can remember), but cc. @stevenbrix just in case this is interesting to you and/or if you want to confirm this bug is in fact not present on either frameworks (or at least WinUI 3 in particular). 😄

FYI @michael-hawker

[timunie]

Hi All,

I filed a bug to the wpf-team: dotnet/wpf#4201

---

Just in case someone is interested
I found another workaround for issue 2. This can be done on users side, so no adjuments to the MVVM-Framwork needed.

1. Add a read-only property to your ViewModel which returns the class-level ValidationResults

   public IEnumerable<ValidationResult> AllErrors => GetErrors(null);

2. In your constructor add a handler to the ErrorsChanged-event

   public ViewModel()
   {
       ValidateAllProperties();
       ErrorsChanged += (_, __) => OnPropertyChanged(nameof(AllErrors));
   }

3. Create and use your own ValidationTemplate like the one below:

    <Validation.ErrorTemplate>
        <ControlTemplate>
            <DockPanel>
                <ItemsControl DockPanel.Dock="Bottom"
                              ItemsSource="{Binding ElementName=ErrorAdorner, Path=AdornedElement.DataContext.AllErrors}">
                    <ItemsControl.ItemTemplate>
                        <DataTemplate>
                            <StackPanel Orientation="Horizontal">
                                <Grid Width="12" Height="12">
                                    <Ellipse Width="12"
                                             Height="12"
                                             HorizontalAlignment="Center"
                                             VerticalAlignment="Center"
                                             Fill="Red" />
                                    <TextBlock Text="X"
                                             HorizontalAlignment="Center"
                                             VerticalAlignment="Center"
                                             FontSize="8"
                                             FontWeight="Heavy"
                                             Foreground="White"
                                             TextAlignment="Center" />
                                </Grid>
                                <TextBlock Text="{Binding ErrorMessage}"
                                           Margin="2,0,0,0"
                                           Foreground="Red" />
                            </StackPanel>
                        </DataTemplate>
                    </ItemsControl.ItemTemplate>
                </ItemsControl>
   
                <AdornedElementPlaceholder x:Name="ErrorAdorner" />
            </DockPanel>
        </ControlTemplate>
    </Validation.ErrorTemplate>

Source: https://github.com/timunie/MvvmToolkitValidationSample/tree/Issue2/Solution-2

---

Happy coding
Tim

[timunie]

Short update: Issue 1 is gone now. Thank you for your effort.

[Sergio0694]

That's awesome, thank you for confirming that and for the original report! 🥳 🎉