Remove from transform many with projection

[dchaib]

What kind of change does this PR introduce?
Bug fix for #264

What is the current behavior?
When removing an item from a SourceList with a TransformMany transfomartion using a projection, the items are never removed from the "output". See #264 for more details.

What is the new behavior?
IChangeSet now has an equality comparer that can be used when looking for items to remove.

What might this PR break?
Hopefully nothing!

Please check if the PR fulfills these requirements

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

Other information:
This is a draft. If I'm headed in the right direction, I'll refactor and add proper comments when needed.
It might also be necessary to extend the usage of the newly added equality comparer for consistency and maybe fixing other cases.

[RolandPheasant]

I think I have worked it out. In TansformMany.cs you need to change the following code:

return _source.Transform(item => new ManyContainer(_manyselector(item).ToArray()), true)
	.Select(changes => new ChangeSet<TDestination>(new DestinationEnumerator(changes, _equalityComparer), qualityComparer))
	.NotEmpty();

to

return Observable.Create<IChangeSet<TDestination>>(observer =>
    {
       //NB: ChangeAwareList is used internally by dd to capture changes to a list and ensure they can be replayed by subsequent operators
        var result = new ChangeAwareList<TDestination>();

        return _source.Transform(item => new ManyContainer(_manyselector(item).ToArray()), true)
            .Select(changes =>
            {
                var destinationChanges = new ChangeSet<TDestination>(new DestinationEnumerator(changes, _equalityComparer));
                result.Clone(destinationChanges, _equalityComparer);
                return result.CaptureChanges();
            })

            .NotEmpty()
            .SubscribeSafe(observer);
    }
);

Also ListEx.Clone should be changed to

void Clone<T>(this IList<T> source, IChangeSet<T> changes, IEqualityComparer<T> equalityComparer = null)

and inside that method change

Clone(source,  item, changes.EqualityComparer);

to

Clone(source,  item, equalityComparer ?? EqualityComparer<T>.Default);

With these changes, your unit tests pass, which means you can remove the equality comparer from the change set

[dchaib]

Awesome! I'll revert my commit and proceed with your proposal! Thanks a lot!

[dchaib]

I have added an overload instead of an optional parameter because of https://github.com/dchaib/DynamicData/blob/e4e0327fd38c909f293e1bd9731049fdd78a73e0/src/DynamicData/List/ObservableListEx.cs#L1246
Let me know if I should fix this differently!

[dchaib]

Should equalityComparer be used in more cases in the Clone method? For example, it might be needed for ListChangeReason.RemoveRange.

Should the equality comparer be passed in more cases to the Clone method? Maybe for TransformMany in CreateWithChangeset:

• https://github.com/dchaib/DynamicData/blob/e4e0327fd38c909f293e1bd9731049fdd78a73e0/src/DynamicData/List/Internal/TransformMany.cs#L141
• https://github.com/dchaib/DynamicData/blob/e4e0327fd38c909f293e1bd9731049fdd78a73e0/src/DynamicData/List/Internal/TransformMany.cs#L149

GitHub

dchaib/DynamicData

Reactive collections based on Rx.Net. Contribute to dchaib/DynamicData development by creating an account on GitHub.

GitHub

dchaib/DynamicData

Reactive collections based on Rx.Net. Contribute to dchaib/DynamicData development by creating an account on GitHub.

[RolandPheasant]

Yes, good spot

[glennawatson]

I would recommend incrementing the number of the minor in version.json in the root directory before merging this commit

[dchaib]

@RolandPheasant do you know how I can trigger ListChangeReason.RemoveRange and a call to RemoveMany? I'm trying to add a unit test to cover this.

[RolandPheasant]

RemoveMany is not produced by the transform many operator

[dchaib]

Thanks, that explains why I couldn't figure out how to produce it. :)

Then, I guess I'm done as far as I'm concerned.

[glennawatson]

Just bump the version.json minor version would be the last thing

[RolandPheasant]

I agree with @glennawatson about the minor version. Please bump then then I will merge this PR

[dchaib]

Done!