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

Issues with ReactiveLists and Xamarin Forms ListView showing new items #806

Closed
PureWeen opened this Issue Feb 11, 2015 · 41 comments

Comments

Projects
None yet
@PureWeen
Copy link
Contributor

PureWeen commented Feb 11, 2015

I have a Xamarin Forms project with a ListView bound to a ReactiveList... After a web call I saturate the list with items... On Android it works great and the items show up but on iOS it doesn't work....

I'm pretty positive it has something to do with the thread the items are being added from because if I add an item in the constructor of the view it shows up but then no subsequent items show up...

I've tried a bunch of scenarios but here's one I thought was interesting:
If I create an ObservableCollection, bind that to the ListView, then subscribe to the CollectionChangedEvent on the ReactiveList, then add items to that ObservableCollection as items are added to the ReactiveList then items show up..... If I change that same ObservableCollection to a ReactiveList then the items no longer show up

Hopefully that makes sense...

It's actually kind of frustrating because I've been trying to create the problem in a smaller project to send you but for some reason in that project everything works super :-(

I've tried as many different permutations of awaits/tasks/ObserveOns that I can but each project doesn't want to budge with how it works/doesn't work on iOS

If that all doesn't trigger anything I can probably take the defunct project and strip things away until I hopefully have something reproduceable... But I was just curious if something in there sounded familiar...

@detroitpro

This comment has been minimized.

Copy link
Contributor

detroitpro commented Feb 11, 2015

👍 I am running into the same problem.

@chillbythewater

This comment has been minimized.

Copy link

chillbythewater commented Feb 23, 2015

The problem is with the ListView not refreshing the layout (at least in my case). The items are added to the ListView but they don't appear until you do something like rotate the device. See: http://forums.xamarin.com/discussion/18631/listview-binding-to-observablecollection-does-not-update-gui
I tested this using a ReactiveList instead of an ObservableCollection and sure enough there are the missing items.

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 24, 2015

Alright I think I was able to create an extremely contrived example that seems to demonstrate the behavior (I realize the clearing of the list makes no sense but it's needed to recreate the behavior in this App)

https://github.com/PureWeen/XamarinFormsiOSListViewReactiveList

--code behind adding to the Lists
https://github.com/PureWeen/XamarinFormsiOSListViewReactiveList/blob/master/TheApp/TheApp/Page.xaml.cs

So when I run this project on iOS the ListView bound to the ReactiveList stops displaying items that have been added after the 4th item .... When I run this same project on Android the ListView keeps displaying added items.

 //uncomment this to use an ObservableCollection instead and the items will show up in the list
            //without a problem
            //ObservableCollection<Monkey> Monkies = new ObservableCollection<Monkey>();
            ReactiveList<Monkey> Monkies = new ReactiveList<Monkey>();

If I replace the ReactiveList with a straight ObservableCollection then on iOS the ListView keeps displaying added items

If I remove the async/await from the subscribe

 await new SomeLibrary.Class1().SomeMethod();

Then it works fine on iOS as well

@chillbythewater

This comment has been minimized.

Copy link

chillbythewater commented Feb 24, 2015

Well re the
await new SomeLibrary.Class1().SomeMethod();
You are awaiting a non async method. Should be:

 public async Task<string> SomeMethod()
        {
            HttpClient client = new HttpClient();
            return await client.GetStringAsync("http://www.google.com");        
        }
@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 25, 2015

At the risk of derailing the intent of this thread :-) That's not true you await TASKS not "async methods"... Marking a method async just indicates to the compiler that this method is going to awaiting something so it's telling the compiler to compile the method using a state machine.. So that it can suspend and resume etc....

http://blogs.msdn.com/b/pfxteam/archive/2012/04/12/async-await-faq.aspx
"You’re telling the compiler that you want to be able to use the “await” keyword inside the method (you can use the await keyword if and only if the method or lambda it’s in is marked as async). In doing so, you’re telling the compiler to compile the method using a state machine, such that the method will be able to suspend and then resume asynchronously at await points."

all await is is the developer indicating ok I've reached a point in the code where now I need the result from this Task... That task may have been running to completion this whole time or it might not even have started running until you call await..

So in your rewrite the only difference is that you are awaiting the state machine inside the method where as the original code is initiating the state machine outside of the method... Which in this case it's basically the same thing. AND actually some might argue it's not a good idea to await inside methods like that because then you aren't giving the implementer the choice of when to execute the task...

For example in the HttpClient library... None of the methods even start executing until you call await because none of them initiate the tasks when you call them... This allows the developer to decide when the actual tasks are going to be executing opposed to having them just execute right when you get the task back

@chillbythewater

This comment has been minimized.

Copy link

chillbythewater commented Feb 25, 2015

Yeah I didn't go too much into the semantics, just immediately applied the "async all the way" principle. Either way my rewrite worked fine for me. ListView updated and continued to do so.

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 25, 2015

I don't think "async all the way" means "await" all the way .... I'm pretty sure it applies to "Don’t mix blocking and async code"
https://msdn.microsoft.com/en-us/magazine/jj991977.aspx
So in SomeMethod if I did

client.GetStringAsync("http://www.google.com").Wait();
//or
client.GetStringAsync("http://www.google.com").Result;

Then I violated async all the way. Just returning the Task doesn't make it blocking.

Either way I removed it from the sample to simplify the contrived sample and just awaited the HttpClient call itself.

The main detail I find interesting is that if I use an ObservableCollection it works but with a ReactiveList it doesn't

And thanks for pulling down the sample and running it :-) Really appreciate that!!

@chillbythewater

This comment has been minimized.

Copy link

chillbythewater commented Feb 25, 2015

These debates have all been done to death in many forums. My interpretation of Stephen Cleary's suggestions re async all the way is to write the code using async/await where possible. And per Paul Bett's advice re using Observable.Start instead of Task.Run etc. It all seemed to work for me on IOS. Rather than try to work out the semantics I've just used a more "natural" style. I'll run again later to verify.

@chillbythewater

This comment has been minimized.

Copy link

chillbythewater commented Feb 25, 2015

ReactiveList has an optional parameter in the constructor for a scheduled. If you omit it will default to main thread so all the observables should fire on the UI thread.

@chillbythewater

This comment has been minimized.

Copy link

chillbythewater commented Feb 25, 2015

Apologies. Missed the point a little with your comment re removing the async/await and it working. Indeed the reactivelist ListView stops responding at some point. I added the threadId to see what was being used in the events. With the async in the subscribe the thread does change. Change the async execution to the following:

System.Reactive.Linq.Observable.Interval (TimeSpan.FromSeconds (5))
                .SelectMany (async _ => {
                return await new SomeLibrary.Class1 ().SomeMethod ();
            })
                .SubscribeOn (RxApp.MainThreadScheduler)
                .Subscribe (x => {


                //var there = Monkies.ToList();
                //Monkies.Clear();
                //there.ForEach(y => Monkies.Add(y));

                Monkey george = new Monkey () { Name = string.Format ("Jungle{0}", i) };

                Monkies.Add (george);
                Console.WriteLine ("MOnkey added {0} to ReactiveList threadid {1}", i, Thread.CurrentThread.ManagedThreadId);
                i++;
            });

I also commented out the Monkies.Clear as it causes an exception in IOS sometimes. Unfortunately you have to remove each item in a loop. The above seems to work fine.

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 25, 2015

Rabble!! Rabble!! Rabble!! Rabble!! Rabble!! Rabble!! :-)

So I just want to emphasize "contrived" ... My main application doesn't use Clear or really look like this.. in fact my main application no longer uses Xamarin Forms and I have all of this working swimmingly with a ReactiveTableViewController.... I had posted this issue and felt responsible for trying to provide something useful to recreate beyond an anecdote... So one day during a particularly riveting gotomeeting I came up with this.

So yea....

It's really less about how to make this sample work and more about demonstrating the difference in behavior between two collections that seem like they should have the same affect on a ListView control... Observable Collection works great and Reactive List stops working .... That's all I was really going for...

I haven't seen the exception though that you refer to... I thought maybe that's what was causing it to stop but for me that exception never really comes about...

@chillbythewater

This comment has been minimized.

Copy link

chillbythewater commented Feb 26, 2015

I get that the example is contrived but obviously we need to look at the specific case to get to the bottom of the difference. Using SelectMany and ObserveOn instead of SubscribeOn in the pattern above at least ensures that the adds and events are all on the main thread. My own testing seemed to show that the ListView for ObservableCollection updated regardless of which thread was doing the inserts. The ListView for the ReactiveList intermittently stopped updating HOWEVER when I rotated the device in EVERY case the items then appeared in the ReactiveList ListView. There are reports on the Xamarin forums about ListView not updating when used with ObservableCollection. In these cases the rotation revealed the "missing" items.

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 26, 2015

