-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature] Microsoft.Toolkit.Mvvm package (Preview 5) #3428
Comments
Posting an update here regarding the changes to the
Code changes: private Task myTask;
public Task MyTask
{
// BEFORE
set => SetPropertyAndNotifyOnCompletion(ref myTask, () => myTask, value);
// AFTER
set => SetPropertyAndNotifyOnCompletion(() => ref myTask, value);
} And some benchmarks: |
Hey @nanney54, let's continue here from your original comment in the issue for the Preview1. I've come up with a solution I liked to add cancellation support to the async commands, so this new feature should be available in the next preview of the MVVM Toolkit, and of course you can also try it out already through the CI preview build 😄 The changes were added in 06cc520, here's the new /// <summary>
/// An interface expanding <see cref="IRelayCommand"/> to support asynchronous operations.
/// </summary>
public interface IAsyncRelayCommand : IRelayCommand, INotifyPropertyChanged
{
/// <summary>
/// Gets the last scheduled <see cref="Task"/>, if available.
/// This property notifies a change when the <see cref="Task"/> completes.
/// </summary>
Task? ExecutionTask { get; }
/// <summary>
/// Gets a value indicating whether running operations for this command can be canceled.
/// </summary>
bool CanBeCanceled { get; }
/// <summary>
/// Gets a value indicating whether a cancelation request has been issued for the current operation.
/// </summary>
bool IsCancellationRequested { get; }
/// <summary>
/// Gets a value indicating whether the command currently has a pending operation being executed.
/// </summary>
bool IsRunning { get; }
/// <summary>
/// Provides a more specific version of <see cref="System.Windows.Input.ICommand.Execute"/>,
/// also returning the <see cref="Task"/> representing the async operation being executed.
/// </summary>
/// <param name="parameter">The input parameter.</param>
/// <returns>The <see cref="Task"/> representing the async operation being executed.</returns>
Task ExecuteAsync(object? parameter);
/// <summary>
/// Communicates a request for cancelation.
/// </summary>
/// <remarks>
/// If the underlying command is not running, or if it does not support cancelation, this method will perform no action.
/// Note that even with a successful cancelation, the completion of the current operation might not be immediate.
/// </remarks>
void Cancel();
} Both Here's a small example: async Task<string> FooAsync(CancellationToken token)
{
await Task.Delay(10000, token); // Delay with the token
return "Hello world";
}
// Create a new async command wrapping the async method with
// the token parameter. The C# compiler will automatically pick the
// right constructor depending on the signature of the method we
// are using here (depending on whether it takes a token or not).
var command = new AsyncRelayCommand(FooAsync);
// Use the command as usual... Let us know what you think! 😊 |
It was fast ! did you have a git stash ready on your PC before my post ? 😁 |
This rewrite of MvvmLight sounds like an excellent idea! We've been left with no official .NET Core support for quite some time. And IMO, this should have been part of the .NET Framework from the start. WPF/UPF development has been a mess because many required bricks were missing and everybody was hacking around it on their own. If you rethink a minimalistic support, however, I'd like to question the Messaging feature itself. In my mind, let's say I want a "Notifications" section in my app, I would create a Singleton class that manages those communications in a strongly-typed way. Are there cases where using Messaging is actually a recommended design? Please help me understand. Another thing that frustrated me with WPF is that lots of required basic bricks were missing and I had to search the web for hacks and solutions all the time until I built a basic library of fundamental tools that should have been in the framework. PushBinding, Int32Extension, MethodBinding and perhaps a few other classes should be part of the fundamental framework IMO. With the idea of keeping it basic. |
Hey @mysteryx93 - glad to see your enthusiasm for this project! 😊
That's... Exactly what the messenger is 😄 As for the other components you mentioned, we don't currently plan to include things like that, because the package itself is completely platform agnostic. For instance, that method binding you mentioned for WPF is effectively replaced by just Because of that, we will not include any framework-specific APIs in this package, but you're free to integrate it into your own codebase by adding your custom extensions where you feel the need 👍 |
My point is... IMO that's a very niche usage that I haven't found a use for yet, and I don't see what it has to do with MVVM either. Perhaps some application can use it, but why is it a core MVVM feature?
WOW! MVVM Docs!?? That would be a nice supplement!! (in the meantime I found out that there is some PRISM doc that can serve to understand MvvmLight) btw the link you posted is broken |
Mmh not sure why that link wasn't working, I've fixed it now (also, it was in the first post in this issue too) 😄
You can argue it's not a part of MVVM per se, yes. But it's still a useful feature to have, especially since it's fully self-contained in the library, so we can include it without having to pull other dependencies in, so there's no cost in doing that. Also, MvvmLight offered that too, so it only seemed natural to include a migration path for those users as well. There aren't other exactly equivalent services available right now (not that I'm aware of), so we figured it'd be a good idea to provide an implementation ready to use out of the box for those that are interested. If you don't need it, you can just not use it. All the components in the MVVM Toolkit are completely modular, so you're free to pick up and choose what parts to use without being forced into any of them 👍 |
I'm also curious about one thing. MvvmLight had a special RelayCommand for WPF otherwise the default one wouldn't work correctly, and this ugly work-around was the only solution they found. How would this situation be handled by this framework? I think the problem had to do with command enabled/disabled status being properly updated on the UI. |
I think you should reconsider the decision to have It seems like a convenience for users which want to use By having only one constructor for |
Following on from the comment by @laurentkempe, is it possible to have the best of both worlds by having a single constructor with a default value for the parameter? protected ObservableRecipient(IMessenger messenger = WeakReferenceMessenger.Default)
{
...
} If I was concerned about developers in a team calling the constructor and not passing a specific messenger, I'd add an analyzer to the project to check for this. Having the |
@mrlacey Unfortunately that is not a valid C# signature, as values for default parameters need to be compile-time constants. I agree that using the |
@mrlacey Even if that would work, and it doesn't because
My point @Sergio0694 was not about |
Thanks @laurentkempe, I do like the idea of forcing the decision for those upgrading. I mean there is a benefit to using the new model; we just want to keep it simple as well for those onboarding to MVVM for the first time. Not having to worry about the lifetime of the messenger is a benefit from that standpoint. This is also not the default case as usually In your cases of using MVVMLight in the past, would you typically have a base class setting up the messenger? If so, then having the extra code only being in one place vs. several for an upgrade path wouldn't be a big overhead. So far our migration guide seems fairly straight-forward. @jamesmcroft you've done a lot with MVVMLight as well, thoughts? |
You are welcome @michael-hawker I am trying to help! I totally understand the point easing the onboarding of newcomers and see it has a valid point and has you said the default case is usually
For sure as you replicate the way MVVMLight was doing it but with its drawback too! |
I recently found out that this is going to be a thing and so i checked out the most recent public preview 4. I don't really have complains and like what i see, how ever i'd like to see 2 things added, if possible:
|
Are you able to make both |
The new update for the MVVM Toolkit is ready for review now, posting an update here for those interested. For starters, here's the complete API breakdown for the namespace Microsoft.Toolkit.Mvvm.ComponentModel
{
public abstract class ObservableValidator : ObservableObject, INotifyDataErrorInfo
{
public event EventHandler<DataErrorsChangedEventArgs>? ErrorsChanged;
protected ObservableValidator();
protected ObservableValidator(IDictionary<object, object> items);
protected ObservableValidator(IServiceProvider serviceProvider, IDictionary<object, object> items);
protected ObservableValidator(ValidationContext validationContext);
public bool HasErrors { get; }
protected bool SetProperty<T>(ref T field, T newValue, bool validate, [CallerMemberName] string? propertyName = null);
protected bool SetProperty<T>(ref T field, T newValue, IEqualityComparer<T> comparer, bool validate, [CallerMemberName] string? propertyName = null);
protected bool SetProperty<T>(T oldValue, T newValue, Action<T> callback, bool validate, [CallerMemberName] string? propertyName = null);
protected bool SetProperty<T>(T oldValue, T newValue, IEqualityComparer<T> comparer, Action<T> callback, bool validate, [CallerMemberName] string? propertyName = null);
protected bool SetProperty<TModel, T>(T oldValue, T newValue, TModel model, Action<TModel, T> callback, bool validate, [CallerMemberName] string? propertyName = null) where TModel : class;
protected bool SetProperty<TModel, T>(T oldValue, T newValue, IEqualityComparer<T> comparer, TModel model, Action<TModel, T> callback, bool validate, [CallerMemberName] string? propertyName = null) where TModel : class;
protected bool TrySetProperty<T>(ref T field, T newValue, out IReadOnlyCollection<ValidationResult> errors, [CallerMemberName] string? propertyName = null);
protected bool TrySetProperty<T>(ref T field, T newValue, IEqualityComparer<T> comparer, out IReadOnlyCollection<ValidationResult> errors, [CallerMemberName] string? propertyName = null);
protected bool TrySetProperty<T>(T oldValue, T newValue, Action<T> callback, out IReadOnlyCollection<ValidationResult> errors, [CallerMemberName] string? propertyName = null);
protected bool TrySetProperty<T>(T oldValue, T newValue, IEqualityComparer<T> comparer, Action<T> callback, out IReadOnlyCollection<ValidationResult> errors, [CallerMemberName] string? propertyName = null);
protected bool TrySetProperty<TModel, T>(T oldValue, T newValue, TModel model, Action<TModel, T> callback, out IReadOnlyCollection<ValidationResult> errors, [CallerMemberName] string? propertyName = null) where TModel : class;
protected bool TrySetProperty<TModel, T>(T oldValue, T newValue, IEqualityComparer<T> comparer, TModel model, Action<TModel, T> callback, out IReadOnlyCollection<ValidationResult> errors, [CallerMemberName] string? propertyName = null) where TModel : class;
protected void ValidateAllProperties();
protected void ValidateProperty(object? value, [CallerMemberName] string? propertyName = null);
protected void ClearErrors(string? propertyName = null);
public IEnumerable<ValidationResult> GetErrors(string? propertyName = null);
IEnumerable INotifyDataErrorInfo.GetErrors(string? propertyName);
}
} In particular, the additions from #3562 include:
cc. @michael-hawker @timunie @thomasclaudiushuber @stevenbrix |
## Follow up to #3428 <!-- Add the relevant issue number after the "#" mentioned above (for ex: Fixes #1234) which will automatically close the issue once the PR is merged. --> This PR is for tracking all changes/fixes/improvements to the `Microsoft.Toolkit.Mvvm` package following the Preview 4. <!-- Add a brief overview here of the feature/bug & fix. --> ## PR Type What kind of change does this PR introduce? <!-- Please uncomment one or more that apply to this PR. --> - Feature - Improvements <!-- - Code style update (formatting) --> <!-- - Refactoring (no functional changes, no api changes) --> <!-- - Build or CI related changes --> <!-- - Documentation content changes --> <!-- - Sample app changes --> <!-- - Other... Please describe: --> ## 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: - [X] Tested code with current [supported SDKs](../readme.md#supported) - [ ] ~~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR 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](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~ - [X] Tests for the changes have been added (for bug fixes / features) (if applicable) - [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*) - [X] Contains **NO** breaking changes
The MVVM Toolkit Preview 5 is now merged and available in the preview Azure feed from master! 🚀 It features a much improved Thank you everyone for all the feedbacks throughout all these iterative previews, it's been super helpful and the MVVM Toolkit has improved a lot over time already! I'll keep an eye out for other possible minor tweaks to slip in before the first official release, but this is all looking really great so far - we're almost there! 🎉 🎉 |
Please confirm where this is available from. The wiki points to a MyGet feed that no longer exists (https://dotnet.myget.org/gallery/uwpcommunitytoolkit) and it appears that support for the use of MyGet was removed in place of using Azure DevOps. The main Azure DevOps feed only lists preview4. The Azure DevOps Pull Request feed has a version of this available but as version 7.0.0-build.840.g99bd68412a. Have I missed something? It seems misleading to say preview 5 is available in that it's not obvious how to get it and try it out. |
Where do you see that link mentioned? It might just be a leftover.
I didn't say "Preview 5", I specifically said the "MVVM Toolkit Preview 5", to mean that the changes in my PR called "MVVM Toolkit Preview 5" are now merged and therefore available through CI packages. For instance, you can find those bits in the latest one, which is Hope this clears things up! 😄 |
Yeah I think the origin of the misunderstanding is here: |
Yes, I didn't think there was a MyGet feed any more and so searched for |
Overview
This issue is meant to be used to track all the feature requests and discussions regarding API changes or other tweaks for the
Microsoft.Toolkit.Mvvm
package, which was introduced with #3230 and recently announced. 🎉Find initial docs and samples here at https://aka.ms/mvvmtoolkit.
Key Tenants
If you want more info on the package, you're welcome to browse our samples and preview documentation (here), watch the initial presentation by @michael-hawker and I (here) or watch Michael's presentation during the UNO Conf (here).
Tracking changes so far
Will be updating this list with all the current (work in progress) changes that are being included into a PR.
Preview 5 (#3562)
ObservableValidator.ClearErrors
methodObservableValidator.ValidateProperty
methodObservableValidator.ValidateAllProperties
methodObservableValidator.GetErrors
methodWeakReferenceMessenger
on .NET Standard 2.0Preview 4 (#3527)
Ioc
class (see docs)ObservableObject.OnPropertyChanged(PropertyChangedEventArgs)
overloadObservableObject.OnPropertyChanging(PropertyChangingEventArgs)
overloadOnPropertyChanged
andOnPropertyChanging
overloads with astring
param are no longer virtualIAsyncRelayCommand.CanBeCanceled
propertyIAsyncRelayCommand
propertiesObservableValidator.HasErrors
propertyTrySetProperty
methods toObservableValidator
Preview 3 (shipped!) 🚀
ComponentModel, Input, DependencyInjection (#3429)
ObservableValidator
class forINotifyDataErrorInfo
SetPropertyAndNotifyOnCompletion
signature (7-14x faster, 60% memory usage reduction)Expression<Func<T>>
argsSetProperty<T, TModel>
overload to allow stateless delegates and better performanceAsyncRelayCommand
types (and interfaces)Ioc
class andMicrosoft.Extensions.DependencyInjection
dependencyMessenger (#3424)
WeakReferenceMessenger
typeMessenger
toStrongReferenceMessenger
StrongReferenceMessenger
IMessenger.Register
signature to remove closures (35% memory usage reduction, nothis
captured)IMessenger.Cleanup
API to help with extensibilityObservableRecipient
toWeakReferenceMessenger
The text was updated successfully, but these errors were encountered: