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

Move Catel.MVVM collections to Catel.Core collections #1507

Open
bigworld12 opened this issue Dec 25, 2019 · 8 comments
Open

Move Catel.MVVM collections to Catel.Core collections #1507

bigworld12 opened this issue Dec 25, 2019 · 8 comments
Labels
Milestone

Comments

@bigworld12
Copy link
Contributor

@bigworld12 bigworld12 commented Dec 25, 2019

Summary

currently we can't use ObservableDictionary or FastObservableCollection or any collection defined in Catel.MVVM in a .net standard project

API Changes

move all the collections to Catel.Core project

Intended Use Case

using the collections outside of .net core applications

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Dec 25, 2019

I don't agree with moving either ObservableDictionary or FastObservableCollection to Catel.Core. Aside from the fact it would be a major breaking change, it doesn't fit the scope of what Catel.Core tries to fill.

The issue at hand is that Catel.MVVM targets netcoreapp3.0 because it's technically a WPF User Control Library, not a Class Library. This is why you can't reference it in a .NET Standard library because they cannot link back that way (it would be cyclical). You would run into the same issue with Telerik as well since they also target this way (if you use them).

Your library would essentially have to target netcoreapp3.0/3.1 in order to use Catel.MVVM at a lower level.

A possible idea if @GeertvanHorrik could chime in would be to take a similar direction to what Microsoft has been doing with their libraries and start breaking them out into foundational libraries that could then do their minimal framework targeting. So for instance Catel.MVVM.Collections would be it's own library but targeting .NET Standard 2.0 (which has native support for .NET Framework 4.7), .NET Standard 2.1 (.NET Core 3), .NET Framework 4.6 and .NET Framework 4.8 (there is not native support for .NET Core 3 for this afaik).

@bigworld12

This comment has been minimized.

Copy link
Contributor Author

@bigworld12 bigworld12 commented Dec 25, 2019

ObservableDictionary and FastObservableCollection are simply data structures, the only thing used in Catel.MVVM is the IDispatcherService.
That means we can create 2 variants of every data strcture
e.g.
ObservableDictionary,DispatcherObservableDictionary
FastObservableCollection , DispatcherFastObservableCollection
i am in the process of creating a PR for the changes

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Dec 25, 2019

I still believe this is a non-trivial endeavor. FastObservableCollection depends on other components in Catel.MVVM such as SuspensionContext<T> which is also used by other components. Additionally ObservableDictionary needs to be fixed to use the same thing (see #1506) due to an outstanding bug.

The amount of concerns that have to be moved to Catel.Core from Catel.MVVM means this probably wouldn't occur until 6.x because of the breaking nature of it.

@bigworld12

This comment has been minimized.

Copy link
Contributor Author

@bigworld12 bigworld12 commented Dec 25, 2019

since SuspensionContext<T> doesn't depend on anything in Catel.MVVM , i also moved it to Catel.Core

also , yes this is a breaking change, since e.g. users of the previous FastObservableCollection will need to rename it to DispatcherFastObservableCollection to maintain previous functionality.

i will also be fixing the suppression bug for ObservableDictionary

@bigworld12 bigworld12 mentioned this issue Dec 25, 2019
0 of 5 tasks complete
@bigworld12

This comment has been minimized.

Copy link
Contributor Author

@bigworld12 bigworld12 commented Dec 25, 2019

also big thank you @vaecors I love your ObservableDictionary implmentation

@GeertvanHorrik

This comment has been minimized.

Copy link
Member

@GeertvanHorrik GeertvanHorrik commented Dec 25, 2019

Vnext will be 6.0 (unless hotfixes), so we could consider this. I am not sure about splitting up into lots of libraries, could over complicate the solution and management.

Current development branch represents 6.0. Will apply most of the simple changes such as breaking changes somewhere this week.

The collections can be moved to core, and we could keep the dispatchable versions in Catel.mvvm

@GeertvanHorrik GeertvanHorrik added this to the 6.0.0 milestone Jan 13, 2020
@GeertvanHorrik

This comment has been minimized.

Copy link
Member

@GeertvanHorrik GeertvanHorrik commented Jan 20, 2020

The hotfixes have been released, we can start thinking about this again.

@stale

This comment has been minimized.

Copy link

@stale stale bot commented Mar 20, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Mar 20, 2020
@GeertvanHorrik GeertvanHorrik added planned and removed wontfix labels Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.