So two things I've noticed...

  1. If I simplify down the template
  <ListView x:Name="theView" >

        </ListView>

Then it updates pretty far... After about the 14th or 16th item it stops. As I make the ItemTemplate more involved then it seems to work less and less...

  1. If I go into ReactiveList and I remove the "#if NET_45" compiler directive and all the Weak Event Handling so that it just uses the strong event binding then the ReactiveList works exactly the same as the ObservableCollection... So I think there's something with how
@delegate.DynamicInvoke(sender, args);

works in iOS that the ListView just stops getting the messages

Which all might have to do with what you're looking at with what thread things are firing on

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 26, 2015

If I run this on Android you can see when the WeakEventHandler gets garbage collected...

02-26 13:32:33.261 D/dalvikvm( 2007): GC_EXPLICIT freed 82K, 4% free 3629K/3772K, paused 2ms+0ms, total 12ms
02-26 13:32:33.265 D/Mono    ( 2007): GC_OLD_BRIDGE num-objects 81 num_hash_entries 93 sccs size 93 init 0.00ms df1 0.45ms sort 0.23ms dfs2 1.37ms setup-cb 0.01ms free-data 0.14ms links 12/12/12/1 dfs passes 186/105
02-26 13:32:33.265 D/Mono    ( 2007): GC_MINOR: (Nursery full) pause 17.00ms, total 17.43ms, bridge 17.30ms promoted 960K major 1248K los 2003K

After that runs then the ReactiveList stops getting messages.. I tried moving the list declarations outside of the loops so they are properties on the view itself but same thing happens..

So I'm not sure at this point if that's just how life goes or if there's a way to ensure those don't get garbage collected... I know I've had this issue before with Weak Event Handlers in SL as well

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 26, 2015

I added a branched version where I removed ReactiveUI from the equation and just pulled out the WeakEventHandler and it does the same thing :-)

https://github.com/PureWeen/XamarinFormsiOSListViewReactiveList/tree/removedreactiveui

I'll post that one over at xamarin see if they have any thoughts

@chillbythewater

This comment has been minimized.

Copy link

chillbythewater commented Feb 26, 2015

Nice work there! I'll have a fiddle and see what I can come up with.

@chillbythewater

This comment has been minimized.

Copy link

chillbythewater commented Feb 26, 2015

In my own issue which brought me here in the first place, I have a version of IReadonlyReactiveList which is backed by an IOS data store. This uses exactly the same event pattern.

@robfe

This comment has been minimized.

Copy link

robfe commented Mar 27, 2015

This hacky piece of code appears to be a viable workaround (because it creates a strong reference to the event handler) - however there's every possibility that this is now creating memory leaks:

    public class MakeTheReactiveListCollectionChangedEventStronglyReferencedConverter : IValueConverter
    {
        public object Convert(object value, Type targetType, object parameter, CultureInfo culture)
        {
            return new ReactiveListWrapper((INotifyCollectionChanged)value);
        }

        public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture)
        {
            throw new NotSupportedException();
        }

        public class ReactiveListWrapper : IList, INotifyCollectionChanged
        {
            readonly IList _list;

            public ReactiveListWrapper(INotifyCollectionChanged list)
            {
                _list = (IList)list;
                list.CollectionChanged += (sender, args) =>
                {
                    var handler = CollectionChanged;
                    if (handler != null)
                    {
                        handler(this, args);
                    }
                };

            }

            public event NotifyCollectionChangedEventHandler CollectionChanged;
            public IEnumerator GetEnumerator() { return _list.GetEnumerator(); }
            public void CopyTo(Array array, int index) { _list.CopyTo(array, index); }
            public int Count { get { return _list.Count; } }
            public bool IsSynchronized { get { return _list.IsSynchronized; } }
            public object SyncRoot { get { return _list.SyncRoot; } }
            public int Add(object value) { return _list.Add(value); }
            public void Clear() { _list.Clear(); }
            public bool Contains(object value) { return _list.Contains(value); }
            public int IndexOf(object value) { return _list.IndexOf(value); }
            public void Insert(int index, object value) { _list.Insert(index, value); }
            public void Remove(object value) { _list.Remove(value); }
            public void RemoveAt(int index) { _list.RemoveAt(index); }
            public bool IsFixedSize { get { return _list.IsFixedSize; } }
            public bool IsReadOnly { get { return _list.IsReadOnly; } }
            public object this[int index] { get { return _list[index]; } set { _list[index] = value; } }
        }

    }
@ghuntley

This comment has been minimized.

Copy link
Member

ghuntley commented Apr 16, 2015

Yep, ran into this today using ReactiveUI = 6.4.0.1 & Xamarin.Forms = 1.4.1.6349.

Bug report is accurate and can confirm reproduction w/iOS 8.3 in the simulator using a ReactiveList that is populated via subscription from the results returned by a ReactiveCommand.CreateAsyncTask:

Employees = new ReactiveList<EmployeesDto>();

RefreshEmployees = ReactiveCommand.CreateAsyncTask(async _ =>
{
    var response = await _apiService.UserInitiated.GetEmployees();

    var employees = response.Results;
    if (employees == null || employees.Count == 0)
    {
        // TODO: Convert to proper UserError.
        throw new Exception("No employees were returned by the API.");
    }

    return employees;
});

RefreshEmployees.IsExecuting.ToProperty(this, x => x.IsRefreshingEmployees, out _isRefreshingEmployees);

RefreshEmployees.Subscribe(results =>
{
    Employees.Clear();

    if (GroupEmployeesBySurname)
    {
        Employees.AddRange(results.OrderBy(x => x.EmployeeSurname));
    }
    else
    {
        Employees.AddRange(results.OrderBy(x => x.EmployeeGivenName));
    }
});



<?xml version="1.0" encoding="UTF-8"?>
<ContentPage 
        xmlns="http://xamarin.com/schemas/2014/forms" 
        xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" 
        x:Class="EmployeeDirectoryApp.Core.Views.EmployeesPage">

    <ContentPage.Content>
        <ListView x:Name="Employees" x:StyleId="Employees"
                x:IsPullToRefreshEnabled="True"
                x:GroupDisplayBinding="{Binding EmployeeGivenName}"
                x:GroupShortNameBinding="{Binding EmployeeGivenName}">
            <ListView.ItemTemplate>
                <DataTemplate>
                    <TextCell Text="{Binding EmployeeDisplayName}" Detail="{Binding EmployeeDisplayName}" />
                </DataTemplate>
            </ListView.ItemTemplate>
        </ListView>
    </ContentPage.Content>
</ContentPage>  

this.OneWayBind(ViewModel, vm => vm.Employees, v => v.Employees.ItemsSource);
this.OneWayBind(ViewModel, vm => vm.RefreshEmployees, v => v.Employees.RefreshCommand);
this.OneWayBind(ViewModel, vm => vm.IsRefreshingEmployees, v => v.Employees.IsRefreshing);

As noted by @chillbythewater the list WILL display correctly upon device rotation/redraw but the initial draw operation is failing.

@chillbythewater

This comment has been minimized.

Copy link

chillbythewater commented Apr 16, 2015

It's obscenely annoying, nice MVVM pattern and it doesn't work!

@ghuntley

This comment has been minimized.

Copy link
Member

ghuntley commented Apr 20, 2015

Pretty sure that this is a issue w/Xamarin.Forms. I changed my ItemSource from ReactiveList to a ObservableCollection and now do a foreach on the result to the collection (which admittedly is incredibly horrible)

ObservableCollection<EmployeesDto> Employees { get; }

RefreshEmployees.Subscribe(results =>
{
    Employees.Clear();

    // <hack>
    // ReactiveList + AddRange does not work, the following is a workaround
    // that uses an ObservableCollection instead. This does yield negative
    // UI performance. Issue https://github.com/reactiveui/ReactiveUI/issues/806
    var employees = results as List<EmployeesDto>;
    foreach (var employee in employees)
    {
        Employees.Add(employee);
    }
    // </hack>
}
@ghuntley

This comment has been minimized.

@PhilBMorris

This comment has been minimized.

Copy link

PhilBMorris commented Apr 29, 2015

My issue may not be related, but something similar occurs only with iOS when using a template to populate a listview.

One listview has three image cells and a single string cell. Android shows the ListView just fine, but on iOS only the string shows up. Alternate rows are also color coded, but no color shows on iOS. I used DisplayAlert call to verify that data really existed for each row, and saw that the data used in the DisplayAlert was now appearing in the ListView. So as an experiment, as the source List<> was being populated, for each item to be added to the List<>, string values for cell contents were assigned to a temporary (and entirely unused) string variable. Specifically I added the 'temp' variable:

items.Add( hotspot );
string temp = hotspot.SSID + "', " + hotspot.Source + "," + hotspot.Encryption + "," + hotspot.Padlock + "," + hotspot.RowColor.ToString( );

Hey Presto, the ListView was populated ok on iOS.

A second listview had items consisting of 2 strings. Again on iOS only the second string value was display and again no alternate color. Assigning the two string values and the color to a temporary and unused string variable again resulted in the ListView being displayed properly.

items.Add( ci );
string temp = ci.RegionName + "," + ci.CountryName + "," + ci.RowColor.ToString( );

No such issues were encountered on Android. BTW I'm still using Xamarin Forms version 1.3.5.6337 (version 1.4.x has bugs re popping Modal Dialogs that causes issues with my apps)

@michaeldimoudis

This comment has been minimized.

Copy link

michaeldimoudis commented May 11, 2015

In response to @ghuntley regarding using ObservableCollection, forcing you to use Employees.Add instead of Employees.AddRange... I actually changed the ItemSource back to a ReactiveList and used Employees.Add in a foreach loop rather than Employees.AddRange and this seemed to work... so maybe it is with ReactiveUI? I'll try dig further into this... this was with the latest ReactiveUI 6.5.0

@omares

This comment has been minimized.

Copy link

omares commented May 22, 2015

Hey does anyone found more details where this issue arises from? Is the only viable workaround to use obersablecollection and copy the date around as suggested by @ghuntley?

We stumbled upon this too in our android app :(

@armintelker

This comment has been minimized.

Copy link

armintelker commented May 22, 2015

Here is a example app that have maybe the same issue.
https://github.com/rebuy-de/reactivelist-xamarin

If you try to remove items from a ListView it works in some cases and on some cases it don't work.
This strange behavior you can see in the example.mov video.

@armintelker

This comment has been minimized.

Copy link

armintelker commented May 26, 2015

We have a solution for our application:

public class ReactiveObservableCollection<T> : ReactiveList<T> {

    public ObservableCollection<T> ObservableCollection { private set; get; }

    public ReactiveObservableCollection()
    {
        this.ObservableCollection = new ObservableCollection<T>();
        ItemsAdded.Subscribe(ObservableCollection.Add);
        ItemsRemoved.Subscribe((x) => ObservableCollection.Remove(x));
    }
}
public class ViewModel : ReactiveObject {
    [Reactive]
    public ReactiveObservableCollection<Item> Items { set; get; }

    public ViewModel() 
    {
         Items = new ReactiveObservableCollection<Item>() {
             ChangeTrackingEnabled = true
         };
    }
}
 <ListView ItemsSource="{Binding Items.ObservableCollection}" />
@Krumelur

This comment has been minimized.

Copy link

Krumelur commented Jun 20, 2015

Just a note: I have the feeling the problem you see here is simply the fact that INotifyCollectionChanged requires you to be on the UI context (unlike INotifyPropertyChanged where the event gets automatically invoked on the UI thread).

A possible solution is to use Xamarin.Forms.BindingBase.EnableCollectionSynchronization.

For references on the topic, see:

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Jun 21, 2015

Not sure if where this has ended up still relates to the original issue I was having but I'm pretty positive my initial issues has to do with the fact that ReactiveList uses a weakreference for collectionChanged and the ListView also internally uses a Weak Reference to watch for collection changed events... So basically it's a weak reference connecting to a weak reference that gets GC'd... In my case if I debug the ReactiveUI source code and breakpoint

internal class WeakHandlerList
....
 public void Purge()

Then after a Garbage Collection

Purge breaks and the connection between the WeakNotifyProxy in Xamarin core gets cleaned up

Here's a really basic sample I put together to illustrate the behavior.. I force the GC after 5 items are added just to get to the end result quicker but if you just let it run until a GC happens then the same result occurs

https://github.com/PureWeen/XamFormsListViewWeakProxy

I just copy and pasted the WeakHandler code out of ReactiveList and just emulated that portion of the ReactiveList for my sample

I also emailed the sample to Xamarin Support to see if they have any thoughts

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Jun 23, 2015

Xamarin support confirmed the behavior in my sample and it is being forwarded to the Xamarin Forms team for more information

fun fun

@omares

This comment has been minimized.

Copy link

omares commented Jun 23, 2015

@PureWeen Thanks for the the work and your update. Lets hope it gets fixed soon.

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Jun 26, 2015

So here it is in all its glory :-)

https://bugzilla.xamarin.com/show_bug.cgi?id=31415

Importance: Minor

In the meantime I think something along the lines of what @armintelker has is what you're stuck with doing... Or some variation of that...

From a ReactiveUI library perspective I think the only fix would be to create a modified version of the list when compiled for Xamarin.Forms that uses Strong References for the event subscription... I know in the current code base there's a compiler directive for NET_45 that uses Strong References when compiled for NET_45 so I'm thinking maybe this same problem existed against WPF? or some variation of the issue.. Not sure if the structure is in place to do that easily against Xam Forms and specifically Xam Forms....

Or there's the deceptively simple answer we haven't stumbled on yet :-)

