1

Resolved

Wrong index in NotifyCollectionChangedEvent for add

description

If you bind a M2m collection (EntityCollection<TLinkTable, TEntity>) to a ListBox and then add a number of items to it (in codebehind/viewmodel) and then remove some entity in the middle of the list (in codebehind/viewmodel), then the wrong entity will be removed from the listbox.

The root of the problem is that EntityCollection will pass the wrong index to the NotifyCollectionChanged construction for the add event.

To fix the isse you should forward the NewStartingIndex/OldStartingIndex properties of the original NotifyCollectionChangedEventArgs in EntityCollection<,.MakeNotifyCollectionChangedEventArgs.
(se fix below)

I think the indexOfChangemember variable should removed, but if you are hesitant to do so you can use the following code which fixes the issue as well as verifies (in Debug mode) that the indexOfChange is correct.
    /// <summary>
    /// Replaces JoinType elements in NotifyCollectionChangedEventArgs by elements of type TEntity
    /// </summary>
    /// <param name="e"></param>
    /// <returns></returns>
    private NotifyCollectionChangedEventArgs MakeNotifyCollectionChangedEventArgs(NotifyCollectionChangedEventArgs e)
    {
        switch (e.Action)
        {
            case NotifyCollectionChangedAction.Add:
                {
                    TEntity entity = getEntity((JoinType)e.NewItems[0]);
                    indexOfChange = e.NewStartingIndex;
                    System.Diagnostics.Debug.Assert(this.IndexOf(entity) == e.NewStartingIndex);

                    return new NotifyCollectionChangedEventArgs(e.Action, entity, e.NewStartingIndex);
                }
            case NotifyCollectionChangedAction.Remove:
                {
                    TEntity entity = getEntity((JoinType)e.OldItems[0]);
                    //indexOfChange = e.OldStartingIndex; (keep setting index in remove untill we have tested this for a while)
                    System.Diagnostics.Debug.Assert(indexOfChange == e.OldStartingIndex);
                    return new NotifyCollectionChangedEventArgs(e.Action, entity, e.OldStartingIndex);
                }
            case NotifyCollectionChangedAction.Reset:
                return new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset);
        }

file attachments

comments

danneesset wrote Aug 28, 2012 at 10:24 AM

Update:

Entities are removed using code similar to ViewModel.SelectedUser.Roles.Remove(listbox.SelectedItem)
where listbox.ItemsSource = ViewModel.SelectedUser.Roles

MdeJ wrote Aug 29, 2012 at 7:50 AM

Hi,
Thanks for reporting this issue and for providing a solution. Would it be possible to also send me a small, but working, example that shows the error?
Thanks again,
Merijn

danneesset wrote Aug 30, 2012 at 9:00 AM

I've downloaded the M2MRiaDemo (using the newly created M2M4RIAv1_BugFixes) branch and was able to reproduce it.

Steps to reproduce.
Add 3 dogs and save the changes (so you have dogs 1,2,3,4).
Add dogs 1,2,3,4 (in the specified order) to trainer 2 by using the "AddDog" button
Select "Dog : 3" in the lower right list box "Trainer.Dogs"
Click DeleteDog, it will now remove "Dog : 2" and not the expected "Dog : 3" from the listbox (the listbox belives that the items come on the wrong order since it gets notified that they are inserted first when they in reality is inserted last in the list).

The only modification I did was to add a textbox (which display) the actually selected "Trainer.Dog" on the MainPage. I don't think this affects the outcome, but I attach the modified file anyway.

danneesset wrote Aug 31, 2012 at 1:32 PM

Update:
Don't include the assert in your code since it can fail when loading entities (the join table can be created before the entity is created, meaning that entity can be null).

I'm not sure if that case need some special treatment so that we don't raise notifycollectionchanges for them until they are loaded, or if we should raise a "add" notify collection changed event and later on raise a "replace" notify collection changed once they are availible (and the "entity" change from null to a real entity). At the moment it seems like it works fine anayway.

MdeJ wrote Sep 1, 2012 at 7:41 AM

Thanks for the additional information.

I'm no longer using the indexOfChange for the Remove and Insert NotificationCollectionChanged events. In stead, I now use e.OldStartingIndex and e.NewStartingIndex indexes, respectively. This seems to work perfectly.

Could you let me know if this change fixes your problem? If it does, I'll also apply this patch to the M2M4RIA-v2.

Thanks

danneesset wrote Sep 3, 2012 at 6:33 AM

I'm no longer using the indexOfChange for the Remove and Insert NotificationCollectionChanged
events. In stead, I now use e.OldStartingIndex and e.NewStartingIndex indexes, respectively.
This seems to work perfectly.
That change works perfectly fine.

MdeJ wrote Apr 18, 2014 at 3:20 PM

Fixed in changeset 3b8007c77e595a24b452d1e593be7535396ea65e