search

MvvmCross / MvvmCross-AndroidSupport

Android support library packages for MvvmCross: The .NET MVVM framework for cross-platform solutions.
http://mvvmcross.com
15 stars 0 forks source link

Nested MvxRecyclerViews display outdated (cached) Views when "Refreshing" the inner data-bound collection #280

Closed Strifex closed 7 years ago

Strifex commented 8 years ago

After databinding an observable collection to a nested MvxRecyclerView, adding to that list, clearing that list, and then adding to the new list, some items show outdated information. I suspect a problem in the adapter or viewholder. It appears to be hanging onto old information. I cannot "refresh" my list atm as it will display outdated information if I try (keep in mind the underlying databound object is correct, it appears just the View itself is displaying old information). Its also possible I messed up when "overriding" the MvxRecyclerView class to disable nested scrolling....

Repository with reproducible issue: https://github.com/Strifex/MvxRecyclerViewTest

Steps to reproduce

  1. Build and Run example project in repository.
  2. Click "Add" button a few times for Group 1.
  3. Click on multiple users to remove them from list (especially the first 2-3 in list)
  4. Hit Refresh Button
  5. The views will either be blank or re-use a previous view after refresh.

    Expected behavior

Tell us what should happen

You should see the MvxRecyclerView display items based on your new items.

Actual behavior

The items inside the MvxRecyclerView display old/outdated information from the previous collection.

Configuration

Version: 4.2.2

kjeremy commented 8 years ago

Thanks for the report. Please point us to a reproducible test case. Thanks.

On Wed, Jul 27, 2016 at 4:03 PM, Robert Till notifications@github.com wrote:

After databinding an observable collection to a MvxRecyclerView, adding to that list, clearing that list, and then adding to the new list, some items show outdated information. I suspect a problem in the adapter or viewholder. It appears to be hanging onto old information. I cannot "refresh" my list atm as it will display outdated information if I try (keep in mind the underlying databound object is correct, it appears just the View itself is displaying old information). Steps to reproduce

1.

Create an ObservableCollection of some custom object in your viewmodel. 2.

Bind that collection to a MvxRecyclerView. 3.

Show a property from the custom object (like an integer or string) to a textview for the individual item inside the recyclerview. 4.

Add new items to the collection and watch the recyclerview update its items. 5.

Either .Clear() or set the ObservableCollection to null; 6.

Set the ObervableCollection to a new set of values (or even the previous set)

Expected behavior

Tell us what should happen

You should see the MvxRecyclerView display items based on your new items. Actual behavior

The items inside the MvxRecyclerView display old/outdated information from the previous collection. Configuration

Version: 4.2.2

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/MvvmCross/MvvmCross-AndroidSupport/issues/280, or mute the thread https://github.com/notifications/unsubscribe-auth/AEIBRAmVB6VckkRtrtQeHWzwi0tlXOkWks5qZ7mvgaJpZM4JWkp5 .

Strifex commented 8 years ago

No problem, will post reproducible test case later.

Strifex commented 8 years ago

I've updated my original post, added a link to a repository with a reporoducible test case, etc.

While re-testing I noticed that the issue wasn't with a basic "MvxRecyclerView" but rather a "nested" one. I override MvxRecyclerView just to disable Vertical Scrolling in my example. If you think I broke something in doing so or I'm doing it wrong, please let me know right away! Otherwise, it appears that views for the inner MvxRecyclerView's aren't properly disposed when the data bound collection is either .Cleared() or even set to a new collection. Let me know if you think there is any other information I forgot to provide....

kjeremy commented 8 years ago

Thanks. I'll take a look. The views aren't disposed because we have no way to monitor the lifetime but we do alter the data context and binding context during some of the recyclerview callbacks.

On Jul 27, 2016 11:27 PM, "Robert Till" notifications@github.com wrote:

I've updated my original post, added a link to a repository with a reporoducible test case, etc.

While re-testing I noticed that the issue wasn't with a basic "MvxRecyclerView" but rather a "nested" one. I override MvxRecyclerView just to disable Vertical Scrolling in my example. If you think I broke something in doing so or I'm doing it wrong, please let me know right away! Otherwise, it appears that views for the inner MvxRecyclerView's aren't properly disposed when the data bound collection is either .Cleared() or even set to a new collection. Let me know if you think there is any other information I forgot to provide....

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MvvmCross/MvvmCross-AndroidSupport/issues/280#issuecomment-235791107, or mute the thread https://github.com/notifications/unsubscribe-auth/AEIBRGKcW4TPRfX6ZBwHRgcQk9X0mAVRks5qaCG3gaJpZM4JWkp5 .

kjeremy commented 8 years ago

Also does this work using normal recyclerviews with their own adapters?

On Jul 27, 2016 11:41 PM, "Jeremy Kolb" kjeremy@gmail.com wrote:

Thanks. I'll take a look. The views aren't disposed because we have no way to monitor the lifetime but we do alter the data context and binding context during some of the recyclerview callbacks.

On Jul 27, 2016 11:27 PM, "Robert Till" notifications@github.com wrote:

I've updated my original post, added a link to a repository with a reporoducible test case, etc.

While re-testing I noticed that the issue wasn't with a basic "MvxRecyclerView" but rather a "nested" one. I override MvxRecyclerView just to disable Vertical Scrolling in my example. If you think I broke something in doing so or I'm doing it wrong, please let me know right away! Otherwise, it appears that views for the inner MvxRecyclerView's aren't properly disposed when the data bound collection is either .Cleared() or even set to a new collection. Let me know if you think there is any other information I forgot to provide....

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MvvmCross/MvvmCross-AndroidSupport/issues/280#issuecomment-235791107, or mute the thread https://github.com/notifications/unsubscribe-auth/AEIBRGKcW4TPRfX6ZBwHRgcQk9X0mAVRks5qaCG3gaJpZM4JWkp5 .

Strifex commented 8 years ago

I haven't tried using standard recycler views and adapters, just the Mvx versions. In my current project I'm trying to utilize databinding everywhere I can instead of writing custom adapters.

Also, you may wanna dig deeper but when I was debugging it seemed as though the underlying datacontext was correct for the individual views, but the view itself was cached/outdated.

Strifex commented 8 years ago

I've been doing some debugging..... so far here is what I've found:

MvxRecyclerViewHolder appears to be attaching/detaching/recycling correctly, I'm not seeing any odd behavior.

The DataContext is correct for the views that are wrong, somehow the UI itself is whats not being rendered correctly.

Investing the MvxRecyclerAdapter currently...haven't found the cause yet. Let me know if you find anything @kjeremy

Strifex commented 8 years ago

Ok so I've found part of the root cause at least. The Inner Collections are not triggering a NotifyCollectionChanged Event when Group.Clear() is called and they are re-added. Therefore they are not cleaning everything up correctly.

I switched my demo project to show the workaround (updating the current groups individually instead of clearing the entire collection of groups and resetting it. This seems to resolve the issue, although its not ideal....

I'm not sure how to narrow down the root cause any further, let me know if you have any ideas, but at least I have a "workaround" for now, as unpleasant as it may be.

kjeremy commented 8 years ago

Part of the issue is that MvxRecyclerView.OnDetachedFromWindow clears the binding context. If you add an override in your InnerMvxRecyclerView and don't call the base then it seems to fix half of the problem: you'll get the right contents but your ItemClick events will still fail for some reason. This makes sense because your inner recycler view has been detached. Still investigating...

kjeremy commented 8 years ago

I'm seriously starting to think that when we destroy an activity we should recurse down all the views and see if they are IMvxBindingContextOwners and if they are we should clear the bindings. I really don't like that idea but it would probably fix a lot of the weird RecyclerView issues.

Strifex commented 8 years ago

Honestly I'm all for paying for the overhead of explicitly clearing data-bound controls/views if it means stability inside RecyclerView lol. I wish there was a better way though.....is there any way we could do that kind of recursive cleanup just for RecyclerView controls? Do you think there are other controls besides RecyclerView that would benefit from that kind of brutal approach to clearing datacontexts?

kjeremy commented 8 years ago

I'm not sure. I need to study how the actual bindings work. My gut tells me that something is strong that should be weak or there's possibly an inverse lifetime or something. It may be as simple as making sure that Java handles are valid when we call SetValueImpl but if I recall I tried that and we still ended up leaking target bindings with RecyclerView.

kjeremy commented 8 years ago

Alright... it looks like the second part of the problem is that we're losing the ItemClick event subscriptions. We only set them up once when the ViewHolder is created. We may need to bake those into the IMvxRecyclerViewHolder interface.

Strifex commented 8 years ago

Will baking them into the interface guarantee that the "proper" click event is tied to the proper datacontext? It sounds like a big change lol

kjeremy commented 8 years ago

When the ViewHolder is detached from the window it sets the DataContext to null. That's legit. The problem is that that seems to also set ItemClick to null too and we don't set it again later

Strifex commented 8 years ago

@kjeremy forgot to respond back here last week, sorry about then. Anyway, I was able to override the OnDetachedFromWindow method in the inner recyclerview like you suggested and everything works great now. I couldn't duplicate losing the ItemClick event like you mentioned.....by any chance did you test for ItemClick event when you were using my sample project? The sample I provided previously doesn't use an ItemClick event :) I have a project in development for my day job currently that uses ItemClick event inside an Inner MvxRecyclerView with OnDetachedFromWindow overrode to do nothing, and everything appears to be working great. Is there anything else you think I'll run into with that workaround? perhaps a memory leak? Or should it be fine since when I dispose of my outer MvxRecyclerView, the inner ones should be killed off as well?

Strifex commented 7 years ago

@kjeremy hey, I know this repository got merged into the main one, but I just wanted to confirm that this is resolved as of version 4.4, your fix worked great!