Request for clarification in DoNotHash and OnModelClickListener documentation

[stephen-mojo]

I am a bit confused by the documentation of DoNotHash and OnModelClickListener.

I would like to start by saying the documentation goes from talking about Animals to talking about Listings:

For example, imagine a Animal object is used to create an EpoxyModel, and the on click listener for the model holds a reference to that Animal for use in the callback. If the animal changes, and the model is rebuilt, the new click listener (with a reference to the new Animal) will not be bound to the view, and when it is invoked the old (and incorrect) Listing will be used in the callback.

I still think I get what is being communicated and feel that the problem OnModelClickListener it is solving makes sense to me, but how it solves it does not.

If we are to store all the data we care about in the model to be retrieved later in an OnModelClickListener callback, what is the point of the OnModelClickListener? If the important data is now part of the model as an Epoxy attribute, wouldn't it result in a rebind with a new closure anyways?

Explanation:

It is my understanding that using DoNotHash on a click listener Epoxy attribute in an Epoxy model can result in an a stale listener with a closure over an out-of-date object. This can happen when none of the Epoxy attributes on your model change, but other attributes of your source data do. Because nothing marked as an Epoxy attribute has changed, Epoxy doesn't bind a new click listener. The old click listener is still in place with a closure over an old object.

Example: Say our source data is made up of a list of Animal Kotlin data classes:

data class Animal {
  id,
  name,
  age,  
  color
}

The Epoxy model for an animal has the following attributes and only visually shows the name:

AnimalModel {
  @EpoxyAttribute
  name

  @EpoxyAttribute(DoNotHash)
  onClickListener
}

In your Epoxy controller, you loop over a list of animals and make a model for each one:

animals.forEach { animal ->
  animal {
    id(animal.id)

    name(animal.name)

    onClickListener {
    print("{animal.name} is {animal.age} years old.")
    }
  }
}

Now say the age of a few animals change and you now have a new list of animals. When the controller loops over the animals, no Epoxy attributes change so nothing is rebound. When the user clicks on an animal, the wrong age is printed due to a stale closure.

What I do not understand is how OnModelClickListener solves this.

It is my understanding that OnModelClickListener always gives you the most up-to-date model even if the view was never re-bound.

The documentation states:

If all data is stored in the model, then it can be retrieved in onClick and always be up to date. You may need to store additional data in the model just for use in the click callback.

If we follow this advice and store all data that we care about on the model as an Epoxy attribute, wouldn't that mean if said data changed, the hash code of the model data would change thus triggering a new bind? If the model is rebound, wouldn't the click listener also be bound again with a closure on the most-up-date data anyways?

Example: Say we add all the data we care about to the model (even stuff we don't visually show):

AnimalModel {
  @EpoxyAttribute
  name

  @EpoxyAttribute
  age

  @EpoxyAttribute(DoNotHash)
  onClickListener
}

It seems to me, both of these work, but the OnModelClickListener isn't necessary.

Option #1: Read from closure

animals.forEach { animal ->
  animal {
    id(animal.id)

    name(animal.name)

    age(animal.age)

    onClickListener {
    print("{animal.name} is {animal.age} years old.")
    }
  }
}

Option #2: Read from model

animals.forEach { animal ->
  animal {
    id(animal.id)

    name(animal.name)

    age(animal.age)

    onModelClickListener { model, view, position ->
    print("{model.name} is {model.age} years old.")
    }
  }
}

Or alternatively, we store the entire Animal object in the model:

AnimalModel {
  @EpoxyAttribute
  animal

  @EpoxyAttribute(DoNotHash)
  onClickListener
}

Option #1: Read from closure

animals.forEach { animal ->
  animal {
    id(animal.id)

    animal(animal)

    onClickListener {
    print("{animal.name} is {animal.age} years old.")
    }
  }
}

Option #2: Read from model

animals.forEach { animal ->
  animal {
    id(animal.id)

    animal(animal)

    onModelClickListener { model, view, position ->
    print("{model.animal.name} is {model.animal.age} years old.")
    }
  }
}

[elihart]

If the model is rebound, wouldn't the click listener also be bound again with a closure on the most-up-date data anyways?

No, what you might be missing is that a re-bind for updated properties is incremental. It only rebinds the things that changed, and with DoNotHash applied the click listener will not be rebound. Take a look at the generated model code to see the implementation details. it would be possible to change this, and always rebind listeners in the incremental bind case, but it adds one more layer of complexity and special handling. maybe it's a great idea we just never considered though

another reason to use the onModelClickListener is that you can share the listener between all items instead of allocating a new one for each item in the list. this can make rebuilding models more efficient.

yes, it seems the docs got out of date in terms of animals and listings, feel free to send a PR to fix it!

[stephen-mojo]

Thank you for the response, but I am still not sure I completely follow.

No, what you might be missing is that a re-bind for updated properties is incremental. It only rebinds the things that changed, and with DoNotHash applied the click listener will not be rebound. Take a look at the generated model code to see the implementation details.

Are you saying that each individual attribute is inspected for a change before re-binding to a view at an attribute by attribute level? It appears to me that EpoxyModel.bind() is all or nothing. Either it binds all the model's attributes (in the case that it is called) or none of them (in the case that it is not called). Also EpoxyModel setter methods seem to always overwrite attributes even when DoNotHash is set on them. This leads me to believe that if any individual EpoxyAttribute changes, then the overall model hash changes, and then all attributes are bound to the view in EpoxyModel.bind(), even those that are marked DoNotHash.

For example, the generated code for setting an DoNotHash onClickListener is like this. Is not the click listener being overwritten each time?

 public ListItemEpoxyModel_ onClickListener(final OnModelClickListener<ListItemEpoxyModel_, ListItemEpoxyModel.Holder> onClickListener) {
  onMutation();
  if (onClickListener == null) {
    super.onClickListener = null;
  }
  else {
    super.onClickListener = new WrappedEpoxyModelClickListener(onClickListener);
  }
  return this;
}

public ListItemEpoxyModel_ onClickListener(View.OnClickListener onClickListener) {
  onMutation();
  super.onClickListener = onClickListener;
  return this;
}

Where in the code is this incremental diffing done?