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

[Question] how to show added children in grouped collection // workaround for ObservableGroup immutable key restriction? #3519

Closed
hansmbakker opened this issue Oct 4, 2020 · 10 comments · Fixed by #3526
Labels
Completed 🔥 help wanted Issues identified as good community contribution opportunities helpers ✋ improvements ✨
Milestone

Comments

@hansmbakker
Copy link
Contributor

hansmbakker commented Oct 4, 2020

I'm trying to build an app that allows modifying a grouped collection (nested ObservableCollection) in a ListView. I am showing the grouped collection in the ListView using a CollectionViewSource.
I am not yet using the ObservableGroup / ObservableGroupedCollection yet, my question is whether they are a viable option.

I would like to achieve the following:

  • edit the group names
  • edit the individual items
  • add / remove new groups
  • add / remove individual items

Imagine this (simplified example): having an ObservableCollection<Book>, and every Book has a Title (string) and an ObservableCollection<Uri> for some images.
I want to add books, edit their title, and also add Uris of images and edit them.

  • Inside my ListView, I have
    • a GroupStyle with a HeaderTemplate containing
      • TextBox to edit the Titles
      • a Button to insert new Uri items --> here is my issue
    • an ItemTemplate containing a TextBox for the image Uris
  • outside my ListView I have a Button to add new Book items to the ObservableCollection<Book>

With my current solution (ObservableCollection<Book> with Title and ObservableCollection<Uri> inside Book), I can edit Book Titles and Uris, but when I add a new Uri, the ListView does not show it because the CollectionViewSource does not notice it, because the ObservableCollection<Book> does not emit a CollectionChanged event.

This is being discussed in in this StackOverflow item as well.

Then I found ObservableGroup / ObservableGroupedCollection from Windows Community Toolkit, but I'm unsure whether that will solve my issue? Since the Key is immutable I assume they won't let me edit the Book Titles?

@hansmbakker hansmbakker added the question ❔ Issues or PR require more information label Oct 4, 2020
@ghost ghost added the needs triage 🔍 label Oct 4, 2020
@ghost
Copy link

ghost commented Oct 4, 2020

Hello hansmbakker, thank you for your interest in Windows Community Toolkit!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible.. Other community members may also answer the question and provide feedback 🙌

@hansmbakker hansmbakker changed the title [Question] how to add children to grouped collection // workaround for ObservableGroup immutable key restriction? [Question] how to show added children in grouped collection // workaround for ObservableGroup immutable key restriction? Oct 4, 2020
@Kyaa-dost
Copy link
Contributor

@hansmbakker that's a good question. Let me loop in the team and see if anyone will be to assist you with this or tell you if it's feasible with ObservableGroup or ObservableGroupCollection.

@Kyaa-dost Kyaa-dost added the help wanted Issues identified as good community contribution opportunities label Oct 5, 2020
@hansmbakker
Copy link
Contributor Author

Thank you!
As a workaround, I see this probably can be built with nested ListViews, or with a custom ItemsRepeater, but I was wondering what the "proper" way of doing this is.
So I'm curious to hear what they will say.

@hansmbakker
Copy link
Contributor Author

hansmbakker commented Oct 5, 2020

Also, maybe good to mention @vgromfeld who created these types originally :)
(#3201)

@vgromfeld
Copy link
Contributor

@hansmbakker ObservableGroup<TKey, TValue> seems to be a good choice since it provides almost all you need.
The only missing part is to make the ObservableGroup<TKey, TValue>.Key property mutable.
This should be easy to change. ObservableGroup<TKey, TValue> is already inheriting from INotifyPropertyChanged so if you change the Key property to read/write, you can directly raise the property changed event and have the behavior you want.

@hansmbakker
Copy link
Contributor Author

@vgromfeld thank you for your response!

Are you suggesting to modify the classes in the WCT?

If so, I was wondering whether the read-only setting was meant for a particular requirement or behaviour?

@vgromfeld
Copy link
Contributor

@hansmbakker Yes, you can modify the toolkit classes.

The Key property is read-only because I didn't need it to be writable when I implemented it 😁.

@michael-hawker michael-hawker added helpers ✋ improvements ✨ and removed needs triage 🔍 question ❔ Issues or PR require more information labels Oct 6, 2020
@michael-hawker michael-hawker added this to the 7.0 milestone Oct 6, 2020
@michael-hawker
Copy link
Member

@hansmbakker thanks for raising the issue, did you also want to submit a PR for this change?

@hansmbakker
Copy link
Contributor Author

Looking at it in more detail now, I see that the ObservableGroup and ObservableGroupedCollection types are sealed.

I want to base two viewmodel types on them, but because of them being sealed I cannot extend them.

Were there any design considerations to add this restriction?

@hansmbakker
Copy link
Contributor Author

hansmbakker commented Oct 6, 2020

@michael-hawker I'm providing #3526 as-is: I'm willing to contribute the changes that unblocked me, and I updated the documentation for it, but I feel not confident in changing the larger picture of this as I am not aware of design considerations that were there when these classes were added.

For example I left ObservableGroupedCollection sealed. Also, I did not update the IReadOnlyObservableGroup because the name suggests it should be immutable.

I want to avoid getting involved in a long-running PR process / ping-ponging this back and forth between me and others; however maintainers have access to the branch in case they want to make modifications to it.

@ghost ghost closed this as completed in #3526 Oct 7, 2020
@ghost ghost removed the In-PR 🚀 label Oct 7, 2020
ghost pushed a commit that referenced this issue Oct 7, 2020
## Fixes #3519
<!-- Add the relevant issue number after the "#" mentioned above (for ex: Fixes #1234) which will automatically close the issue once the PR is merged. -->

<!-- Add a brief overview here of the feature/bug & fix. -->
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?
<!-- Please uncomment one or more that apply to this PR. -->

<!-- - Bugfix -->
- Feature
<!-- - Code style update (formatting) -->
<!-- - Refactoring (no functional changes, no api changes) -->
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
`ObservableGroup` has an immutable `Key` property
`ObservableGroup` is not inheritable


## What is the new behavior?
<!-- Describe how was this issue resolved or changed? -->
`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](../readme.md#supported)
- [x] Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: MicrosoftDocs/WindowsCommunityToolkitDocs#394
- [x] Sample in sample app has been added / updated (for bug fixes / features) --> _no update needed_
    - [x] Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets) --> _not applicable_
- [x] Tests for the changes have been added (for bug fixes / features) (if applicable) --> _not applicable_
- [x] Header has been added to all new source files (run *build/UpdateHeaders.bat*) --> _not applicable_
- [ ] Contains **NO** breaking changes

<!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. 
     Please note that breaking changes are likely to be rejected. -->


## Other information
@ghost ghost added the Completed 🔥 label Oct 7, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 6, 2020
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Completed 🔥 help wanted Issues identified as good community contribution opportunities helpers ✋ improvements ✨
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants