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

RemoveMany from a merged SourceList causes Clear #10

Open
Nintynuts opened this issue Apr 24, 2024 · 3 comments
Open

RemoveMany from a merged SourceList causes Clear #10

Nintynuts opened this issue Apr 24, 2024 · 3 comments

Comments

@Nintynuts
Copy link

Nintynuts commented Apr 24, 2024

I'm guessing it's due to some optimisation, but this test fails, and the console output shows a clear.

[TestClass]
public class DynamicDataIssues
{
	private interface ICommonInterface { }
	private struct A : ICommonInterface { }
	private class B : ICommonInterface { }

	[TestMethod]
	public void RemoveAllFromMerged()
	{
		A[] fixedItems = { new(), new() };
		SourceList<B> moreItems = new();

		var disposable = fixedItems.AsObservableChangeSet(completable: true).Transform(i => (ICommonInterface)i)
			.Merge(moreItems.Connect().Transform(i => (ICommonInterface)i))
			.Do(x => Console.WriteLine($"+{x.Adds}, -{x.Removes}, {x.First()}"))
			// The sorting is to keep things in the order I merged them, but I would prefer to not need to do this.
			.Sort(SortExpressionComparer<ICommonInterface>
				.Ascending(i => i is not A)
				.ThenByAscending(i => i is not B))
			.Bind(out var allItems)
			.Subscribe();

		Assert.AreEqual(2, allItems.Count);

		// These work as expected
		moreItems.Add(new B());
		moreItems.Add(new B());

		// But these result it the fixed items being removed
		moreItems.RemoveMany(moreItems.Items.ToArray());

		Assert.AreEqual(2, allItems.Count);

		Assert.AreEqual(2, allItems.OfType<A>().Count());
	}
}

I've worked around the issue by removing items gradially rather than all in one go, but if I do a .Clear() the whole merged collection is cleared, which is also wrong. Is there more correct way to do this?

Thanks

Update: seems likely to be coming from here:

https://github.com/reactivemarbles/DynamicData/blob/76fd915924fab0e6756038f50a4fff4464a4ed00/src/DynamicData/List/ChangeAwareList.cs#L139

Is there a better way to prevent this than to remove all items individually?

@Nintynuts
Copy link
Author

If this is the wrong place for this, I'm sorry. I can create it somewhere else.

@JakenVeina
Copy link

JakenVeina commented May 1, 2024

Sorry, activity on this particular repo doesn't pop up in my feed, and probably the same for @dwcullop. And @RolandPheasant has been on vacation.

No need to move, now that it's here, but yeah, in the future, https://github.com/reactivemarbles/DynamicData is the best spot for submitting issues.

The problem is that you cannot use .Merge() on DynamicData streams, and it's for exactly this kind of reason. DynamicData collections are only "logical" collections, built up from a stream of changes made. When you create two separate logical collections, and then just merge all the changesets together, without any consideration for which of the two logical collections they belong to, you end up with a corrupted downstream collection. You need to use one of the DynamicData .MergeXXX() operators, which will maintain that awareness. I.E. when a changeset comes in that says to clear one of the two source collections, it will recognize that the downstream "merged" collection is logically built from more than just that one source, and translate the upstream changeset into a changeset that will correctly-preserve other items in the downstream collection.

At a glance, what you probably want is...

namespace DynamicData;

public static class ObservableListEx
{
    public static IObservable<IChangeSet<TObject>> MergeChangeSets<TObject>(
            this    IObservable<IChangeSet<TObject>>    source,
                    IObservable<IChangeSet<TObject>>    other,
                    IEqualityComparer<TObject>?         equalityComparer    = null,
                    IScheduler?                         scheduler           = null,
                    bool                                completable         = true)
        where TObject : notnull;
}

@Nintynuts
Copy link
Author

Ah, great. I suspected there would be something like that.

I was trying to find relevant examples in the various places you have them, but couldn't see anything similar to what I was trying to do (I guess because I was doing the wrong thing). It's great that you have examples, but they're a bit fragmented and it seems like there are some holes. Also, it's not clear when should or shouldn't use something as in this case. When should I use Merge rather than the suffixed method you mentioned? Could it be an overload which does the right thing based on the input types?

Another example I got even less far with was trying to join two sets of different data together into view models. I found the Join method, but I had no idea what to do with the duration delegates. I expected to have a left and right key selector then to need to transform them into my VMs.

It seems like this system should be really powerful and flexible, but I don't understand it well enough yet and need a crash course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants