Android: Abstract custom view and common layout inflation

Question

So, I've done some research and it seems that view inflation within the init/constructor of an abstract base class is not really a best practice. I understand that's because the base class initializer happens before the init/constructor of the derived class. Since the abstract class is non-final there's a nice IDE message about this being leaked in the init block.

Here is what I'm after though:

abstract class Foo @JvmOverloads constructor(
    context: Context,
    attrs: AttributeSet? = null,
    defStyleAttr: Int = 0
) : FrameLayout(context, attrs, defStyleAttr) {

    private val myView: View

    init {
        // todo@patches fix leaking "this"
        View.inflate(context, R.layout.view_foo, this)
        myView = requireNotNull(findViewById(R.id.my_view))
    }
}

class Bar @JvmOverloads constructor(
    context: Context,
    attrs: AttributeSet? = null,
    defStyleAttr: Int = 0
) : Foo(context, attrs, defStyleAttr)

I really don't want to have to add anything to the init of the derived class or make myView a nullable/mutable variable that is set later in the abstract class.

Does anyone else find this slightly frustrating or have any advice? It seems like it would be not uncommon to want to inflate the same layout from a base class.

Does this answer your question? [Leaking this in constructor warning](https://stackoverflow.com/questions/3921616/leaking-this-in-constructor-warning) — Santanu Sur, Mar 06 '20 at 16:29
I'm sorry, but it does not. It only suggests suppressing the warning and doesn't offer an alternative to truly fix this warning. It is also not view related which has different constraints for Android. — isuPatches, Mar 06 '20 at 16:32
The general advice is not to do work in constructors, hence I suggest just moving this logic elsewhere. Refactoing the code so that this class takes a `myView` already initialized is the way to go. — al3c, Mar 06 '20 at 17:32

score 5 · Answer 1 · answered Mar 06 '20 at 21:32

Leaking this in a constructor is dangerous because the object you leak it to could start accessing its members before the constructor even finishes, so it might not be ready. You could get NPEs even for non-nullable Kotlin properties, or other odd behavior.

In the case of LayoutInflator.inflate, this appears to not be an issue, if only because Android's built-in views frequently pass this as the parent to the inflate() method. For example, DatePicker's constructor instantiates a DatePickerSpinnerDelegate, which passes that DatePicker instance to inflate(), all happening before DatePicker's constructor has returned.

When you pass a view as the parent to inflate(), by following the call chain I see two things happen to the parent. It calls getContext() on that parent, and it calls addView() on that parent if addToRoot is true. So I think leaking this is safe so long as you don't override addView() to do additional work that relies on members that you set up after your call to inflate(). ButaddView()also internally callsrequestLayout()andinvalidate()`, so the same concern applies to those.

In most cases your custom ViewGroup would be a subclass of an existing Android ViewGroup class, so you wouldn't need to override those methods.

Unfortunately, we can only infer this behavior by inspecting the code. It's not terribly reassuring that it's not guaranteed safe by the documentation, but as far as I know, we just have to accept that its probably safe. Maybe an issue should be opened on AOSP. This warning doesn't even appear if you write your same code in Java, but the risk is the same.

Suppressing a warning shouldn't mean that you are ignoring a warning or just hacking at your code. It means, "I acknowledge the failure mode and have checked that my code will not fail in this way." If that weren't the case, it would be a compiler error, rather than a warning. In Kotlin, the suppression annotation you can use is @Suppress("LeakingThis").

Android: Abstract custom view and common layout inflation

1 Answers1