Support for LayoutContainer

[emmano]

As it turns out, Kotlin synthetic properties cache works by default on Activities, Fragments, and Views, but it does not do so for ViewHolder. They fixed this as of Kotlin 1.1.4 via LayoutContainer.

Consumers of this libraries have to make sure they make their Renderers implement LayoutContainer if they are using synthetic properties inside of them. If they do not, their ViewHolders are going to call findViewById() on every call to render() making their ViewHolders obsolete. It would be nice if the library itself did this maybe by adding a Kotlin specific artifact that implements this interface (implementation should be trivial). Thoughts?

[pedrovgs]

Hi, @emmano I'd love to make this library Kotlin friendly and this could be just a really nice feature to add to the project.

About this issue you mention, this feature could be helpful for the renderers implementation invoking findViewById() inside the render method, but not for most of the implementations using this method from the setUpView method.

However, the usage cache you mention working on Activity and Fragment thanks to the implementation of the LayoutContainer could be a nice to have. Right now I'm working on a different project and I don't have time for this feature, but feel free to send a PR or any other contribution. I'm marking this issue as help_wanted and enhacement so anyone interested in this feature can contribute.

[emmano]

About this issue you mention, this feature could be helpful for the renderers implementation invoking findViewById() inside the render method, but not for most of the implementations using this method from the setUpView method.

This is true, but since most Kotlin users are just going to use synthetic properties (similarly to how you use ButterKnife), their setUpView() will be empty and their render() will look like:

fun render() {
    title.text = getContent().title
}

That will basically break the ViewHolder pattern if LayoutContainer is not supported. I will try to carve out some time to get it done.

[pedrovgs]

It could be nice if we have this feature. My proposal is to implement this in two steps. The first step is simple. Update the documentation explaining how to implement LayoutContainer and Renderers. Thanks to this simple step we could let the users know that they can have all the benefits of this feature while we develop the official support.

The second step is sending the PR. We need to create the Kotlin module. Implement the feature. Configure the module to be deployed to maven central as the root module is configured. We should also add a linter and testing support for this new Kotlin module. What do you think @emmano, would you like to collaborate sending these two pull requests?

[pedrovgs]

Maybe @Serchinastico can help us with the pull request review 👍

[emmano]

Sure. The first one should be simple. The second one will take more time. I am also looking into issue #45.

[pedrovgs]

Thank you so much @emmano 😃

[emmano]

So, it looks like in order to get this to work (without library changes) the following needs to happen.

Renderers will have to take a View(the root View) in their constructor:

val rootView = layoutInflater.inflate(R.layout.comic_item, recycler, false)
val rendererBuilder = RendererBuilder<Comic>(ComicRenderer(rootView))

This is required since the Renderer is has to implement LayoutContainer. LayoutContainer is declared as follows:

public interface LayoutContainer {
    /** Returns the root holder view. */
    public val containerView: View?
}

I think it makes the most sense to satisfy the dependency via a constructor argument as follows:

class ComicRenderer(override val containerView: View) : Renderer<Comic>(), LayoutContainer {

    override fun inflate(inflater: LayoutInflater, parent: ViewGroup?): View = containerView
    
    override fun hookListeners(rootView: View?) = Unit
    
    override fun setUpView(rootView: View?) = Unit

    override fun render() {
        comic_title.text = content.title
    }
}

I know it does not look great, but it will be temporary until the library officially supports LayoutContainer

[emmano]

The second step is sending the PR. We need to create the Kotlin module. Implement the feature. Configure the module to be deployed to maven central as the root module is configured. We should also add a linter and testing support for this new Kotlin module. What do you think @emmano, would you like to collaborate sending these two pull requests?

Only the actual class that implements `LayoutContainer` can benefit from `View` caching. I think it will be left to the users of the library to correctly implement `LayoutContainer` in their `Renderers`. I can still send you a PR with an updated README that shows how to do it (from the comment above).

[Serchinastico]

Hi @emmano,

I think passing the view in the constructor will break some uses of the library. Keep in mind that renderers are cloned under the hood, I'm not sure how that will work as we expect it to. Besides, with the new API change we wouldn't need the layout inflater no more and we'd be requiring renderer constructor to know how to build views.

I'd go for a new Renderer child class that does the LayoutContainer magic for you, something like:

abstract class KRenderer<T>: Renderer<T>(), LayoutContainer {
    override lateinit var containerView: View

    override fun setUpView(rootView: View?) {
        containerView = rootView!!
    }
}

I don't know if that's worth a module, thought. We can just add a new section to the README for Kotlin users.

Note: Please, do not call it KRenderer, it's just a joke :)

[emmano]

Besides, with the new API change we wouldn't need the layout inflater no more and we'd be requiring renderer constructor to know how to build views.

Yeah, I did not like that either. I just wanted to keep the contract as it is.(i.e. keeping containerView immutable)

I agree that this change does not grant a module and updating the README should be enough.

Implementers would do something like this:

class ComicRenderer : Renderer<Comic>(), LayoutContainer {
    override lateinit var containerView: View

    override fun inflate(inflater: LayoutInflater, parent: ViewGroup?): View =
            inflater.inflate(R.layout.comic_item, parent, false)

    override fun hookListeners(rootView: View?) = Unit

    override fun setUpView(rootView: View?)  {
        containerView = rootView!!
    }

    override fun render() {
        comic_title.text = content.title
    }
}

If you agree I will work on the README file and submit a pull request.

[pedrovgs]

@emmano I think this solution should be enough and should work for everyone 😃 If you want to send a PR adding this information to the README.md file that'd be awesome 👏