Skip to content
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

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

Closed
gmk57 opened this issue Mar 9, 2021 · 12 comments · Fixed by #64
Closed

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

gmk57 opened this issue Mar 9, 2021 · 12 comments · Fixed by #64
Assignees
Labels
bug Something isn't working

Comments

@gmk57
Copy link

gmk57 commented Mar 9, 2021

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 kirich1409 self-assigned this Mar 10, 2021
@kirich1409 kirich1409 added the bug Something isn't working label Mar 10, 2021
@kirich1409
Copy link
Collaborator

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
Copy link
Author

gmk57 commented Mar 10, 2021

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

@dmitryskorbovenko
Copy link

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

@kirich1409
Copy link
Collaborator

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

@gmk57
Copy link
Author

gmk57 commented Mar 11, 2021

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

@ahulyk
Copy link

ahulyk commented Apr 6, 2021

subscribing to updates

@05nelsonm
Copy link

This issue is plaguing my application as well.

@d4rken
Copy link

d4rken commented Apr 22, 2021

@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?

corona-warn-app/cwa-app-android#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
Copy link
Collaborator

@d4rken , I will check your solution. Thanks

@kirich1409
Copy link
Collaborator

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

@kirich1409
Copy link
Collaborator

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

@kirich1409 kirich1409 linked a pull request May 26, 2021 that will close this issue
@gmk57
Copy link
Author

gmk57 commented Jul 7, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants