-
Notifications
You must be signed in to change notification settings - Fork 729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
View Binder Support #1175
View Binder Support #1175
Conversation
Add view binder to kotlin sample
…up (#1165) * Add epoxyView shortcut to ViewGroup * Add optionalEpoxyView for activity and view (with a minor refactor for less copy/paste) * switch to `androidx.core.app.ComponentActivity` Co-authored-by: Emmanuel Boudrant <eboudrant@netflix.com>
* Add view binder visibility tests * Add basic unit tests for the view binder * Gradle clean up * Can't use ComponentActivity directly with the current dependency version Co-authored-by: Peter Elliott <peter.elliott@airbnb.com>
* package="com.airbnb.epoxy.viewbinder" * import com.airbnb.epoxy.viewbinder.R
Some documentation tweaks
Add newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmecho looks pretty great overall! Just have some polish suggestions. Thanks for all your work on this and for adding such great test coverage, super exciting to finally be able to share this!
@@ -19,6 +19,9 @@ rootProject.ext.ANDROIDX_RECYCLERVIEW = "1.1.0" | |||
rootProject.ext.ANDROIDX_MATERIAL = "1.0.0" | |||
rootProject.ext.ANDROIDX_APPCOMPAT = "1.0.0" | |||
rootProject.ext.ANDROIDX_CARDVIEW = "1.0.0" | |||
rootProject.ext.ANDROIDX_CORE_KTX = "1.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the latest version is 1.3.2 https://maven.google.com/web/index.html#androidx.core:core-ktx
or could use 1.5.0-rc01
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would probably bump up the app compat version though. Should we just increase the versions across the board for AndroidX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to leave these as is and bump those versions in a separate PR in case there are issues.
* 2. You have a predefined view in a layout and you want to update its data functionally with an | ||
* EpoxyModel | ||
*/ | ||
class EpoxyViewBinder : EpoxyController() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be much better to implement ModelCollector
instead of extending EpoxyController. This removes some hackiness around extending the controller and is also more efficient since we don't need to create extra objects in the controller that are unused.
I meant to do this in our codebase and never got around to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would then need to replace all function receivers EpoxyController.() ->
with ModelCollector.()
I think that would be pretty simple
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oo nice I'll try that out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is much cleaner now! I added an exception handler. Chose not to use the global one in the EpoxyController
as that'd need an API change.
if (existingHolder == null || !model.hasSameViewType(existingHolder.model)) { | ||
val newView = model.buildView(parentView) | ||
newView.id = previousView?.id ?: ViewCompat.generateViewId() | ||
parentView.addView(newView) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kdoc seems wrong since it says it does not add the view to the parent [ViewGroup]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I'll figure out which is the intended behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it looks like this was just an error. Removed the line.
fun ComponentActivity.epoxyView( | ||
@IdRes viewId: Int, | ||
useVisibilityTracking: Boolean = false, | ||
initializer: LifecycleAwareEpoxyViewBinder.() -> Unit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should initializer
have a default empty lambda so it can be skipped?
@IdRes viewId: Int, | ||
useVisibilityTracking: Boolean = false, | ||
initializer: LifecycleAwareEpoxyViewBinder.() -> Unit, | ||
modelProvider: EpoxyController.() -> Unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be nice if we provided a context parameter to the modelProvider lambda
fun Fragment.epoxyView( | ||
@IdRes viewId: Int, | ||
useVisibilityTracking: Boolean = false, | ||
initializer: LifecycleAwareEpoxyViewBinder.() -> Unit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe for completeness fallbackToNameLookup
should be in the non optional functions too. I think we just didn't have that use case internally
initializer: LifecycleAwareEpoxyViewBinder.() -> Unit, | ||
modelProvider: EpoxyController.() -> Unit | ||
) = LifecycleAwareEpoxyViewBinder( | ||
(this.context as? LifecycleOwner) ?: error("LifecycleOwner required as view's context "), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are cases where we may need to unwrap the context in case of ContextWrapper
. We do this in EpoxyRecyclerView
private fun getContextForSharedViewPool(): Context {
var workingContext = this.context
while (workingContext is ContextWrapper) {
if (workingContext is Activity) {
return workingContext
}
workingContext = workingContext.baseContext
}
return this.context
}
```
epoxy-viewbinder/src/main/java/com/airbnb/epoxy/EpoxyViewBinder.kt
Outdated
Show resolved
Hide resolved
viewBinder.addInterceptor(interceptor) | ||
} | ||
|
||
/** Replaces the [View] with the model produced by the [modelProvider] lambda. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this more accurate?
/** Replaces the [View] with the model produced by the [modelProvider] lambda. */ | |
/** Updates the [View] with the model produced by the [modelProvider] lambda. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably too simplistic of a kdoc for this function as it could either hide, replace, or update the view depending on the prior and new model. I'll update it.
epoxy-viewbinder/src/main/java/com/airbnb/epoxy/EpoxyViewBinderVisibilityTracker.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Eli Hart <eli.hart@airbnb.com>
Add exception handler Misc improvements based on PR comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmecho looks great! thanks for the polish :)
* `context` is a ContextWrapper it will continually unwrap it until it finds the Activity. If | ||
* no Activity is found it will return the the view's context. | ||
*/ | ||
private fun Context.getContextForSharedViewPool(): Context { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could rename this to unwrapContextForLifecycle
.
Should the comment be updated since this no longer is used for view pools?
* * View binders in scrollable views will only forward the initial visibility state. New events | ||
* will not be emitted on scroll actions. This is due to not knowing when the outer view scrolls. | ||
*/ | ||
class EpoxyViewBinderVisibilityTracker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it's not feasible or worthwhile to share more of this code with the original visibility tracker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah unfortunately I think they're different enough that it'd be challenging to refactor them to share code. The original one is tightly coupled to working with a recycler view. Some methods are the same/close to the same but I don't think we gain much with the effort to consolidate with compose on the horizon.
Adds an
EpoxyViewBinder
class inside a newepoxy-viewbinder
module that allows for using Epoxy models outside of recycler views. This has been used internally at Airbnb for a few years.Additional changes:
Model
in the integration test module to update the view for Espresso testing.kotlin-sample
Wiki documentation to come in a separate PR.
A special thanks to:
EpoxyViewBinderVisibilityTracker
, which built off of theEpoxyVisibilityTracker
.