RolandPheasant / DynamicData.ReactiveUI

Adaptors to integrate Dynamic Data with ReactiveUI
Microsoft Public License
63 stars 9 forks source link

Seeing duplication on the ReactiveList when using a filter before the bind #7

Closed PureWeen closed 6 years ago

PureWeen commented 6 years ago

Maybe I'm not understanding the interaction of bind quite correctly but I'm seeing items from the source collection being duplicated into the destination ReactiveList.

When I bind to an ObservableCollectionExtended that behaves as I'd expect it to

 [Test]
        public void DuplicatingResult()
        {

            _source.Add(new Person("Adult1", 47));
            _source.Add(new Person("Adult2", 48));
            _source.Add(new Person("Adult3", 49));
            _source.Add(new Person("Adult4", 150));
            _source.Add(new Person("Adult5", 150));
            _source.Add(new Person("Adult6", 150));
            _source.Add(new Person("Adult7", 150));
            _source.Add(new Person("Adult8", 150));

            Subject<int> ageTofilter = new Subject<int>();
            ObservableCollectionExtended<Person> nonReactiveListBinding = new ObservableCollectionExtended<Person>();

            var passengerFilter =
                ageTofilter
                    .Select(data =>
                    {
                        Func<Person, bool> filter = (twp) =>
                        {
                            return twp.Age == data;
                        };

                        return filter;
                    });

            _source
                .Connect()
                .Filter(passengerFilter)
                .Bind(nonReactiveListBinding)
                .Bind(_collection)
                .Subscribe();

            //this passes
            Assert.AreEqual(8, _collection.Count);

            //Change the filter
            ageTofilter.OnNext(150);

            //this passes
            Assert.AreEqual(5, nonReactiveListBinding.Count);

            //this fails with a result of 13. Items are being duplicated
            //into the reactivelist
            Assert.AreEqual(5, _collection.Count);

        }
RolandPheasant commented 6 years ago

That's odd and clearly wrong. What version off dd, dd.reactiveui and reactiveui are you using?

PureWeen commented 6 years ago

The master branch. All I did was pull down the master branch and write a unit test (above) into this file

https://github.com/RolandPheasant/DynamicData.ReactiveUI/blob/master/DynamicData.ReactiveUI.Tests/Fixtures/BindFromObservableListFixture.cs

namespace DynamicData.ReactiveUI.Tests.Fixtures
{
    [TestFixture]
    public class BindFromObservableListFixture
    {
        private ReactiveList<Person> _collection;
        private SourceList<Person> _source;
        private IDisposable _binder;
        private readonly RandomPersonGenerator _generator = new RandomPersonGenerator();

        [SetUp]
        public void SetUp()
        {
            _collection = new ReactiveList<Person>();
            _source = new SourceList<Person>();
            _binder = _source.Connect().Bind(_collection).Subscribe();
        }

        [TearDown]
        public void CleanUp()
        {
            _binder.Dispose();
            _source.Dispose();
        }

        [Test]
        public void DuplicatingResult()
        {
PureWeen commented 6 years ago

though now I'm seeing that creates a double binding :-/ let me check if my test is bad

PureWeen commented 6 years ago

Yea I did a bad job with that unit test... Let me see if I can recreate it better or if I'm just doing something wrong in my application. I was seeing this issue in my app with ReactiveList showing duplicate results in the Bound to reactive list

RolandPheasant commented 6 years ago

Also in the latest dd I have moved Rx UI integration into the main dynamic data code base, so it may be that what exists in this project is out of date.

I will update the the readme to reflect this change

PureWeen commented 6 years ago

Cool I'll work from there and see what I can recreate... Sadly unit test recreation isn't going so smooth so seeing if I can recreate in an android project

In my app basically I'm seeing this

If I use bind then I get duplicates in the ReactiveList

.ObserveOn(RxApp.MainThreadScheduler)
.Bind(SomeObservableCollectionExtended)
.Bind(SomeReactiveList); //this RL list gets duplicates

If I do it like this then everything works fine and ReactiveList doesn't get duplicates.

.ObserveOn(RxApp.MainThreadScheduler)
.Bind(SomeObservableCollectionExtended)
.Do(_ =>
{
     SomeReactiveList.Clear();
     SomeReactiveList.AddRange(SomeObservableCollectionExtended);

})

Hopefully I can easily recreate in an Android project :-)

RolandPheasant commented 6 years ago

I see the problem.

The idea behind the bind operator is that dd does everything for you and you are not supposed to do any manual maintainance of the list.

This is why in all dd examples I use read-only observable collection to bind to. The observable collection extended is a throwback from earlier versions of dd.

So my question is what are you trying to achieve with the double bind?

PureWeen commented 6 years ago

Mainly because of this issue https://github.com/reactiveui/ReactiveUI/issues/1345

I was first just binding to the ReactiveList but that was crashing on me for iOS because of that bug in ReactiveUI

so for now I stuck that bind into their and for iOS instead of allowing DynamicData to do a range add which breaks the ReactiveUI iOS CollectionSources. I do an item by item remove and add which interacts a lot better with the Reactive iOS UI components.

So basically this is what I have after accounting for iOS and cleaning it up a little by using a Readonly Collection

var activePassengerUpdates =
                Passengers
                    .Connect()
                    .DisposeMany()
                    .Filter(passengerFilter)
                    .Sort(SortExpressionComparer<CheckInRecordModel>.Ascending(p => $"{p.Priority}_{p.Passenger.LastName}_{p.Passenger.PassengerIdentifier}"))
                    .ObserveOn(RxApp.MainThreadScheduler);

if (ApplicationStateService.DeviceType == Device.iOS)
            {
                ReadOnlyObservableCollection<CheckInRecordModel> ActiveTripWaypointPassengers;
                activePassengerUpdates
                    .Bind(out ActiveTripWaypointPassengers)
                    .Do(_ =>
                    {
                        //SyncList just individually removes and adds items
                        ActiveTripWaypointPassengersRL
                            .SyncList(
                                ActiveTripWaypointPassengers,
                                x => x,
                                (x, y) => x == y,
                                null);

                    })
                    .Subscribe()
                    .DisposeWith(disposable);
            }
            else
            {
                activePassengerUpdates
                    .Bind(ActiveTripWaypointPassengersRL)
                    .Subscribe()
                    .DisposeWith(disposable);
            }

ActiveTripWaypointPassengersRL is what gets surfaced to the various reactive view sources and adapters.

This works great on iOS but on android my ReactiveList shows duplicate items from the source. If I change it to just use the SyncList one on Android then it works great on Android with no duplicate items

PureWeen commented 6 years ago

And in my app I'm still using all the version 4 of things

  <package id="reactiveui" version="7.4.0" targetFramework="monoandroid71" />
  <package id="DynamicData" version="4.15.0.2299" targetFramework="monoandroid71" />
  <package id="DynamicData.ReactiveUI" version="2.5.0.2009" targetFramework="monoandroid71" />
PureWeen commented 6 years ago

Going to close this for now. I haven't been able to create in a sample project with the latest code from the other repo. I'm still seeing the issue in my app but haven't been able to separate whether it's a me thing or a this library thing. Once I'm able to update my project to all the latest Rx netstandard bits I'll try this again and create an issue over there if applicable