airbnb / epoxy

Epoxy is an Android library for building complex screens in a RecyclerView
https://goo.gl/eIK82p
Apache License 2.0
8.51k stars 726 forks source link

Changing the underlaying data #300

Closed mradzinski closed 7 years ago

mradzinski commented 7 years ago

Hey @elihart, QQ over here: I'm using EpoxyControllers and EpoxyModelGroup and I was wondering if it's possible to update/change the underlying data of the EpoxyController and call notifyItemChanged or notifyDataSetChanged on it's adapter like on a regular one.

The use case is quite simple: I have a to update the presence of a user (they have the classical green and red dots next to their avatars), and I receive that information from outside the models so I would need to update the underlaying list of users somehow and notify the adapter of the change way after the construction of the controller (so can't really use Interceptors) and throughout the lifespan of the RecyclerView.

Is this possible using Epoxy? My first attempt didn't throw any errors, but no "refresh" to the adapters data happened.

elihart commented 7 years ago

@mradzinski what you should do is update the underlying data of the EpoxyController and then call epoxyController.requestModelBuild() so that the models are rebuilt. The models built in buildModels should reflect the state of the users, so when they are built again the models should change if there are any changes to the users. Epoxy will diff the models and update the adapter if needed. For this to work equals/hashcode must be implemented correctly.

This is a very common usage and you hopefully shouldn't have problems, but if you do the models are probably not set up up correctly. lmk if you have problems and share code here and I can help.

mradzinski commented 7 years ago

@elihart It's actually not working, and I suspect it has something to do with the models being inside group. equals and hashcode are properly implemented (I'm using a Kotlin Data Classes which implements that automatically and checking the generated Java code I can see the presence property there on equals and hashcode).

Here's my buildModels adapter code (I'm using a BasePostModel which is simply an interface implemented by many different kinds of posts and which "makes" them implement the getPostType):

    override fun buildModels(posts: List<BasePostModel>, isLoadingMore: Boolean?) {
        posts.forEach { post ->
            val group: EpoxyModelGroup = when(post.getPostType()) {
                PostType.PHOTO -> PostPhotoGroup(post as PostPhotoModel, callback)
                // ....
            }

            group.apply { id(post.id) } // <-- here I'm applying the group an ID which equals the Post ID.

            add(group) // <-- Simply adding the group, nothing weird
        }
    }

My implementation of PostPhotoGroup (they all mostly the same and in this scenario is the one that has to be update):

class PostPhotoGroup(post: PostPhotoModel, callback: AdapterCallback) :
        EpoxyModelGroup(R.layout.item_post_group_photo, buildViewModels(post, callback)) {

    companion object {

        fun buildViewModels(post: PostPhotoModel, callback: AdapterCallback): List<EpoxyModel<*>> {
            val models = ArrayList<EpoxyModel<*>>()

            models.apply {
                add(PostGroupFactory.buildHeader(post, callback))
                // ...
            }

            return models
        }
    }
}

Here's the PostGroupFactory.buildHeader() method which returns the PostHeaderModel_ which is the one that has the avatar with presence property:

    fun buildHeader(post: BasePostHeaderModel, callback: AdapterCallback): PostHeaderModel_ {
        return PostHeaderModel_()
                .avatar(post.user.avatar)
                .circleColor("#6b41f4")
                .connectivityIndicatorColor("#6b41f4")
                .userPresenceState(post.user.presenceStatus) // <-- Here's the presence
                .avatarClickListener { _ -> callback.onAvatarClick(post.user.id) }
                .userFullname(post.user.userFullName)
                .username("@${post.user.username}")
                .company(post.user.company.companyName)
                .userPositionInCompany(post.user.userPositionInCompany)
                .postCreationTime(post.createdTimeAgo)
    }

My implementation of the PostHeaderModel looks like this: (abbreviated for simplicity):

@ModelView(autoLayout = ModelView.Size.MATCH_WIDTH_WRAP_HEIGHT)
class PostHeader(context: Context): FrameLayout(context) {

    //...

    @ModelProp
    fun setUserPresenceState(status: Boolean) {
        // The avatar is just a custom view holding two ImageView's and has two methods that only show or hide the connectivity indicator.
        if (status) {
            avatar.showConnectivityIndicator()
        } else {
            avatar.hideConnectivityIndicator()
        }
    }
}

When I invoke controller.requestModelBuild() I get no errors, the underlying data changes but I see no effects visually speaking on the RecyclerView of the changes.

elihart commented 7 years ago

Can you enable setDebugLoggingEnabled in your controller and check log output to see what adapter change notifications are being sent?

elihart commented 7 years ago