@chillbythewater

This comment has been minimized.

Copy link

chillbythewater commented Jun 28, 2015

Nice one! Not sure about the "minor" importance however.

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Oct 29, 2015

Sadly I missed in the bug that the project support included changed my initial project a little so it was crashing...

I updated the ticket and also include a WINRT sample
Here's a link
http://1drv.ms/1GNbrUm

ReactiveList bound directly to a Xamarin Forms list (in my experience) is completely unusable in a WINRT scenario..... Just an FYI

You have to wrap the list in something otherwise it just never works

@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Apr 10, 2016

Bugzilla issue has finally been marked as confirmed 🎉

@ghuntley

This comment has been minimized.

Copy link
Member

ghuntley commented Apr 28, 2016

Now that Xamarin Forms is open-source, we could send in a PR? :-)

@hscharitzer

This comment has been minimized.

Copy link

hscharitzer commented Jun 30, 2016

I'd like to know if there has been any progress on this and/or the Xamarin.Forms PR ;)

@escamoteur

This comment has been minimized.

Copy link

escamoteur commented Oct 3, 2016

Hi,

I encountered the same problem today, this time on Android. The first time the page is displayed no items are displayed in the ListView, the second time the page is shown it works.
If I change it to an ObservableCollection it works just fine.

This is my ViewModel:

    [ImplementPropertyChanged]
    public class PublicApiViewModel : FreshBasePageModel
    {
        private IStreamingProvider streamingProvider;

        public ReactiveCommand<Unit, ItemViewModel> UpdateViewCommand { get; protected set; }

        public ObservableCollection<ItemViewModel> AlbumsList { get; set; } = new ObservableCollection<ItemViewModel>();

        public PublicApiViewModel()
        {

            streamingProvider = Locator.Current.GetService<IStreamingProvider>();

            // NewReleases
            UpdateViewCommand = ReactiveCommand.CreateFromObservable(()=> streamingProvider.GetNewReleases(10, 0)
                .Do(_ => AlbumsList.Clear())
                .ObserveOn(SynchronizationContext.Current)
                .SelectMany(albums => FlattenAlbumsList(albums)));

            UpdateViewCommand.Subscribe(x => AlbumsList.Add(x));



            //AlbumsList = UpdateViewCommand.CreateCollection();
            UpdateViewCommand.ThrownExceptions.Subscribe(OnError);

        }

        private IEnumerable<ItemViewModel> FlattenAlbumsList(IEnumerable<IAlbum> albums)
        {
            return albums.Select(album => new ItemViewModel() {FirstCol = album.Name});
        }

(I exatracted the FlattenMethod so that I could put a breakpoint in it)

I also tried creating the List as DerivedList via CreateCollection but there is was even more unpredictable if it worked or not.

Any new insights on this?

Best
Thomas

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Oct 3, 2016

Not really... I haven't tested this in Rx7 but I'm assuming the same "problem" still exists. The ticket (as you saw) on Xamarin's side is confirmed but it'll probably only get fixed from someone in the community doing a pull request.

Basically what I do is on the view binding I wrap the list with a list that creates a strong connection to the weakevent in the ListView


 this.OneWayBind(ViewModel, x => x.ViewModelProperty, x => x.ListView.ItemsSource,
                    selector: x =>
                    {

                        return new ObservableReactiveList<T>(x);

                    }
                );
public class ObservableReactiveList<T> :
        ReactiveObject,
        IEnumerable<T>, INotifyCollectionChanged, IDisposable, IEnableLogger
    {
        IReactiveCollection<T> _internalList;
        CompositeDisposable killMe = null;
        bool _ignoreCollectionChangedNotifications = false;
        public ObservableReactiveList() : this(new ReactiveList<T>())
        {

        }

        public ObservableReactiveList(IReactiveCollection<T> sourceList, bool ignoreCollectionChangedNotifications = false)
        {
            _ignoreCollectionChangedNotifications = ignoreCollectionChangedNotifications;
            _internalList = sourceList;
            killMe = new CompositeDisposable();
            killMe.Add(
                sourceList.Changed.ObserveOn(ImmediateScheduler.Instance).Subscribe(args => OnCollectionChanged(args))
            );
        }

        protected void OnCollectionChanged(NotifyCollectionChangedEventArgs args)
        {
            if (!_ignoreCollectionChangedNotifications)
            {
                if (CollectionChanged != null)
                    CollectionChanged(this, args);
            }
        }

        public event NotifyCollectionChangedEventHandler CollectionChanged;

        public IEnumerator<T> GetEnumerator()
        {
            return _internalList.GetEnumerator();
        }

        IEnumerator IEnumerable.GetEnumerator()
        {
            return _internalList.GetEnumerator();
        }

        public void Dispose()
        {
            killMe.Dispose();
            killMe = null;
        }
    }

It's not the most elegant thing in the universe and my collection is a wee bit specialized to my app but it works :-)

@armintelker has a solution earlier in the thread that's a bit simpler

@ghuntley ghuntley removed the vNext label Nov 6, 2016

@bratsche bratsche referenced this issue Nov 23, 2016

Merged

Fix memory leaks with events. #547

1 of 2 tasks complete

@ghuntley ghuntley changed the title Issues in iOS with ReactiveLists and Xamarin Forms ListView showing new items Issues with ReactiveLists and Xamarin Forms ListView showing new items Nov 23, 2016

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Feb 26, 2017

A few notes... The main thing I'm trying to figure out is can a weak reference connect to a weak reference and survive? It feels like two people hanging onto each other for support but nothing else :-p

This is the handler that gets collected
https://github.com/reactiveui/ReactiveUI/blob/master/src/ReactiveUI/WeakEventManager.cs#L274

If you make a local variable reference to that handler then everything "works" obviously because now you're violating the WeakReference

On the Xamarin forms side here's the WeakProxy which nothing retains a reference to
https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Core/ListProxy.cs#L43

And here's the Event Subscription that gets cleaned up because RxUI is only holding a weak reference to this handler which also nothing holds a reference to
https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Core/ListProxy.cs#L368

The ListProxy implementation on the Xam Forms side is aggressively weak... In order for it to work I think you'd have to retain a reference to WeakProxy on ListProxy and then change out how the event subscription is done so that the handler is retained and managed opposed to just relying on scope to manage it... Not quite sure if there's an acceptable way to do that. Acceptable being defined as retaining the same current safety while working with ReactiveList :-)

I'm not super well versed on ConditionalWeakTable but I'm curious if using those with the ListProxy as the key would achieve the same result

        ConditionalWeakTable<ListProxy, WeakNotifyProxy> sourceToWeakHandlers = new ConditionalWeakTable<ListProxy, WeakNotifyProxy>();
ConditionalWeakTable<ListProxy, NotifyCollectionChangedEventHandler> sourceToWeakHandlers = new ConditionalWeakTable<ListProxy, NotifyCollectionChangedEventHandler>();

When I do that in Xam Forms the eventhandler doesn't get cleaned up upon GC and it all works

I feel like that would be a safe way to go as the ListPRoxy is going to collect once the ListView goes out of scope which means the Value on the CWT will also clean up

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

PureWeen commented Apr 14, 2017

Tested with 2.3.5-pre1 Xamarin Forms and a ReactiveList
Items kept propagating after forcing lots of GCs

@PureWeen PureWeen closed this Apr 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment