IDialogService.Close manage a boolean return type such as IDialogServ…

[rogiardi]

…ice.Activate method.

Description

In case Show or ShowModal are based on certain logic, you cannot know if the window is modal or not. If you also need to close the window from its ViewModel, you can call .Close(this) without worrying about possible exceptions.
To check if window has been found and correctly closed, you can test the boolean result

Checklist

• I have squashed my commits into a single one with a message that aligns with the contributing guidelines
• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works

[FantasticFiasco]

I'm currently on a mini-vacation and will be back on Saturday. I'll look more closely at the PR then, but from a glance I think we should cover this use-case with tests.

[FantasticFiasco]

The current behavior is to throw an exception when closing a dialog fails, and with your changes the behavior will be that a boolean is returned indicating whether the operation was successful or not.

Both behaviors convey whether the operation was successful or not, am I correct in that statement? I'm here scratching my head thinking I'm missing something, but then again it's early morning here in Sweden and my brain is in startup mode.

With your changes, what do we gain other than that the caller can use if statements instead of try/catchstatements?

[rogiardi]

Hello Mattias, thanks for your fast feedback, I hope you are experiencing a nice vacation. Well, as you told me both behaviors seem to have the same result, you finally know if the closing operation was successful or not, but “raise and catch” an exception is a more heavy operation then “if then else” or “ ? : ”. I think the real question is: why use an exception to communicate a boolean result? And why only in the Close method and not also in the Activate method? Please let me try to summarize and explain what we could gain by my approach: 1) By the second behavior you’ll have the same approach in both Activate and Close methods. 2) Depending on the dialog opening logic, you couldn’t know if the opened dialog is modal or not. By the second behavior you can simply close it by calling both DialogResult = xxx; and then DialogService.Close(VMInstance); without worrying about dialog’s opening mode and without using a try-catch block. 3) In the actual behavior the “DialogNotFoundException” is not the only one exception that the Close method could raise: think about some WPF error. So, in a try-catch block you must consider the exception type. 4) By the actual behavior, in some particular case, using a try-catch block is not “quick” for a developer. 5) By the second behavior, if you are not interested in Close result, you can always call Close(VMInstance) without using an empty catch or without using the Activate method before, to know if the dialog is still alive.. Because my english (I know) is not fluent, to clarify what I mean please let me add some code snippets. Consider the following showing logic:  Now, in the actual behavior, if you wanna close the dialog from its viewmodel you must do:  Or, if you don’t like to use a try catch block for that operation:  (the screenshot above is the actual scenario used in my current project but, when the dialog is in "non modal mode" and not "on top", we have a flashing to top effect before its closure) By my behavior you could do:  In a shorter logic, by the actual approach you must do:  By my approach you could do:  Anyway, if you prefer, you could also consider to add my behavior as an overloading of Close method. I hope you'll find my thought clear. Thanks for your time and please keep me in touch. Best Regards, Roberto.

[FantasticFiasco]

Your argument is sound, having a parity between IDialogService.Activate and IDialogService.Close is probably a good thing, but I have some considerations.

• Regarding your 3), to truly being able to call Close without any try/catch statement we need to catch the exceptions in IDialogService.Close. As you say, if System.Windows.Window.Close throws an exception, it has to be caught in DialogService.Close. Do you agree?
• Returning a boolean instead of throwing an exception is a breaking behavioral change. This has to be indicated with a new major version. Unfortunately I don't think the value of the PR is enough to grant a new major version. What we can do is this. I've created a new branch called releases/v8. If you edit the PR to target that branch instead of master, then I can keep breaking changes on that branch until there is value enough to create a new major version. I can also publish pre-releases from that branch, lets say we call it 8.0.0-beta1 or something similar, and you can take a dependency on that package until the new major version is released. Would that be acceptable to you?

[FantasticFiasco]

BTW, your English is perfect, you should not apologize for it.

[rogiardi]

Hi Mattias,
I'm really glad to read your comments about my considerations.
Regarding your comment at point 3: yes absolutely, I agree with you.
As you said the content and the value of my PR is not enough for a new major version, your proposal of publishing a new beta version is the right solution. This is absolutely acceptable for me and the team I working on.
I'll switch my PR to releases/V8 branch asap. Please let me know when we can update the package refence from Nuget.
Congratulations for the good Job on MVVMDialog, It's a very useful component and, if you agree, I would be so happy to give my contribute on it. Let me know if you have some bug or other short activity that I could achieve for you.

Thanks for your availability and see you soon.

Regards,

Roberto.

[FantasticFiasco]

I'll prepare the code for the release, but I have to do one other thing first. I'm estimating that we'll have a release for you later this week.

[rogiardi]

Perfect Mattias, take your time, there's no hurry..

[FantasticFiasco]

@rogiardi v8.0.0-beta.1 has now been published on www.nuget.org. Would you be able to update and see if the changes are according to your requirements?

[rogiardi]

Hi Mattias,
sorry for delay, I was out for a vacation... Yes, I saw you added an internal try-catch block in Close method. That changes are compliant with our requirements.. I'm going to reference the new version right now.. I'll update you soon..

Thanks,

Roberto.