sonnemaf / ToolkitMvvmDemo

Demo project using the Toolkit.Mvvm package.
13 stars 1 forks source link

A couple questions, and thank you for the demo! #1

Open Sergio0694 opened 4 years ago

Sergio0694 commented 4 years ago

Hey Fons, Thank you for taking the time to try out the new lib and for building a demo, this is cool! 😄 I'm glad to hear the new lib is working fine for you so far!

I have a couple of quick feedbacks/questions:

Async request messages

I was looking at this line:

https://github.com/sonnemaf/ToolkitMvvmDemo/blob/12c7a3c5050967200dbb32932f950af75ad99688/MvvmDemo/ViewModels/MainViewModel.cs#L41

Where you have that verbose .Result access at the end. I checked the request message, which is:

https://github.com/sonnemaf/ToolkitMvvmDemo/blob/12c7a3c5050967200dbb32932f950af75ad99688/MvvmDemo/Messages/AsyncYesNoMessage.cs#L10

And you've just inherited from RequestMessage<T> with a Task<T> type. For these situations, I've created the AsyncRequestMessage<T>, which is basically the same but supporting Task<T> results directly, so that the code is less verbose and you can directly use await in a message of that type, as it implements GetAwaiter and just relays that call to the wrapped Task<T> instance. If you give it a try, let me know if that works! You should literally just be able to have your message inherit from AsyncRequestMessage<bool>, and then remove that .Result when awaiting the request.

Redundant null checks?

I noticed these checks here:

https://github.com/sonnemaf/ToolkitMvvmDemo/blob/12c7a3c5050967200dbb32932f950af75ad99688/MvvmDemo/ViewModels/MainViewModel.cs#L40

Since these methods are private and only invoked from the commands, which have a canExecute method that also checks whether the input is not null, whouldn't it be possible to remove these checks here to make the code less verbose? Did you try this out already? I'm just curious to know whether you did this just out of habit or whether you had any issues when trying to remove those checks. Let me know!

Thanks again for your time testing this out, I really appreciate it! 😊

sonnemaf commented 4 years ago

As expected you are right. The null checks where necessary before I wrote the canExecute. They are now removed.

I wrote the AsyncYesNoMessage from your June 18 example . Just now I read that you have introduced the AsyncRequestMessage on July 1st. I have updated my code.

Thanks for your remarks. I started MVVM (Silverlight period) using the MVVMLight framework. Used my own implementations (mainly ObservableObject, RelayCommand) in UWP apps. I will switch to your solution for future projects. It is nice that this also works form WPF.

My solution now also contains a ClassLibrary (Models, ViewModels, Services, Messages), a WPF project (MainWindow) and a UnitTest. To make it complete.

Sergio0694 commented 4 years ago

Happy to help! Also it's super cool that you've added a WPF sample as well, thank you! 😊

I'm really glad to hear you plan to switch to this new library for future projects, hope it'll work great for you!

sonnemaf commented 4 years ago

I'm not sure how I can use ObservableRecipient. This is a new pattern for me. Will play with to figure it out next.

Sergio0694 commented 4 years ago

It's mostly done to facilitate the usage of the IMessenger service within observable objects. Basically, the point is that you need to remove message handlers from the messenger in use, otherwise it'll keep strong references to them and you basically will end up with a memory leak until you just reset the messenger entirely. So that class exposes the OnActivated and OnDeactivated methods which are called when you set IsActive to true or false.

The idea is that if you register messages only from OnActivated, you then get a single line of code to check the lifetime of all those handlers. For instance, if this type is used as a viewmodel for a page, you could set IsActive = true when the page is navigated to, and IsActive = false when the page is navigated away from. This way it'll be easier to make sure that all the message handlers are only registered when a given page is effectively loaded and displayed on screen.

Also note that you can register messages with the pattern you prefer, you don't have to use IRecipient<TMessage>. If you prefer to just do Messenger.Register<MyMessage>(this, m => { ... });, that's fine as well. When OnDeactivated is called, it will unregister all the registered messages no matter how they were registered.

Let me know if the docs on ObservableRecipient are clear enough or if you have any other questions! 😊