google / agera

Reactive Programming for Android
Apache License 2.0
7.2k stars 639 forks source link

RepositoryAdapter calls recycle when rebinding a ViewHolder at a different position. #150

Closed rosiebye closed 7 years ago

codecov-io commented 7 years ago

Codecov Report

Merging #150 into master will increase coverage by <.01%. The diff coverage is 100%.

@@             Coverage Diff             @@
##             master    #150      +/-   ##
===========================================
+ Coverage      97.2%   97.2%   +<.01%     
- Complexity      619     621       +2     
===========================================
  Files            41      41              
  Lines          1679    1681       +2     
  Branches        198     199       +1     
===========================================
+ Hits           1632    1634       +2     
  Misses           14      14              
  Partials         33      33
Impacted Files Coverage Δ Complexity Δ
...gle/android/agera/rvadapter/RepositoryAdapter.java 87.09% <100%> (+0.21%) 23 <0> (+2) :white_check_mark:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f14356c...6090a7e. Read the comment docs.

maxtroy commented 7 years ago

The issue still lies within the RepositoryPresenter collaboration model. The same view holder will be handed over to another RepositoryPresenter without going through a recycle process only in this situation:

In other words:

So from this library's point of view, the two RepositoryPresenters seem more collaborating than conflicting.

If this is not what the coder wants (it seems you've hit this issue by sharing too many view types / stable IDs :) ), then they should break the tie in one way or another; for example:

Or up the collaboration game, so the two RepositoryPresenters share common bookkeeping/rebinding/recycling code (it's only fair if you've already shared the view type and stable IDs).

rosiebye commented 7 years ago

How do stable IDs affect what goes through the recycle process? It seems completely independent to me.

I think it's reasonable that someone might want to have collaborating RepositoryPresenters which share ViewHolders, however I don't think using the same view type signifies this intent (especially as view type is getLayoutResId in the RepositoryPresenter world). If using the same view type with completely different RepositoryPresenters then all of these RepositoryPresenters need to know about each other which dramatically increases the complexity of the code.

Maybe instead RepositoryPresenters should have some sharing token and if two RepositoryPresenters have the same sharing token this indicates they know about each other and can handle ViewHolders moving from one to the other, if not then the ViewHolder must be "recycled" ready for the new RepositoryPresenter. This would be similar to RecycledViewPool, two RecyclerViews don't automatically reuse views from each other, you have to opt in by using a shared RecycledViewPool.

ghost commented 7 years ago

"The issue still lies within the RepositoryPresenter collaboration model"; I agree with this in principle, but in practice the API we provide makes it hard to create this collaboration model. As far as I see it the change would make it harder to do collaboration model mistakes?

maxtroy commented 7 years ago

Maybe this document is not giving out enough hints, or maybe the use of compiler has totally masked the document away from end users:

(class RepositoryPresenter)
  /**
   * Returns the stable ID for the {@code index}-th item to present the data. 
   * ...
   * If stable IDs are enabled, this ID and
   * the item's {@linkplain #getLayoutResId layout resource ID} should together uniquely identify
   * this item in the whole {@link RecyclerView} throughout all changes.
   * ...
   */
  public long getItemId(@NonNull final T data, final int index) {
ghost commented 7 years ago

As discussed separately, when used correctly, the RV will handle this case. Closing.