Skip to content

Comments

Track updates to underlying list in mInternalObjects on notifyDataSetChanged().#304

Open
mattgmg1990 wants to merge 2 commits intogabrielemariotti:devfrom
mattgmg1990:bug_fix
Open

Track updates to underlying list in mInternalObjects on notifyDataSetChanged().#304
mattgmg1990 wants to merge 2 commits intogabrielemariotti:devfrom
mattgmg1990:bug_fix

Conversation

@mattgmg1990
Copy link
Contributor

When working on my project, I realized that this important piece was missing to my previous pull request. If the list that the CardArrayAdapter is referencing is modified outside of the adapter and notifyDataSetChanged() is called, mInternalObjects will be in an incorrect state. This can completely break the undo functionality.

The solution is to totally regenerate mInternalObjects when notifyDataSetChanged is called, since we have no guarantee what state the ArrayList is in anymore. Using notifyDataSetChanged is a relatively common workflow, so this should be patched.

If the list is modified outside of the CardArrayAdapter, re-generate
mInternalObjects in notifyDataSetChanged(), as it's contents are now stale.

Change-Id: I5bc6879970fbc1d8debc21d6dd0a8b8185ab2b30
@gabrielemariotti
Copy link
Owner

True, but this kind of solution can have a performance issue if the adapter has a lot of items.
I have to study this case.

@mattgmg1990
Copy link
Contributor Author

Yes, that is true that the performance will suffer with many items. However, this is necessary since in the worst case the elements in the ArrayList could be completely different than before.

In CardArrayAdapter, notifyDataSetChanged() will now rebuild
mInternalObjects from the current list contents. However, this will
discard removed cards unless they are added back in upon swiping. Fix
this bug by adding removed cards back in.

Change-Id: Ib836fc8773b7a087090760e119094d2decc1ecf6
@mattgmg1990
Copy link
Contributor Author

I still think this commit is necessary, but realized that it caused undo to become non-functional. I have fixed this properly in a57aa8b. The behavior is now what I expect it to be.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants