Fix pulse numbering bug. #481
Conversation
This will prevent a bug in FragmentStatePagerAdapter that aggressively caches the states of the Fragments inside it.
| return (value.getPlusMembers() - value2.getPlusMembers()) * -1; | ||
| } | ||
| return 0; | ||
| return entry.getValue().compareTo(mode, entry2.getValue()); |
There was a problem hiding this comment.
compareTo is a function in PulseEntry. And it was exact duplicate of this.
There was a problem hiding this comment.
Could you just make PulseEntry implement Comparable and avoid this here altogether?
There was a problem hiding this comment.
The type of the objects is Map.Entry<String, PulseEntry> not just PulseEntry. Map.Entry does not implement Comparable. So it will not work. Getting rid of Map.Entry<String, PulseEntry> is out of this PR's scope I think.
There was a problem hiding this comment.
This adapter manages an ArrayList of Map.Entry. WAT? 😉
I'll try to address this later because it bothers me physically. 😱
There was a problem hiding this comment.
I said OMG when I first saw it. :) It is everywhere in the adapter. The key in the map entry is actually used as the title.
|
👍 awesome contribution @tasomaniac |
|
@PareshMayani you already reviewed this right? |
|
|
||
| private void refreshSpinner(String selectedPulse) { | ||
|
|
||
| mSpinner.setAdapter(mSpinnerAdapter); |
There was a problem hiding this comment.
This line needed to be reset every time? Looks out of scope for this method.
There was a problem hiding this comment.
You are right. Fixing now.
|
I reviewed bits of the code but I don't understand enough to assess the change. If you could explain again what you persist, where and why it would help me understand. It'll be best to comment the lines where something is happening. |
| if (!previous.equals(mSpinnerAdapter.getItem(position))) { | ||
| Timber.d("Switching chapter!"); | ||
| mViewPagerAdapter.notifyDataSetChanged(); | ||
| refreshSpinner(mSpinnerAdapter.getItem(position)); |
There was a problem hiding this comment.
ViewPager's notifyDataSetChanged is not working because it aggressively cache the Fragment state which shouldn't be cached because we are changing the list completely. Eg. first we show countries, then we show cities in a specific country. This was causing ListView to restore its scroll position when we change cities, which doesn't make sense.
Multiple Spinner Adapter initialization is removed.
|
Ok, let's 🚢 it. |
The problem is because FragmentStatePagerAdapter was aggressively caching the states of its children Fragments.
Here is the bug reported and it is still open. https://code.google.com/p/android/issues/detail?id=37990
There are couple of ways to fix it using a really really custom FragmentStatePagerAdapter without subclassing it. I don't like those solutions. It is not optimal but recreating the adapter fixes this problem.
The problem is actually because of the positions calculation in the code. On orientation change it reset and makes another problem. I saved it on config change and now it works perfectly.
Fixes #451