Mutable and subclassable version of ObservableGroup

[hansmbakker]

Fixes #3519

Removes two restrictions from ObservableGroup to make it usable in more situations. In my case, I wanted to be able to rename the groups (for this I needed a mutable key), and I wanted to add properties on the group level (I needed to unseal it)

• Removes the immutable restriction on the Key property and makes the Key property observable.
• Removes the sealed keyword from ObservableGroup

PR Type

What kind of change does this PR introduce?

• Feature

What is the current behavior?

ObservableGroup has an immutable Key property
ObservableGroup is not inheritable

What is the new behavior?

ObservableGroup has a mutable Key property
ObservableGroup is inheritable

PR Checklist

Please check if your PR fulfills the following requirements:

• Tested code with current supported SDKs
• Pull Request has been submitted to the documentation repository instructions. Link: Update documentation to match code change for ObservableGroup MicrosoftDocs/WindowsCommunityToolkitDocs#394
• Sample in sample app has been added / updated (for bug fixes / features) --> no update needed
  • Icon has been created (if new sample) following the Thumbnail Style Guide and templates --> not applicable
• Tests for the changes have been added (for bug fixes / features) (if applicable) --> not applicable
• Header has been added to all new source files (run build/UpdateHeaders.bat) --> not applicable
• Contains NO breaking changes

Other information

[ghost]

Thanks hansmbakker for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[Sergio0694]

Overall it looks good, though this change makes me wonder whether we should really consider moving all these types directly into the MVVM Toolkit, as in particular with this change, these are really just MVVM-specific collection types. I mean I really can't think of a reason why anyone would want to use these types (or even just ObservableCollection<T> too) if not in a MVVM application - the whole point of these collections is that they're observable, as they implement both INotifyPropertyChanged and INotifyCollectionChanged.

@michael-hawker With the MVVM Toolkit being added in the 7.0 release, should we consider just moving these types over there instead? I mean, all the rest of the base Microsoft.Toolkit has nothing related to MVVM, while the MVVM Toolkit is entirely dedicated to it - I feel like these observable collections types are a bit the odd ones out just being in the base package?

Thoughts? 😄

[vgromfeld]

Approved with @Sergio0694 suggestions 👍

[ghost]

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

[hansmbakker]

Thank you for the suggestions @Sergio0694 ! Those were really helpful.

I was not aware of those conventions; VS must have picked up my own project's editorconfig settings when I added the Microsoft.Toolkit project to my solution.

[hansmbakker]

With the MVVM Toolkit being added in the 7.0 release, should we consider just moving these types over there instead? I mean, all the rest of the base Microsoft.Toolkit has nothing related to MVVM, while the MVVM Toolkit is entirely dedicated to it - I feel like these observable collections types are a bit the odd ones out just being in the base package?

Totally agree.

[Sergio0694]

LGTM after these last 3 small changes 🚀

We can talk about possibly moving these APIs later on but the PR is alright as is, since all these APIs are aalready available in the base package anyway, so that's not an issue to merge this first.

[ghost]

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

[hansmbakker]

@Sergio0694

KeyChangedEventArgs static field declared at the top of the class

Is this a convention? Or another reason?

In https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/634e243809b964700d736bbb9bae1f7b8efa464a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs I see things above and below the constructor. In ObservableGroup everything was below the constructor?

[Sergio0694]

Looks great! 🚀

[ghost]

Hello @Sergio0694!

Because this pull request has the auto merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[Sergio0694]

Is this a convention? Or another reason?

Yup, generally speaking we follow this structure:

• Constants
• Static readonly fields
• Static fields
• Readonly fields
• Fields
• Events
• Constructors
• Destructors
• Static properties
• Static methods
• Properties
• Methods

Of course, there might be some very specific exceptions, eg. static throw helper methods should always just go at the bottom of a type, or fields that back an instance property with both a getter and a setter should be declared right above the corresponding property. I feel like following this pattern helps a lot to improve consistency and keep the code easier to read 😊
I believe this is also very close (or the same) as the standard ordering that can also be configured in StyleCop or Re#.

[hansmbakker]

@Sergio0694 thank you for the quick iterations and helpful comments!

[Sergio0694]

Of course, happy to help! 😄

Thanks for the PR! 🚀