michaellperry / Assisticant

MIT License
36 stars 19 forks source link

Binding XceedWPFToolket.CheckComboBox.SelectedItemsOverride to an (ObservableList exposed as IEnumerable) - Changing selection from UI does not seem to update dependent Computeds #29

Closed smasihemami closed 4 years ago

smasihemami commented 4 years ago

Hello Michael,

First of all, I should express my highest gratitudes and excitements towards using your absolutely awesome library. I'm greatly happy that such a library exists. My happiness is just beyond words.

I have an issue regarding binding XceedWPFToolkit.CheckComboBox.SelectedItemsOverride to an (ObservableList exposed as IEnumerable). When I bind XceedWPFToolkit.CheckComboBox.ItemsSource to an (ObservableList exposed as IEnumerable), it correctly shows the items. But on changing the selection from UI, Assisticant does not trigger an update on all the computeds (i.e. those properties depending on that observable list).

Previously, when using XceedWPFToolket.CheckComboBox together with Prism framework, I was binding XceedWPFToolkit.CheckComboBox.SelectedItemsOverride to (an ObservableCollection exposed as ICollection) and it was working fine.

I also tried to cast IEnumerable to ICollection but it didn't make any difference.

On top of this, I am wondering how to make sure a property depending on an ObservableList gets updated on a change in that ObservableList. I followed your great tutorials, but yet there seems to be still something missing in my mind.

Would highly appreciate if you be kind to shed some light on this matter.

smasihemami commented 4 years ago

Assisticant-CheckComboBox.zip

Here is the solution for reproducing the issue. There are 2 projects within the solution. One is called Assisticant.Sample which does not work. The other one is called Prism.Sample and it works.

michaellperry commented 4 years ago

I see the issue. The Assisticant wrapper creates an ObservableCollection for the view to subscribe to. It does not, however, subscribe to changes itself. It can't tell when the view adds or removes items.

The reasoning behind this was that IEnumerable properties would be treated as output-only. This is appropriate for linq expressions in the view model, where the resulting IEnumerable is not a collection.

What we could do is to switch based on the property type. IEnumerables will continue to be output-only. But ILists could be input-output. That is, after all, what the interface promises.

I think this change can be made without breaking existing apps. Any ILists that are currently data bound and working correctly are only using one-way data binding anyway. Enabling two-way data binding should not change their behavior.

Do you want to take a crack at this? To help get you started, you can see how the strategy for each property is selected in Assisticant\Metas\MemberSlot.cs. You can start with a CollectionSlot and modify it to pick up on CollectionChanged events. When those events are raised, call the Add and Remove methods of the source IList.

smasihemami commented 4 years ago

Thanks for the answer Michael, Here is the handler for collection change in CollectionSlot, to get the source collection update on a change from UI: _collectionChangedFromUIEventHandler = new NotifyCollectionChangedEventHandler((s, e) => { switch (e.Action) { case System.Collections.Specialized.NotifyCollectionChangedAction.Add: foreach (var item in e.NewItems) { _sourceCollection.Add(item); } break; case System.Collections.Specialized.NotifyCollectionChangedAction.Remove: foreach (var item in e.OldItems) { _sourceCollection.Remove(item); } break; case System.Collections.Specialized.NotifyCollectionChangedAction.Replace: case System.Collections.Specialized.NotifyCollectionChangedAction.Move: case System.Collections.Specialized.NotifyCollectionChangedAction.Reset: default: break; } }); But it's not clear to me how to instruct Assisticant to update the dependents afterwards. I guess I'm missing the big picture.

Could you give some more inputs on how to make this happen?

CollectionSlot.zip

smasihemami commented 4 years ago

Assisticant-master.zip

Here is the whole solution including Assisticant.Sample project in which I have some dependent properties on an ObservableList.

michaellperry commented 4 years ago

Thanks for taking this on.

First, I'd like to have the binding to an IList as a separate kind of slot, leaving CollectionSlot unchanged. But I can still answer your questions in this context.

The _sourceCollection list is a different instance than the one that comes from the view model. Notice the code in UpdateValue that copies the collection into a new list _sourceCollection = source.OfType<object>().ToList(). Therefore changing this instance will not affect the original collection. The slot will need to get the original list as an IList and not make a copy.

Once this is in place, then changing the original IList should have the desired effect. Presumably, that IList is actually an ObservableList, and so adding and removing items will cause dependents to go out of date.

Go ahead and create a new branch and start a pull request. It will be easier to view and discuss code changes in a PR than in attached zip files.

smasihemami commented 4 years ago

I've just made a pull request Michael.

smasihemami commented 4 years ago

Again thanks for your time and efforts Michael. Highly appreciate it.

smasihemami commented 4 years ago

On adding/removing items to an ObservableList, it seems to cast items to PlatformProxy which leads to InvalidCastException if the item type is not a primitive type. In other words, the items passed through NotifyCollectionChangedAction are of type Assisticant.Descriptors.PlatformProxy while items in the _sourceCollection in ListSlot are of type T.

Do you have a guess on possible workarounds Michael?

smasihemami commented 4 years ago

For now, the issue seems to have been solved by changing the following 2 lines in ListSlot. I will create another pull request a bit later:

Old: _sourceCollection.Add(item);

New: var itemUnwrapped = (item as ViewProxy)?.Instance; _sourceCollection.Add(itemUnwrapped ?? item);

And the same for Remove.

michaellperry commented 4 years ago

I've applied the fix and updated the package. Thanks again!