[Feature] Microsoft.Toolkit.Mvvm package (Preview 2) #3429

Sergio0694 · 2020-08-13T16:36:15Z

Closes #3428

PR Type

What kind of change does this PR introduce?

Feature

Overview

This PR is used to track and implement new features and tweaks for the Microsoft.Toolkit.Mvvm package.
See the linked issue for more info, and for a full list of changes included in this PR.

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
Pull Request has been submitted to the documentation repository instructions. Link:
Sample in sample app has been added / updated (for bug fixes / features)
- Icon has been created (if new sample) following the Thumbnail Style Guide and templates
Tests for the changes have been added (for bug fixes / features) (if applicable)
Header has been added to all new source files (run build/UpdateHeaders.bat)
Contains NO breaking changes

ghost · 2020-08-13T16:36:20Z

Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

Sergio0694 · 2020-08-22T00:51:08Z

Looks like the changes in 1cb4df2 uncovered a bug in the .NET Native compiler, so the build is now failing in Release UWP 🤦‍♂️

ILT0005: 'C:\Program Files (x86)\Microsoft SDKs\UWPNuGetPackages\runtime.win10-x64.microsoft.net.native.compiler\2.2.8-rel-28605-00\tools\x64\ilc\Tools\nutc_driver.exe @"C:\Users\Sergio\source\repos\FieldAccessorRepro\FieldAccessorRepro\obj\x64\Release\ilc\intermediate\MDIL\FieldAccessorRepro.rsp"' returned exit code 2