Oh, I think that is an issue with payloads not working with EpoxyModelGroup. If you scroll the changed item off screen and back (so it is unbound and then rebound does it show correctly?

EpoxyModelGroup is not set up correctly to pass the changed model to the sub models. it needs to implement public void bind(T view, EpoxyModel<?> previouslyBoundModel), access the list of models off of the previous model, and then bind it appropriately.

pretty sure that is the issue, I could try to have a fix out today

mradzinski commented 7 years ago

No logs when invoking requestModelBuild(), the only ones printer by Epoxy are:

D/DashboardAdapterController: Models built: 33.874ms
D/DashboardAdapterController: Duplicates filtered: 1.494ms
D/DashboardAdapterController: Item range inserted. Start: 0 Count: 147
D/DashboardAdapterController: Item range inserted. Start: 0 Count: 147
D/DashboardAdapterController: Models diffed: 1.172ms

And no, when I scroll off the screen and back I still see the same presence status (although I'm positive it changed... I put a debug point at the change and the property does change the from true to false).

Now I changed a bit the implementation of the method that mocks the presence change and Epoxy throws: You cannot call `requestModelBuild` directly. Call `setData` instead to trigger a model refresh with new data.. Should I call setData instead to trigger a model refresh then?

elihart commented 7 years ago

@mradzinski yes, if you are using a TypeEpoxyController you need to call setData again with the updated data

mradzinski commented 7 years ago

So yes, calling setData did the trick. I don't know why the exception was being swallowed before though.

Does setData does the diffing process and that kind of stuff or refreshes the whole underlying data and all the models? Just concerned a bit about performance here. If it does the diffing then cool, performance shouldn't be an issue, but this logs got me worried me a bit:

First time I call setData:

D/DashboardAdapterController: Models built: 4.585ms
D/DashboardAdapterController: Duplicates filtered: 0.339ms
D/DashboardAdapterController: Item range inserted. Start: 0 Count: 147
D/DashboardAdapterController: Item range inserted. Start: 0 Count: 147
D/DashboardAdapterController: Models diffed: 1.304ms

Second time I call setData with the updated presence state:

D/DashboardAdapterController: Models built: 2.467ms
D/DashboardAdapterController: Duplicates filtered: 0.059ms
D/DashboardAdapterController: Item range changed with payloads. Start: 1 Count: 146
D/DashboardAdapterController: Item range changed with payloads. Start: 1 Count: 146
D/DashboardAdapterController: Models diffed: 1.420ms

Does Item range changed with payloads. Start: 1 Count: 146 means the whole data set changed?

elihart commented 7 years ago

Yes, that means 146 items changed. You should double check that your model equals method is working as expected. Are you including click listeners in your equals method? for example avatarClickListener should probably be using @CallbackProp or DoNotHash.

mradzinski commented 7 years ago

Funny, I'm changing only 1 out of 147 and it says that I changed 146 (but not the one I actually changed).

I do use a click listener which is defined as:

    @CallbackProp
    fun setAvatarClickListener(@Nullable listener: View.OnClickListener?) {
        avatar.setAvatarClickListener(listener)
    }

This is the generated equals for the UserModel Data class:

   public boolean equals(Object o) {
      if(this != o) {
         if(o instanceof UserModel) {
            UserModel var2 = (UserModel) o;
            if(this.getId() == var2.getId() && Intrinsics.areEqual(this.getAvatar(), var2.getAvatar()) && 
               Intrinsics.areEqual(this.getUserFullName(), var2.getUserFullName()) && 
               Intrinsics.areEqual(this.getUsername(), var2.getUsername()) && 
               Intrinsics.areEqual(this.getUserPositionInCompany(), var2.getUserPositionInCompany()) &&
                this.getPresenceStatus() == var2.getPresenceStatus()) {
                  return true;
            }
         }

         return false;
      } else {
         return true;
      }
   }

The generated hashcode (just a classical hashcode method but with some var1 and var10001" stuff) :

   public int hashCode() {
      long var10000 = this.getId();
      int var1 = (int)(var10000 ^ var10000 >>> 32) * 31;
      String var10001 = this.getAvatar();
      var1 = (var1 + (var10001 != null?var10001.hashCode():0)) * 31;
      var10001 = this.getUserFullName();
      var1 = (var1 + (var10001 != null?var10001.hashCode():0)) * 31;
      var10001 = this.getUsername();
      var1 = (var1 + (var10001 != null?var10001.hashCode():0)) * 31;
      var1 = (var1 + (var2 != null?var2.hashCode():0)) * 31;
      var10001 = this.getUserPositionInCompany();
      var1 = (var1 + (var10001 != null?var10001.hashCode():0)) * 31;
      byte var3 = this.getPresenceStatus();
      if(var3 != 0) {
         var3 = 1;
      }

      return var1 + var3;
   }
    public static boolean areEqual(Object first, Object second) {
        return first == null ? second == null : first.equals(second);
    }

No idea why that equals method might return that o is different than this 😕 . If I manually copy and compare the elements in the lists by invoking the equals or hashcode I get the proper results (only 1 of the elements is different)

elihart commented 7 years ago

the EpoxyModelGroup equals method compares all models in its list of models - so if any of those changed it would trigger an update, so it's not necessarily the user

mradzinski commented 7 years ago

I wrote a simple test to check if equals is properly implemented:

// Create a copy of the original state of the ArrayList of posts before doing any changes to it.
val originalPosts = posts.toMutableList() // <-- This is like calling: new ArrayList(posts)

// ... Do all the stuff related to changing the underlaying data for post id 0 on "posts" ArrayList...

originalPosts.removeAll(posts) // <-- Remove all BasePosts that changed and are no longer matching the original ones. removeAll internally uses `equals`.
originalPosts.forEach { Log.d(TAG, "Post ID ${it.id} has changed.") } // <-- Iterate and print only the ones that are still contained in "originalPosts"

Logcat output says:

D/DashboardAdapterController: Post ID 0 has changed.

That's the expected behavior since I'm only changing the post ID 0. So, equals is properly implemented.

Now, the real question is, Does the output printed by Epoxy is right? If you look at it closely it skipped the Id 0 which is the one I actually changed, it says Item range changed with payloads. Start: 1 Count: 146 (Start: 1 ... that's weird, it's actually telling me that I changed from 1 to 146 when in reality it's the other way around). I can assure you Post ID 0 changed because I see the change reflected in the RecyclerView and no other changes are visible on any other posts. Maybe the output is wrong but the operation is working as expected?

elihart commented 7 years ago

The logging logic is in EpoxyDiffLogger. I double checked and it seems right.

Do you have a header or something at position 0? It seems like it is detecting that all of your user model groups changed.

I would guess something besides the Post object is changing in the PostPhotoGroup. Try put a breakpoint in EpoxyModelGroup#equals and step through the equals comparison to find out what is changing.

mradzinski commented 7 years ago

When I put a breaking point on return models.equals(that.models); (on the EpoxyModelGroup#equals method) the biggest difference I find is the models Id's which I'm not setting (I'm setting the group Id but not the model), they simply don't (for example, on that.models[0] the id is -3 when in models[0] the id is -576). Might be that the issue why models are being recognized as not equal?

elihart commented 7 years ago

oh yes, you definitely need to set an id for each nested model in the group

mradzinski commented 7 years ago

Now the logs are a little bit more verbose (for putting it one way):

D/DashboardAdapterController: Item range changed with payloads. Start: 1 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 1 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 4 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 4 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 9 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 9 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 11 Count: 3
D/DashboardAdapterController: Item range changed with payloads. Start: 11 Count: 3
D/DashboardAdapterController: Item range changed with payloads. Start: 15 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 15 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 24 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 24 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 30 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 30 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 35 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 35 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 38 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 38 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 40 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 40 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 43 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 43 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 50 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 50 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 58 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 58 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 62 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 62 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 67 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 67 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 72 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 72 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 78 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 78 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 81 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 81 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 100 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 100 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 103 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 103 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 107 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 107 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 121 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 121 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 125 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 125 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 130 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 130 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 135 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 135 Count: 2
D/DashboardAdapterController: Item range changed with payloads. Start: 138 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 138 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 140 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 140 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 142 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 142 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 146 Count: 1
D/DashboardAdapterController: Item range changed with payloads. Start: 146 Count: 1

BUT... They all belong to the same user, so it totally make sense that if I change the reference to that user then the whole thing returns false on equals. Problem solved, it works as intended. @elihart man, I owe you a huge cold pint 🍺. Next time I'm around the SF area I'll make sure to get you one.

elihart commented 7 years ago

@mradzinski ah great, glad its working I created https://github.com/airbnb/epoxy/issues/303 as a performance enhancement issue, but it shouldn't be a big deal for now.

Thanks for the detailed code and explanations, glad we could get it resolved.

Much of this seems like it could be either better communicated in the documentation, or have checks built into Epoxy. Any suggestions on how you think this could be made clearer so other people don't have similar issues?

mradzinski commented 7 years ago

I think the most important part to mention in the groups document is that although groups should/must have an id, that doesn't exclude models to have their own as well because it's needed for diffing when an underlying change on the data source occurs and we want to reflect the same on the RecyclerView.

The documentation is pretty clear about callbacks and how to exclude methods from being part of the hashcode, so I wouldn't touch that part of it at all.

elihart commented 7 years ago

I updated the wiki about the ids for models within a group.

It seemed like you were not calling setData with your updated data either, was the need for that unclear?

mradzinski commented 7 years ago

I was calling setData the first time (when I received the data) but not when I needed to update it. I actually though EpoxyController would behave just like a regular adapter where you just can have a reference to the underlying data, update that and call controller#adapter#notifyDataSetChanged, but that was my fault for not reading the documentation of setData because the wiki clearly states:

... to pass an object of that type whenever models should be rebuilt

I mean, it's pretty clear once you read it thinking that it's not only for setting up the data but to update the same as well.