Skip to content
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

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

Open
Sergio0694 opened this issue Aug 3, 2020 · 4 comments
Open

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

Sergio0694 opened this issue Aug 3, 2020 · 4 comments

Comments

@Sergio0694
Copy link

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:

if (await Messenger.Default.Send(new AsyncYesNoMessage($"Delete {emp.Name}?")).Response) {

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

public sealed class AsyncYesNoMessage : RequestMessage<Task<bool>> {

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:

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
Copy link
Owner

sonnemaf commented Aug 3, 2020

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
Copy link
Author

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
Copy link
Owner

sonnemaf commented Aug 3, 2020

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
Copy link
Author

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! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants