pockethub / PocketHub

PocketHub Android App
Apache License 2.0
9.39k stars 3.46k forks source link

Fix gist list crash #1170

Closed plweegie closed 5 years ago

plweegie commented 6 years ago

Steps to reproduce:

Meisolsson commented 6 years ago

I don't really like this way of doing it. I've been looking at this before and the problem is deeper than just a one line change.

The problem, at it's root, it probably my misunderstanding of DiffUtil. I'll try to explain my findings here:

We give Groupie a list of Item. Then the user does a "forced refresh" (pull to refresh), we do this when the user returns to the GistsActivity if they looked at a Gist. A this point we clear the list of Item that we had before and create a new list of Item. We then pass the new list of Item to Groupie. That's out story which seems fine.

Groupie take out items and diff them against what it has. When we pass the last list Groupie takes that and notices one or many other items which are the same (areItemsTheSame) and that the content is the same (areContentsTheSame). Since both are true DiffUtil doesn't update the ViewHolder. But Groupie has no clue that the item was not replaced in the list so it updates it's own internal list of Item to match what we passed it. So now both us and Groupie has the correct list of Item but the ViewHolder that we press has an old instance of that Item.

This might be complete nosens to read so here's a short version: (The !## is the instance number)

  1. We pass [ Item(id= 1, name = "Test")!01 ] to Groupie.
  2. Groupie sets that to its internal list.
  3. DiffUtils tells Groupie that a change has happened and Groupie creates a ViewHolder.
  4. The ViewModel is bound with the current Item.
  5. We force refresh.
  6. We pass [ Item(id= 1, name = "Test")!02] to Groupie
  7. Groupie sets that to its internal list.
  8. DiffUtils finds no changes and says noting about an update. (This is what we want, kind of)
  9. We click the view (item in the list) its ViewHolder which has the click listener gets called. Since it's not been bound after the last update it still has the old Item (Item(id= 1, name = "Test")!01 ) so it returns it.
  10. We try to find the item in the Adapter (Groupie) but since it has updated to our new list the item is not found thus returns -1.

This was a long explanation but probably for the best. So this fix might work but it's wrong. I'm gonna try to get a hold of the maintainer of Groupie and see if we are doing something wrong or if it's a bug in the library.

plweegie commented 6 years ago

Exactly, thanks for that. That's more or less what I've found as well - the contents are identical so Groupie callbacks are not sent, but the actual object references for Items are different. The bug also applies to the commit list, issue list and probably anywhere there's a pull to refresh.

I think we're implementing Groupie correctly, my guess is we just need some sort of response caching along the lines of https://android.jlelse.eu/reducing-your-networking-footprint-with-okhttp-etags-and-if-modified-since-b598b8dd81a1

Octodroid does it by passing the boolean force flag all the way down to OkHttp, whereas in our case it looks like it ultimately ends up unused.

plweegie commented 5 years ago

Tested again recently and this seems to have now been fixed.