Add support for Async Layout inflation with EPoxy #1292
Conversation
| import android.widget.FrameLayout | ||
|
|
||
| /** | ||
| * Base class to support Async layout inflation with EPoxy. |
There was a problem hiding this comment.
We don't upper case the P
| * Base class to support Async layout inflation with EPoxy. | |
| * Base class to support Async layout inflation with Epoxy. |
| package com.airbnb.epoxy | ||
|
|
||
| /** | ||
| * Interface to support Async layout inflation with EPoxy |
There was a problem hiding this comment.
Maybe expand more on what this does/why someone would want to use it and give small usage example
| * onInflationComplete method that MUST be called after the view is asyncronously inflated. | ||
| * It runs all pending runnables waiting for view inflation. | ||
| */ | ||
| fun onInflationComplete() { |
There was a problem hiding this comment.
I wonder if we should provide a convenience function here that calls this. Maybe something like
fun ViewGroup.inflateAsync(@LayoutRes layoutRes: Int, parent: ViewGroup, callback: OnInflateFinishedListener) {
AsyncLayoutInflater(context).inflate(R.layout.async_custom_view_item, this) {
view, resId, parent ->
addView(view)
callback.onInflateFinished(view, resId, parent)
onInflationComplete()
}
}
That might make for less boiler plate when using it in the model view
There was a problem hiding this comment.
Though I guess that would then force a dependency on the async layout inflator in the core module which we probably don't want to do.
There was a problem hiding this comment.
I think so, we should keep it optional to keep the library as small as possible.
gradle.properties
Outdated
| VERSION_NAME=5.0.0-beta05 | ||
| VERSION_NAME=5.0.0-beta06 |
There was a problem hiding this comment.
Make sure to roll this version bump back as we do that as part of the release process
| * inflated view. If the view is already inflated, the runnable will immediately run, | ||
| * otherwise it is added to the list of pending runnables. | ||
| */ | ||
| fun executeWhenInflated(runnable: Runnable) { |
There was a problem hiding this comment.
Do we need a way of clearing these pending executions when the view is recycled?
| @@ -339,4 +348,12 @@ public boolean isStickyHeader(int position) { | |||
| } | |||
|
|
|||
| //endregion | |||
|
|
|||
| protected void executeWhenInflated(EpoxyViewHolder holder, Runnable runnable) { | |||
| if (holder.itemView instanceof AsyncInflatedView) { | |||
There was a problem hiding this comment.
@varungu Have we considered making the async layout a property of the EpoxyModel or EpoxyViewHolder so we can avoid needing to do an instanceof check here?
There was a problem hiding this comment.
I have a TODO for this in the description. It is not very straight forward to make it a property. Biggest problem is all views will need to extend from this interface, that we don't want to do. If we don't enforce the extension, we will need the check in addition to the property.
My plan is to test this change before putting effort into adding a property.
There was a problem hiding this comment.
@varungu I just realized an additional concern with this approach. On each bind and attach/detach from the window this will do an object allocation of the runnable which may cause some performance hits for those not using async layouts. I think we'd probably want to better integrate this into the model or view holder before merging.
There was a problem hiding this comment.
We can separate this and create runnable only for AsyncViews, would that resolve your concern?
| @@ -108,6 +108,15 @@ public void onBindViewHolder(EpoxyViewHolder holder, int position) { | |||
|
|
|||
| @Override | |||
| public void onBindViewHolder(EpoxyViewHolder holder, int position, List<Object> payloads) { | |||
| if (holder.itemView instanceof AsyncInflatedView) { | |||
There was a problem hiding this comment.
I think Peter might've mentioned this earlier and I'm not completely familiar with the internals of Epoxy but is there a way where we could avoid these checks? We discussed an implementation where we potentially create an AsycEpoxyModel class (similar to how we handled it for Compose interoperability - https://github.com/airbnb/epoxy/blob/master/epoxy-compose/src/main/java/com/airbnb/epoxy/ComposeInterop.kt)
|
@varungu @vinaygaba can we merge this? |
|
@elihart I think Varun is running a test with these changes. |
…to support Async view inflation with EPoxy
…er.getBindingAdapterPosition
EPoxy currently supports only synchronous layout inflation with most of the time during first layout is spent on the inflation of the views.
To make the first layout faster, we should support asynchronous layout inflation. With this approach, the application developer can make heavy views (like video player) asynchronous and improve the page load time
I investigated if we can use a different view like LinearLayout and merge tag in the xml. It won't work with Asynchronous layout inflation since merge tag requires "attachToParent" as true, and asyncronousLayoutInflator inflates with it as false. We will need to use FrameLayout for asynchronous inflation.
TODO
Screen.Recording.2022-04-27.at.11.45.22.AM.mov