androidbroadcast / ViewBindingPropertyDelegate

Make work with Android View Binding simpler
https://proandroiddev.com/make-android-view-binding-great-with-kotlin-b71dd9c87719
Apache License 2.0
1.42k stars 102 forks source link

Invalid ViewBinding on quick forward-back navigation in Fragments #45

Closed gmk57 closed 3 years ago

gmk57 commented 3 years ago

If the Fragment is put in the back stack and then immediately restored with FragmentManager.popBackStack(), ViewBinding still points to old view during onViewCreated.

Here is a sample project to reproduce.

I suppose this is the same bug as in #35, but it is still reproducible with Fragments 1.3.0 and ViewBindingPropertyDelegate 1.4.4. So, as you suggested, I'm opening a new issue.

kirich1409 commented 3 years ago

The problem connected to work of Fragment ViewLifecycle. The delegate use it's callback to clear value, but if the are not called, you will have the old value

gmk57 commented 3 years ago

As midery pointed out, this is caused by mainHandler.post { viewBinding = null } in LifecycleViewBindingProperty.clear(). Calling viewBinding = null synchronously eliminates the bug.

dmitryskorbovenko commented 3 years ago

I just want to add that LeakCanary reports leaks because of mainHandler.post { viewBinding = null } . What the reason was for using mainHandler?

kirich1409 commented 3 years ago

That's behaviour was added, because Fragment.onDestroyView() called after Lifecycle.onDestroy()

gmk57 commented 3 years ago

Yes, I remember well your article on the subject. But it has this unfortunate side-effect.

ahulyk commented 3 years ago

subscribing to updates

05nelsonm commented 3 years ago

This issue is plaguing my application as well.

d4rken commented 3 years ago

@kirich1409 Your awesome blog post in September about leveraging Kotlin's property mechanics inspired me to write similar code, while not as advanced as your library, the mechanism is the same, and I have encountered similar bugs as you. Recently the issue on quick fragment navigation described in this post by @gmk57 .

I've come up with a solution that seems to work for me, maybe this can also work for your library?

https://github.com/corona-warn-app/cwa-app-android/pull/2913

I null the viewBinding before rerunning the initialization if I noticed that the LifeCycle.onDestroy triggered, but the Runnable { viewBinding = null } didn't.

kirich1409 commented 3 years ago

@d4rken , I will check your solution. Thanks

kirich1409 commented 3 years ago

@d4rken , I've checked the solution, but it will help if getValue() is called. But if not we will have memory leak

kirich1409 commented 3 years ago

As a solution for Fragment I've tried

private class FragmentViewBindingProperty<in F : Fragment, out T : ViewBinding>(
    viewBinder: (F) -> T
) : LifecycleViewBindingProperty<F, T>(viewBinder) {

    private var fragmentLifecycleCallbacks: FragmentManager.FragmentLifecycleCallbacks? = null

    override fun getValue(thisRef: F, property: KProperty<*>): T {
        registerFragmentLifecycleCallbacks(thisRef)
        return super.getValue(thisRef, property)
    }

    private fun registerFragmentLifecycleCallbacks(fragment: Fragment) {
        if (fragmentLifecycleCallbacks != null) {
            return
        }

        fragmentLifecycleCallbacks = ClearOnDestroy().also { callbacks ->
            fragment.parentFragmentManager
                .registerFragmentLifecycleCallbacks(callbacks, false)
        }
    }

    override fun clear() {
        super.clear()
        fragmentLifecycleCallbacks = null
    }

    override fun getLifecycleOwner(thisRef: F): LifecycleOwner {
        try {
            return thisRef.viewLifecycleOwner
        } catch (ignored: IllegalStateException) {
            error("Fragment doesn't have view associated with it or the view has been destroyed")
        }
    }

    private inner class ClearOnDestroy : FragmentManager.FragmentLifecycleCallbacks() {

        override fun onFragmentDestroyed(fm: FragmentManager, f: Fragment) {
            // Fix for destroying view for case with issue of navigation
            postClear()
        }
    }
}

I don't like that solution, but it must solve the issue with Lifecycle

gmk57 commented 3 years ago

Another possible approach is described here, but I haven't tried it yet.