Closed virzak closed 2 years ago
Hi @virzak!
I think the abstraction, to have a core assembly and a second assembly/NuGet package targeting a specific presentation framework, is viable once MVVM Dialogs supports a second presentation framework.
Until then there isn't really any value added by introducing the core assembly. Even though it would lend itself to support many presentation frameworks, it would be a steep curve for any single developer to implement support for their specific presentation framework. Another thing that would be hard to get right would be the correct API of the core assembly. Building the API based on only one presentation framework would be hard, one would like to validate the API with at least two implementations, preferably three.
Given that this feature request depends on supporting more than one presentation framework, what would be your personal pick for a second?
You're right, the PR I made should include an example of a second or third framework, which I haven't had a chance to do yet.
The project I'm working on is a WPF project. Since all the resources are being moved to WinUI and WPF/UWP get no new development, WinUI seems like a clear choice. Additionally, the client of my project would benefit from a move to the web, so Blazor implementation would be extremely valuable to reuse a lot of code. Blazor MVVM support is limited and is something Microsoft wants to leave up to the community to develop.
It's also a must for Avalonia.
Avalonia, WinUI3 and Blazor will all need this. I can't see how it can be that complicated, I could help with this.
Unless someone can point me to a more viable solution, which I have yet to see.
Let me take a look at the source code. Remove dependencies and see how much blows up :P
It already nearly compiles after removing platform-specific code.
It shouldn't be too hard. 80% of platform-specific code is in the classes ending with Wrapper. Just the public interfaces will need to be cleaned up a bit.
This week there is no bandwidth for me to look at these code changes, but maybe in the end of next or the week after that I'll see if I can help.
I think you should start with trying to implement the functionality of DialogServiceViews
in your respective UI framework. If you get that file right, then DialogService
/DialogServiceBase
will fall into line. And after that It's just a lot of easy changes to get the functionality there.
I do platform-specific code last. Delete dependencies, make code compile, comment some files and codes.
For today: move all commented code into a separate assembly, then clean/adapt the code structure.
In spite or radical structural changes, the public API should remain the same except namespace changes, so your unit tests will be useful.
It will take me a few days to work on this. Then you can add more unit-testing, more platforms and samples, and ensure it has proper compatibility with the various frameworks. Although Avalonia, Blazor and WinUI3 are all .NET Core so no need for backwards compatibility here.
@virzak has a PR open here. What you're doing is trying to accomplish the same thing. Please see if there's code from the PR that you can lift over to your fork, or do the work on his fork, the decision is yours.
@FantasticFiasco BTW, there are a few other RPs I wanted to send. If you'd be open to them, they should be done before this abstraction:
What do you think?
Are we both working on it at the same time separately? I'm nearly done with my branch of the code. MvvmDialogs.Core compiles perfectly (untested), and mostly remains to deal with platform dialogs in WPF.
And here's a dilemma: settings can vary from one platform to the other. MessageBoxResult has a different definition in every platform, as well as MessageBoxIcon and MessageBoxOption.
So either we
and yes in the sample projects, MvvmLight should be removed because it's no longer supported and never properly supported .NET Core
How much did you do on your side? Please wait until I've got the code completed tomorrow, and then let's work from that branch. It's nearly done.
@mysteryx93 I haven't done anything since I checked in #176
Take a lead on this then and we'll discuss once you're done.
If .NET 4.5 isn't supported anymore, I'd vote for removing all PRE_NET45 and PRE_NET40 stuff.
I prefer a cleaner code base.
@virzak regarding your questions.
Reduce amount of sample projects by multitargeting sample projects to both .Net Core and .Net Framework
Sounds like a good idea if you also verify that the UI tests pass.
Remove support for .Net Frameworks before 4.5.2. They reached end of life and are not supported by VS 2022.
.NET Framework 4.5.2 will be supported until April 26, 2022 according to their documentation, but there aren't any planned releases so removing support for it on default branch is perfectly fine.
Replace MVVMLight with CommunityToolkit.Mvvm
MVVM Light proposes customers to move over to WindowsCommunityToolkit. Do you have any prior experience from any of them?
@mysteryx93 I'll have to take a closer look at the changes before making any decisions, but I'm leaning on to agree with you regarding the second option.
@FantasticFiasco, I meant to keep 4.5.2, but to get rid of 4.5.1 and before. The main driver is being unable to compile: "Starting with Visual Studio 2022, Visual Studio no longer includes .NET Framework components for .NET Framework 4.0 - 4.5.1 because these versions are no longer supported."
WindowsCommunityToolkit was renamed to CommunityToolkit.Mvvm and it is extremely similar to MVVM Light.
CommunityToolkit.Mvvm was extracted from WindowsCommunityToolkit into CommunityToolkit.
It now compiles beautifully :) Now going for the tests.
btw your tests were referencing internal members like cache and stuff... how was it working? Is there another solution than making everything public?
No point in supporting older frameworks. If they want to stay with earlier framework, then just don't upgrade MvvmDialogs and stick to the one that works.
It's working absolutely beautifully out-of-the-box. Fixing sample projects. Views and ViewModels need nothing more than namespace changes. Since it only references IDialogService, I can change class names and it doesn't break the code. Just need to fix class registration.
Custom dialogs need more code changes, simplifying the existing code.
I just made an interesting code change. Moved FrameworkDialogs out of IDialogService, and have them as extension methods instead. This way, we can have an extra library adding Ookii dialogs. Since we're not limited by the default implementations, there is no need for IDialogFactory anymore. It's a big change; but doesn't break existing code.
Unless using custom dialogs, then those calls will need to be renamed to new extension method names. Some code change in the ViewModels, but Replace All should take care of it. Overall I think this modular approach is worth it.
Nooooo bad idea. If dialogs are extension methods, the ViewModel needs a reference to the WPF assembly, and if the ViewModel has references to 2 different platform implementations, it will bring collisions of extension methods defined twice. Need to go back to DialogFactory with a standard platform-agnostic API.
Remains the question. Is there interest in having extra dialog methods available?
Ookii has
How would one use such extended dialogs? Implementing them on other platforms would be a hassle too. Note that not all methods of the IFrameworkDialogFactory need to be implemented for every platform.
Ideally we'd add another assembly MvvmDialogs.Wpf.Ookii
One option is to add Custom1, Custom2 and Custom3 to the IFrameworkDialogFactory and IDialogFactory.ShowCustom1(), but I don't like that idea.
Then I don't like the demo implementation of TaskDialog. It uses the same MessageBoxSettings, meaning it cannot use any extra features of the TaskDialog. The IFrameworkDialogFactory pattern doesn't allow us to change the Settings class.
Ideas as to how to implement this to be flexible and extensible way while maintaining UI separation?
Unless... IFrameworkDialogFactory has a single method taking the Settings type as parameter. Then its configuration can become a lot more flexible. Let me play around with that. If going with extension methods, they'd have to be defined in the Core library.
Works now. Here's the new CustomFrameworkDialogFactory. FrameworkDialog extension methods are defined here.
Much simpler and more extensible.
Will need to develop those libraries
Blazor implementation should be an interesting experiment
I'm also looking to add Async overloads to thread-locking functions like ShowDialog; may be required for some frameworks.
All .NET Core demos work!
Here's a TODO list. There's still plenty of work to do, I'll do some of it, and both of you can also do parts
You can submit push on my code branch; once it's done I push it all back to Fiasco's main repository
InternalsVisibleTo
) into public classes placed in a Private
namespace (@FantasticFiasco)try
/catch
logic in DialogServiceBase.Close
Once the core library is stable enough, the project is modular enough to be multiple working on it without collision.
Being based on .NET Standard 2.0, it should support from .NET Core 3.1 and .NET Framework 4.6.1
All .NET Framework demos now run!
Running the tests, some work, and those based on SpecFlow run the app and open the window and the test fails. I've never played with SpecFlow and I'm not sure why it's behaving that way. Fiasco can you take a look at that? I'll leave that part to you.
Added properties to NamingConventionDialogTypeLocator to make it more flexible:
public string ViewModelFolder { get; set; } = "ViewModels";
public string ViewModelSuffix { get; set; } = "ViewModel";
public string ViewFolder { get; set; } = "Views";
public string ViewSuffix { get; set; } = ""; // I'd put this to View but don't want to break default compatibility
Remains to test.
@mysteryx93 you're on a roll!
Tonight I will dedicate time to catch up, we'll split that tasks among us.
For ShowDialog vs ShowDialogAsync, there's a design decision to make. How to implement it?
WPF uses ShowDialog (non-async)
Avalonia uses ShowDialog which is async
OK that answers my question, better have the internal API work based on async only.
Remains the question as to whether or not to expose a non-async ShowDialog.
Here it says that calling async as sync can be dangerous. Particularly in this case the async code (dialog) will be doing a lot of UI work. Running the async method as sync could lead to deadlocks.
If we only expose an Async method, that's a breaking change that will require API users to update their code, but would ensure forward-compatibility and avoid nasty deadlock issues. Thoughts?
Also ShowCustom and ShowCustomDialog have been removed, no longer needed. There are some breaking changes but they're very easy to fix.
ShowDialogAsync would be the "hardest" to fix because it requires decorating methods with async that can have a zombie viral effect if the code doesn't already have it.
ShowDialogAsync has been implemented.
Now there's a problem: demos now need AsyncRelayCommand which isn't in MvvmLight but is in MVVM Toolkit. Need to fix this while porting MvvmLight to MVVM Toolkit. I'll leave the demos as-is, virzah can you take care of this?
I'll take a quick look at unit tests then will move on to Avalonia support.
Edit: @FantasticFiasco, I'll leave the unit tests to you alongside code review. Naming convension need a bit more testing after adding configurable parameters. MessageBox tests fail because I moved the MessageBoxOptions flags as separate bool properties in the MessageBoxSettings class. Sync class is removed which breaks other tests. Couple of things to clean up in there.
Another thing to review in core library: what needs to be internal and what needs to be public.
Another design decision to make: core library and namespace should be MvvmDialogs or MvvmDialogs.Core ?
Here's an interesting discussion about using Avalonia ShowDialog sync vs non-sync
About the namespace, looking at MvvmDialogs vs MvvmDialogs.Core, looking at other libraries. There's xunit, xunit.abstractions, xunit.core, etc. There's Avalonia, Avalonia.Desktop, etc.
Core namespace is generally used as a dependency that is referred to directly by the user.
root namespace is generally the common API that the user is linking to -- that's what we have here.
I vote for removing the Core namespace and making it root. Also makes it closer to the existing behavior. Plus MvvmDialogs.Core otherwise gets lost in the list of projects instead of being at the top.
Done.
Just cloned your fork and started to look at the code. When it comes to the comments in this thread, I'll work backwards, assuming that the latest is of most importance. If this isn't the case, please let me know.
I agree regarding naming the core project MvvmDialogs
, where MvvmDialogs
also is the root namespace of said project. And we are fortunate here, since you've already completed that task, nice work! 😃 When it comes to the target specific projects (MvvmDialogs.Avalonia
, MvvmDialogs.Uwp
and MvvmDialogs.Wpf
) I would like us to try being consistent with using the same root namespace as in MvvmDialogs
, i.e. MvvmDialogs
. We should only change into MvvmDialogs.Avalonia
and similar if we encounter any problems transcending the same namespace across multiple projects.
I'll leave the unit tests to you alongside code review.
Check
Another thing to review in core library: what needs to be internal and what needs to be public.
I added a new task to your list, to refactor the API of the projects. Either I will stay with the current setup where some internal classes are exposed to friend assemblies, or I will move the internal classes into a new Private
namespace and change their accessibility to public.
WIthout Wpf and Avalonia namespaces, there will be class name collisions. Many of the class names begin with Wpf or Avalonia anyway, like WpfWindow implementing IWindow, and WpfView implementing ViewBase. But there are other classes like DialogServiceViews that would collide.
Plus; you never use those extra namespaces in your View Models. Only during configuration. You could even skip the need to reference those namespaces by adding UseMvvmDialogsWpf and UseMvvmDialogsAvalonia extension methods that auto-configure it. I don't know whether it's worth doing that since there is only a single class to register, and that class may need extra settings.
Note... if we don't need to reference most classes in those namespaces, most of it can be internal except what needs to be registered or what can be extended (framework dialogs).
Note: It may be possible to create an extended MessageBox with extra features working in both Ookii.Wpf and Avalonia, the Avalonia library here has extended features.
So, the initial publication was over 10 years ago. Give that a thought. An open source project that after 10 years still is maintained and extremely relevant with the release of .NET Core 3. Take that all you out there claiming open source code is volatile!
:laughing::laughing::laughing::dizzy_face:
3 issues with Avalonia MessageBox:
Plus it supports none of the Win32 dialog option flags. Some of these flags (RightAlign, RtlReading) could be set at the project level too. Having platform-specific settings configured at the project level works for "some" settings but not all. Then we go "lowest common denominator" or have settings taking no effect, or how do we handle this?
OpenFileDialog doesn't have ShowReadOnly/ReadOnlyChecked. CheckFileExists ... could check it manually ourselves. SafeFileName? That's useless, I'll just take it out.
SaveFileDialog just doesn't support much options...
We got an issue that some platforms support more features (Task Dialog), while some platform support less features (Avalonia Open/Save File), while some platform has just different option values (MessageBox icons). Filter extensions will need to be configured differently than then old Win32 string hack, which will be a breaking change. We'll need to rethink the whole framework dialogs settings approach.
Since Avalonia is open source, another option would be to add missing features into it.
Some settings are ancient and not useful for modern applications: SafeFileName, ValidateNames, SupportMultiDottedExtensions. RestoreDirectory doc says: This property is not implemented.
Other settings can be handled by MvvmDialogs: CheckFileExists, CheckPathExists, DereferenceLinks
While more settings can be set at the project level: Style, ShowHelp. Perhaps icon mapping? You define the icon styles that your app will use, and can configure them to different icons for different platforms? A bit complex to use though.
It will be considerable work to sort this out. It will be interesting how Blazor integration will play into this.
After splitting the settings into those 3 categories for Open/Save File, I'm left with ShowReadOnly, ReadOnlyChecked and FilterIndex to decide what to do with. You set the default filter by putting it first in the list, and you don't need to know which filter is selected in the list, so... OUT. As for ReadOnly, do note that I migrated your OpenFileDialog/SaveFileDialog from Microsoft.Win32 to System.Windows.Forms to have the Vista styles, but that if you set ReadOnly, it will revert back to the old style. Do we really need this in modern apps?
WIthout Wpf and Avalonia namespaces, there will be class name collisions. Many of the class names begin with Wpf or Avalonia anyway, like WpfWindow implementing IWindow, and WpfView implementing ViewBase. But there are other classes like DialogServiceViews that would collide.
From a project reference perspective I assume we are working with this:
MvvmDialogs
has no project referencesMvvmDialogs.Avalonia
is referencing MvvmDialogs
MvvmDialogs.Uwp
is referencing MvvmDialogs
MvvmDialogs.Wpf
is referencing MvvmDialogs
Having the same class name is the last three projects will be no problem since there are no references between them. But I have no problem with waiting to do this exercise in the end, attempting to align the names across the projects. I'll add that as a task.
Regarding the issue with the Avalon MessageBox, I'll have to invest more time into understanding the problem before I can provide any feedback.
I was thinking the same thing, standardizing the names between the various implementations; so you can shift from Wpf to Uwp simply by changing the namespace and nothing more. MvvmDialogs.Wpf.WpfWindow also is repetitive.
Renaming files... for WpfView and WpfWindow... can't think of anything better than going back to ViewWrapper and WindowWrapper? I don't like that name much though.
Done.
The window Activate/Close code is bulky.
Any reason you were doing a Try...Catch around Close method? Any particular reason it would fail? If it fails for user's fault, any reason not to leave the exception as-is?
Aaahhh calling Close() twice will throw an error, but here we'd want to return False instead. That will be a unit test case.
Code cleaned and restructured. Added AsWrapper extension method. There's really not much left in platform-specific DialogService! I like empty classes.
...can't think of anything better than going back to ViewWrapper and WindowWrapper? I don't like that name much though.
Add it as a task. It's something we can do later, perhaps as a better informed decision as we are closer to the target.
That will be a unit test case.
Added it as a task.
There's really not much left in platform-specific DialogService! I like empty classes.
😄
For now let's focus on adding support for Avalonia while remaining the support for WPF. This means we can unload UWP related projects, and I'll fix them later, while having all WPF and Avalonia projects loaded in the editor of choice. I will focus on reading up on the changes in MvvmDialogs
and MvvmDialogs.Wpf
.
I added appSettings as the first parameter of DialogService, allowing specifying platform-specific application-wide settings. It can be overridden for individual calls to allow setting platform-specific parameters.
In the rare cases where they'll set platform-specific settings (MessageBoxRightToLeft, MessageBoxDefaultDesktopOnly, MessageBoxServiceNotification), at least they'll be aware of it. Right-to-left sets both RightAlign+RtlReading. Note that Avalonia doesn't currently support Right-to-Left, but it will be added in the future.
CustomPlaces... probably should be set application-wide too.
InitialPath will now set both InitialDirectory/FileName in WPF; and Directory/InitialFileName in Avalonia.
Filters is now a list and it will encode itself automatically in WPF -- complex piece of code, needs extensive testing.
The '.' in extensions is optional. Extensions will automatically be added to the descriptions unless it contains '('. If you do not wish to display extensions, end the name with '()' and it will be trimmed away.
I need a little break after this one... good luck with testing. Using filters will be a LOT more pleasant!
Mostly remains manually adding missing features:
And finding a solution about icons.
Also I'll see if I can have the dialogs return their data instead of storing the result in the settings class.
I don't have permissions to push. Could you add both me and @virzak as contributors to the fork?
I added both of you
Dialogs now return their data directly instead of storing it in the settings class
Extracted Win32 framework dialogs API into IFrameworkDialogsApi so that it can be replaced with a mock for testing
Remains to extract the API for Avalonia
Also will need to create a wrapper for any file access.
All I/O need to be properly encapsulated so that you can unit test.
ShowHelp, replaced it with HelpRequest property; when set, it will show the help button. Not application-wide setting anymore.
I was thinking that we could allow passing derived settings class with extra platform-specific settings. If you pass WPF settings to Avalonia, it just takes the base class.
Dialogs API interface extracted for both WPF and Avalonia.
Encapsulated access to file and directory information as IPathInfoFactory
Adapting this library for cross-platform support is more work than it looks like on the surface!
Avalonia support is nearly completed. Just remains to create demo projects and test it.
Remains to do and I'm done:
@virzak there's still plenty of work to adapt all the demo projects if you want to help
@FantasticFiasco for unit testing, I do not think spreading the tests across 15 different projects is a good idea. It's better to have a well-structured test project that provides proper code coverage, testing all the possibilities and particularly corner cases. What happens if you set a filter extension containing separators '|' or ';'? How should it behave? Also creating the tests in a way that they're easy to copy/paste for a new platform. Just need to change what each setting should be doing.
Also we could create a Core namespace. All the classes and interfaces that the user needs would be in the root namespace, and internal implementations that don't need to referenced from code could be in the Core namespace. They would only be used for registration and for implementing custom factories.
Also logging may need to be reviewed. It seems it logs exactly the same thing for all without telling the dialog type.
DialogLogger.Write($"Title: {settings.Title}");
Also looking at the very last comment of this thread. Avalonia will soon have mobile and WebAssembly support. Those cases may require a separate Avalonia dialogs implementation, can see to that later.
About FolderBrowserDialog, are there cases where RootFolder does something that you can't do with SelectedPath? Also what should be the behaviour when opening multiple dialogs, keep the previously selected path or reset? This needs to be tested.
@mysteryx93 , yep, will contribute soon.
I cleaned up the settings classes.
ShowDialogAsync in WPF was broken. Fixed it. ShowDialogAsync should works for standard windows (but just had an issue where default close button wouldn't work, need to check) and if you simply call .Result on it you get a deadlock as you might expect. Need to convert projects to Community Toolkit to make samples work with Async commands.
As for WPF framework dialogs, I haven't found any way to show the dialog as a task and continue doing other work in the background. You can see if you can find a way to do it, but otherwise, it's simply returning Task.FromResult() in a sync way.
I fixed several .NET Core demo projects randomly so that they compile.
BTW CustomPlaces only apply to old XP-style dialogs? Better delete it then. "Seems the only way to use 'Recent Places' is to keep the default settings." LOL
For MessageBox, I'm still hesitating whether to return bool? or MessageBoxResult. I've been going back and forth; but honestly it's easier to use with bool?.
@virzak when updating projects, you might as well add the app.manifest to have modern messagebox styles. I added it to 2 projects so far.
Good news, default button just got implemented in Avalonia.
MessageBox return value is now bool?, and you set DefaultValue as bool?. Avalonia now supports default value.
@mysteryx93 I'm getting a bunch of compilation errors.
In this line for example.
Have you considered making a PR or a Draft PR, so that we can enjoy the continuous integration that has been set up?
You need an Async Command, which isn't in Mvvm Light but is in Mvvm Community Toolkit.
I don't know anything about PR nor continuous integration.
Is your feature request related to a problem? Please describe.
MVVM does not have a dependency on WPF or any other presentation framework. This is true for both
MVVM-Light
and its successorMVVM Toolkit
MVVM Dialogs
codebase could be split between core (or base) package, which only relies on .Net Standard and presentation package(s) responsible for the presentation layer implementation.Describe the solution you'd like
Introduce a new project and NuGet package, which declares framework-agnostic types and interfaces such as
IModalDialogViewModel
Draft PR #176 demonstrates the general idea.
Describe alternatives you've considered Keeping dialogs away from the presentation layer could be achieved (maybe?) with a wrapper:
Copy and rename the interface
Implement dialog VM outside of presentation layer using
IModalDialogViewModelStandard
Wrap IModalDialogViewModelStandard inside a IModalDialogViewModel
IDialogTypeLocator
needs to associate a ViewModels from non-presentation assembly to Window in the presentation layer. Window could be wrapped in IWindow to overrideIWindow.DataContext
What's your opinion @FantasticFiasco ?