Zhuinden / simple-stack

[ACTIVE] Simple Stack, a backstack library / navigation framework for simpler navigation and state management (for fragments, views, or whatevers).
Apache License 2.0
1.36k stars 76 forks source link

Weird behaviour when navigating to new key of same class fragment #261

Closed omkar-tenkale closed 2 years ago

omkar-tenkale commented 2 years ago

I was navigating to a fragment of a class whose instance already exists in the backstack I found that the fragment key's createFragment() was not called and somehow the old key was being passed to the new fragment Dunno why Intended behaviour is a 2 new fragments should be created for 2 key instances even if they are of same class

I had to do override the getFragmentTag() method in key and i probably fixed it

    @Nonnull
    override fun getFragmentTag() = this::class.qualifiedName + ":" + System.identityHashCode(this)

@Zhuinden I'm not sure if its correct or even right?

Zhuinden commented 2 years ago

Hey hey,

System identity hash code is definitely not a good choice, it's not exact across process death. You'll end up with duplicate fragments.

If you need 2 different fragment instances, especially based on the incoming arguments, I recommend making the key be a data class, and override getFragmentTag with toString().

I typically use toString() but you need to ensure that it's implemented in a stable way, like how data class does it, so using the class FQN as the default makes more sense for fragment tag from the library perspective (specifically because the default implementation of toString adds the hash code, which differs after process death).

If there is no differing argument, you can also create UUID as part of the key.

@Parcelize
data class MyKey(private val keyUuid: String = UUID().toString()): DefaultFragmentKey() {
    override fun getFragmentTag(): String = toString() 

    ... 
}
omkar-tenkale commented 2 years ago

Hey, thanks for lightening fast response @Zhuinden

Seems like i have some complications here

I have a base class

@kotlinx.android.parcel.Parcelize
open class BaseKey(val creator : ()->Fragment): DefaultFragmentKey() {
    override fun instantiateFragment(): Fragment  = creator()

    @Nonnull
    override fun getFragmentTag() = this::class.qualifiedName + ":" + System.identityHashCode(this)
}

and a sample key

class PersonDetailKey(val personId: String) : BaseKey({
        PersonDetailFragment()
    })

Would it be fine if I do following modifications

@kotlinx.android.parcel.Parcelize
open class BaseKey(val creator : ()->Fragment): DefaultFragmentKey() {
    override fun instantiateFragment(): Fragment  = creator()

    @Nonnull
    override fun getFragmentTag() = toString() // < maybe this wont work n i have to shift this to all subclasses?
data class PersonDetailKey(val personId: String) : BaseKey({    //< made it a data class
        PersonDetailFragment()
    })
omkar-tenkale commented 2 years ago
fun main() {
    val c = C("somearg")
    println(c.toString())
    println(c.getTag())

}

open class P():I{
    override fun getTag() = toString()
}

data class C(val p :String):P()

interface I{
    fun getTag():String
}

prints

C(p=somearg)
C(p=somearg)

So i guess it'd work 🤫

omkar-tenkale commented 2 years ago

@Zhuinden I see one more problem with your solution of using UUID and toString of data class Let's say I have a key data class ShowInputArgsScreenKey(val input : String) It creates a fragment whose edittext shows input value and clicking on button below it goes to new Key with current edittext input

As this key has a parameter, adding uuid param is not required but i think there will be issues if user just keeps clicking the go button, as two keys are not different objects but their toString values are same (passing unedited input)

Then I have 2 questions

  1. Would that be issue?
  2. If that'd be an issue then can I use nanoid as field in base class and override getFragmentTag as
@kotlinx.android.parcel.Parcelize
open class BaseKey(val creator : ()->Fragment): DefaultFragmentKey() {
   val uuid = UUIDUtils.createNewUUID()
    override fun instantiateFragment(): Fragment  = creator()

    @Nonnull
    override fun getFragmentTag() = toString()+uuid
    }
Zhuinden commented 2 years ago

1.) definitely don't use System.identityHashCode(this) as part of the fragment tag

2.) if you use UUID for a key to ensure each key is unique, I'd definitely make sure that gets parcelized, so the UUID string has to be part of the primary constructor.

Normally I use toString() alone, and ensure that each key is a data class (so that hashcode isn't part of toString).

omkar-tenkale commented 2 years ago

I don't get how System.identityHashCode(this) will cause issues.. can you explain

Zhuinden commented 2 years ago

If you test for process death https://stackoverflow.com/a/49107399/2413303 you'll have fragments recreated by the system that you won't be able to remove as you navigate, so they'll overlap each other. The default implementation of DefaultFragmentKey uses getClass().getName() specifically to avoid usage of hasbCode() as part of the fragment tag.

omkar-tenkale commented 2 years ago

So after process death the fragments are recreated with the old tags but as new keys are also recreated their getFragmentTag() will return a different string because of hashcode change, so the DefaultFragmentStateChanger won't be able to find and remove the old fragments when navigating and instead create new fragments with instantiateFragment() This results in old fragments instantiated by the os again to remain in fragment backstack maybe even visible to user.. So to avoid this, the keys which should differ based on input args and hence return toString for getFragmentTag() must also retain those input args across process death Got it 👍

I wonder if simple-stack's default behaviour to use same fragment for different key instances differs from android (new activity/fragments (screen) are created when user navigates with new data (bundles/args) unless singleInstance ofc but that's explicit declaration. I'm amazed how nobody saw that as a problem before)

Zhuinden commented 2 years ago

I think they didn't see it as an issue because changing the fragment tag is fairly simple.

The default Fragment behaviors would allow you to stack the same fragment type 3 times on itself but if it has the same tag it'd only find the last one at a time, a bit surprising really. Finding the fragment by its tag being reliable is something people generally like as a feature.