Potential Bind Issue (bug?) in AutoGenerated Models

[tikimcfee]

Greetings!

First off, just wanted to say that Epoxy is awesome. Hands down, without question, one of the best libraries I've ever used within the Android ecosystem, period. Ok, enough gushing!

So I'm reaching out with what I think might be an issue with the autogenerated models' bind call with a previous model. I'm not totally sure I'm right here, so please excuse my ignorance! :

I've got a model being bound to a controller. That model is AutoGenerated from a custom view's annotations. At runtime, an instance will have an id "user_id_1". When something changes on a related bit of data, I recreate that model with the same id ("user_id_1"), and set some properties on it. At runtime, during the rebind with current and previous data, they have these properties:

During bind() call with previousModel:
assignedAttributes_epoxyGeneratedModel = {5, 8, 10, 11}
[that.]assignedAttributes_epoxyGeneratedModel = {1, 2, 5, 8, 10, 11, 12, 13}

This means that the call to

if (assignedAttributes_epoxyGeneratedModel.equals(that.assignedAttributes_epoxyGeneratedModel)) 

... will be false. Cool, makes sense; our BitSets are different, so check which thing you need to bind.

However, we then hit:

else {
  if (assignedAttributes_epoxyGeneratedModel.get(5) && !that.assignedAttributes_epoxyGeneratedModel.get(5)) {
    object.setUserImage(userImage_UserDisplayParams);
  }
   else if (assignedAttributes_epoxyGeneratedModel.get(6) && !that.assignedAttributes_epoxyGeneratedModel.get(6)) {
    object.setEnlargedImage(enlargedImage_Params);
  }
   else if (assignedAttributes_epoxyGeneratedModel.get(7) && !that.assignedAttributes_epoxyGeneratedModel.get(7)) {
    object.setStandardImage(standardImage_Params);
  }
  else {
    object.setStandardImage((Params) null);
  }
}

The issue here is that, in both cases, 5 is a set bit for both. However, that value, which is a new object 'UserDisplayParams' has actually changed, which means I need that value to be rebound! What happens in this case is the final group call is made to setStandardImage, which ends up blowing the changed image away.

My custom view is annotated this way:

@ModelProp(group = "image")
public void setUserImage(@Nullable UserDisplayParams displayParams) { // ... }

@ModelProp(group = "image")
public void setEnlargedImage(@Nullable Params displayParams) { // ... }

@ModelProp(group = "image")
public void setStandardImage(@Nullable Params params) { // ... }

If I had to guess, I'd say I'm probably doing something wrong. Perhaps I should be attaching a unique id to the modelID each time?

I really appreciate your time with this, and thanks again for the awesome tool! Looking forward to the tut-tut for breaking something 😅

[tikimcfee]

Bit more info - as a workaround, I tested this out:

model.id(model.id() + SystemClock.elapsedRealtimeNanos());

So the id's will (barring long overflows that collide) always be different. It looks like in this case, the changes are always propagating through, but because the models id's are different, it's a non-payload change and you get a few unnecessary change/fade animations from the default RV animations.

[elihart]

Hey, thanks for the kind words and the detailed report. I believe this is an error on my side, you are probably doing everything correctly.

I believe the logic is faulty in the case that you 1) have a prop group 2) toggle a prop outside of that group to be set or not set.

A bit surprised we haven't run into this one before, thanks a lot for reporting. I'll try to have a fix out soon - sorry for the inconvenience. You're right that a workaround is to change the model id every time, you just might get slightly weird animations since it thinks items are being added and removed.

If you can change the model so that the same props are being set (so the same bits in the bitset are set) then it should also work around this for now

[tikimcfee]

Thanks for the update!

Your diagnosis is exactly right - I have some stuff in and out and of the prop group, and they are indeed toggled in different paths.

Much appreciated on your incoming change, and don't worry about it at all. I'm still working on the component that we're using the prop group feature in, so there's still lots to do outside of this piece.

I actually might be able to do that - temporarily manage what BitSet ends up seeing. It seems like if I add a couple wrapper objects around some of my primitives (some Strings, couple resource id's, etc.) and then always set them, that would keep the two sets equal and then do the correct comparisons. I like!

Thanks again for looking in to it. I'll keep watching and happily lend a testing hand if you'd like. Cheers!

[elihart]

I have a fix here #347

This new logic should be correct, and I have unit tests now. If you want to take a look at the logic and see if it looks right to you that would be appreciated. I'll try to release a new version with the fix tomorrow.

[tikimcfee]

This looks exactly right; without looking at the view, I would assume the if/else if structure implies a prop group, and the logic for that looks exactly right. Reading through Carousel.java confirms - padding setters are in a prop group! I'll also assume the model generator will do equality checks on non-primitives with standard equals() (I think I read that in the wiki, such that you need to make sure your pojo's implement proper equals/hashes).

if (assignedAttributes_epoxyGeneratedModel.get(3)) {
  if ((paddingRes_Int != that.paddingRes_Int)) { /* .. */ }
}
else if (assignedAttributes_epoxyGeneratedModel.get(4)) {
  if ((paddingDp_Int != that.paddingDp_Int)) { /* .. */ }
}

This one I was a little unsure of, but then I remembered that I believe you are only allowed a single default value in a prop group because.. well, how the heck can you have a group of things with an implicit contract of a single one being set but have multiple methods that say they have defaults? 😂 This also looks good, seems like a clever way to ensure a default is set.

else if (that.assignedAttributes_epoxyGeneratedModel.get(3) || that.assignedAttributes_epoxyGeneratedModel.get(4)) { /* .. */ }

Awesome sir, can't wait to pull the changes down. Many thanks again to ya!

[elihart]

Great, thanks for looking, really appreciate the feedback! PR is merged, I'll have a release out soon.

[elihart]

2.7.3 is out with the fix.