I managed to isolate the issue in the custom FieldAccessor<T> type that was introduced. Specifically, this code is enough to cause the build error even in a blank UWP app (just place this right in MainPage.xaml.cs or anywhere else:

public delegate ref T FieldAccessor<T>();

private int n;

private void Button_OnClick(object sender, RoutedEventArgs e)
{
    FieldAccessor<int> accessor = () => ref n;

    accessor() = default;
}

I've also prepared a repro project which you can find here: FieldAccessorRepro.zip.

Will create a full .NET Native repro to send to the team, but in the meantime I was wondering whether @MichalStrehovsky or @tommcdon might have some ideas of how to work around the issue on the UWP end? Maybe some .rd.xml magic? 😄

Regression to unblock UWP for now, see dotnet/corert#8273

michael-hawker · 2020-09-10T17:30:46Z

@msftbot make sure @stevenbrix reviews before merging

ghost · 2020-09-10T17:30:48Z

Hello @michael-hawker!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @stevenbrix

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

stevenbrix

Looks fairly good. Most comments are around validation of unset required properties. I feel like those should be considered invalid. There is a Validator.TryValidateObject method you can use to validate the whole model.

stevenbrix · 2020-09-11T14:33:17Z

Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs

+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private IEnumerable GetAllErrors()
+        {
+            return this.errors.Values.SelectMany(errors => errors);


What do we do for properties marked Required?

If those fields are null, should we consider them errors?

So, I have to say I'm not 100% I understand the official documentation here.
From the MS docs:

GetErrors: Gets the validation errors for a specified property or for the entire entity.

I read that as "get/retrieve the currently stored errors for either a specific property or for the entity (so for all properties)", as in, return the errors that have previously been computed, without actually doing any validation now. By that logic, the code right now is just returning the existing errors corresponding to the input query 🤔

I mean, that makes sense to me since we expose a separate method to actually validate properties and update errors, which is ValidateProperty, but I have to say with how the docs are currently phrased that's not entirely clear.

I guess the question is - should GetErrors just return existing errors for a given query, or actually perform a new validation for a given property (or all properties, if the whole entity is targeted)? Thoughts?

I agree, the docs aren't clear. I imagine, like many things, it's up to us to define the behavior.

Edit: I saw your samples below, and realized this behavior is correct :)

Some bug in GitHub is causing me to not be able to comment on this further down. But basically, I don't feel like a ValidateProperty method should be required. Can ObservableValidator override the SetProperty method and call ValidateProperty itself?

But, at the end of the day - I think this ok. When a user opens up a form, we don't want a bunch of errors for fields they haven't entered anything into yet

Can we fold ValidateProperty into the SetProperty method? Those should always go hand-in-hand.

Done in 28b1977 as discussed on Discord 👍

stevenbrix · 2020-09-11T14:34:29Z

Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs

+            {
+                if (errorsChanged)
+                {
+                    this.totalErrors--;


I'm not exactly sure what this count value is for. Can we not use the errors dictionary as the source of truth?

I've added an XML comment for that here:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/be109be7a7c1b03c9fac2fa51e6b1f4c398f5a48/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs#L27-L32

My idea here was - with how the code is currently setup, the dictionary can contain entries for valid properties, just with the list of errors being empty (so that we can reuse the same List<ValidationResult> instances when possible. So in order to check whether we have errors, we'd have to traverse the entire dictionary and check whether all values are empty lists (or if no entries are there at all). This would have a cast of O(N), as you'd have to traverse the entire dictionary once to check all properties, and the dictionary could potentially contain all the existing properties for a given object. So the point of this additional field, which is kept in sync with the dictionary, is to provide a fast O(1) accessor to quickly check whether or not there are any errors in the current instance.

Let me know what you think! 😊

I appreciate the attention to perf, but how often do we expect errors to transition between valid/invalid?

My gut is that we won't really be allocating that often to warrant this extra bit of complexity and size trade off. Computers are much faster at allocating/deallocating memory than humans are at typing :)

Yeah that's a good point, I've removed the field in e7a4824 😊

Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs

UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs

ghost · 2020-09-11T14:41:44Z

Hello @msftbot[bot]!

Because this pull request has the auto merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2020-09-11T15:38:35Z

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

stevenbrix

Overall looks good!

The only thing I would suggest is to remove the ValidateProperty method. There is already the Validator class.

stevenbrix · 2020-09-23T23:40:38Z

Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs

+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private IEnumerable GetAllErrors()
+        {
+            return this.errors.Values.SelectMany(errors => errors);


Some bug in GitHub is causing me to not be able to comment on this further down. But basically, I don't feel like a ValidateProperty method should be required. Can ObservableValidator override the SetProperty method and call ValidateProperty itself?

stevenbrix · 2020-09-23T23:46:43Z

Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs

+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private IEnumerable GetAllErrors()
+        {
+            return this.errors.Values.SelectMany(errors => errors);


But, at the end of the day - I think this ok. When a user opens up a form, we don't want a bunch of errors for fields they haven't entered anything into yet

stevenbrix · 2020-09-23T23:51:21Z

Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs

+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private IEnumerable GetAllErrors()
+        {
+            return this.errors.Values.SelectMany(errors => errors);


Can we fold ValidateProperty into the SetProperty method? Those should always go hand-in-hand.

ghost · 2020-09-24T09:37:54Z

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

Sergio0694 · 2020-09-24T11:23:33Z

And part 1 is done! Thanks @stevenbrix for the help! 🎉🎉🎉

Add initial version of ObservableValidator type

c3b3d03

Sergio0694 added feature 💡 improvements ✨ DO NOT MERGE ⚠️ .NET Components which are .NET based (non UWP specific) labels Aug 13, 2020

Sergio0694 added this to the 7.0 milestone Aug 13, 2020

ghost requested review from michael-hawker, azchohfi and Kyaa-dost August 13, 2020 16:36

ghost added the feature request 📬 A request for new changes to improve functionality label Aug 13, 2020

Sergio0694 mentioned this pull request Aug 13, 2020

[Feature] Microsoft.Toolkit.Mvvm package (Preview 5) #3428

Closed

27 tasks

Sergio0694 added 8 commits August 13, 2020 18:47

Update ValidateProperty API to mirror SetProperty

67908f3

Add XML docs for thrown exceptions

ffb54ca

Removed unnecessary using directive

f429a78

Speed/memory improvements to SetPropertyAndNotifyOnCompletion

1cb4df2

Added cancelation support for async commands

06cc520

Fixed spelling to follow conventions

06ca186

Merge branch 'master' into feature/mvvm-preview2

bcb4244

Added unit tests for cancelable async commands

b8610ab

Removed unnecessary visibility modifiers

38680ea

Sergio0694 mentioned this pull request Aug 23, 2020

[.NET Native] ILT0005 build error with ref T returning delegate dotnet/corert#8273

Closed

Sergio0694 added 6 commits August 31, 2020 22:35

Temporary reverted FieldAccessor<T> APIs

cfb2628

Regression to unblock UWP for now, see dotnet/corert#8273

Removed Ioc class and MS.Ex.DI dependency

0bd1f92

Removed repeated ErrorsChanged event raise

4144f60

Added null/"" handling in ObservableValidator.GetErrors

fed7051

Added TaskAccessor<TTask> API, code refactoring

b0f6b71

Hidden Task/Task<T> type from TaskNotifier

dd4fd4c

michael-hawker added auto merge ⚡ and removed auto merge ⚡ labels Sep 10, 2020

CommunityToolkit deleted a comment Sep 10, 2020

stevenbrix suggested changes Sep 11, 2020

View reviewed changes

ghost added the needs author feedback 📝 label Sep 11, 2020

Removed unnecessary List<T> allocation

77ed6e8

ghost added needs attention 👋 and removed needs author feedback 📝 labels Sep 11, 2020

Merge branch 'master' into feature/mvvm-preview2

e57ba22

stevenbrix suggested changes Sep 23, 2020

View reviewed changes

ghost added the needs author feedback 📝 label Sep 23, 2020

stevenbrix approved these changes Sep 24, 2020

View reviewed changes

Code refactoring in ObservableValidator

28b1977

ghost removed the needs author feedback 📝 label Sep 24, 2020

Sergio0694 added 4 commits September 24, 2020 11:45

Removed tracking field for HasErrors

e7a4824

Merge branch 'master' into feature/mvvm-preview2

93018fa

Fixed build error due to StyleCop

c5a5d5d

Code refactoring (not a fan of StyleCop here)

fec4b22

Sergio0694 merged commit ce296f9 into CommunityToolkit:master Sep 24, 2020

Sergio0694 mentioned this pull request Sep 27, 2020

Leftover ObservableObject XML docs fix, minor tweaks #3505

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Microsoft.Toolkit.Mvvm package (Preview 2) #3429

[Feature] Microsoft.Toolkit.Mvvm package (Preview 2) #3429

Sergio0694 commented Aug 13, 2020 •

edited

Loading

ghost commented Aug 13, 2020

Sergio0694 commented Aug 22, 2020

michael-hawker commented Sep 10, 2020

ghost commented Sep 10, 2020

stevenbrix left a comment

stevenbrix Sep 11, 2020

Sergio0694 Sep 11, 2020 •

edited

Loading

stevenbrix Sep 23, 2020 •

edited

Loading

stevenbrix Sep 23, 2020

stevenbrix Sep 23, 2020

stevenbrix Sep 23, 2020

Sergio0694 Sep 24, 2020

stevenbrix Sep 11, 2020

Sergio0694 Sep 11, 2020 •

edited

Loading

stevenbrix Sep 23, 2020

Sergio0694 Sep 24, 2020

ghost commented Sep 11, 2020

ghost commented Sep 11, 2020

stevenbrix left a comment

stevenbrix Sep 23, 2020

stevenbrix Sep 23, 2020

stevenbrix Sep 23, 2020

ghost commented Sep 24, 2020

Sergio0694 commented Sep 24, 2020

[Feature] Microsoft.Toolkit.Mvvm package (Preview 2) #3429

[Feature] Microsoft.Toolkit.Mvvm package (Preview 2) #3429

Conversation

Sergio0694 commented Aug 13, 2020 • edited Loading

Closes #3428

PR Type

Overview

PR Checklist

ghost commented Aug 13, 2020

Sergio0694 commented Aug 22, 2020

michael-hawker commented Sep 10, 2020

ghost commented Sep 10, 2020

stevenbrix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

stevenbrix Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 11, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Sep 11, 2020

stevenbrix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 24, 2020

Sergio0694 commented Sep 24, 2020

Sergio0694 commented Aug 13, 2020 •

edited

Loading

Sergio0694 Sep 11, 2020 •

edited

Loading

stevenbrix Sep 23, 2020 •

edited

Loading

Sergio0694 Sep 11, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.