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

Wrong SaveableStateHolder key generation #466

Closed
Serpov opened this issue Aug 29, 2023 · 6 comments · Fixed by #474
Closed

Wrong SaveableStateHolder key generation #466

Serpov opened this issue Aug 29, 2023 · 6 comments · Fixed by #474
Labels
enhancement New feature or request

Comments

@Serpov
Copy link

Serpov commented Aug 29, 2023

Hi
package com.arkivanov.decompose.extensions.compose.jetbrains.stack
file Children.kt

line 56
private fun Any.key(): String = "${this::class.simpleName}_${hashCode().toString(radix = 36)}"

class Class1 {
data class Data(val data: Int)
}

class Class2 {
data class Data(val data: Int)
}

Class1.Data(1).key() -> "Data_1"

Class2.Data(1).key() -> "Data_1"

key should be different for inner data classes with the same names

@arkivanov
Copy link
Owner

arkivanov commented Aug 29, 2023

This is a trade-off to avoid using configurations as keys (and so to avoid saving them in the Android Bundle). Currently, KClass#qualifiedName is not available in JS.

Assuming your configurations are defined as a sealed class/interface, or just a simple standalone class, this shouldn't be an issue. What's your use case?

interface SomeComponent {
    val stack: Value<ChildStack<*, ...>>
}

class DefaultSomeComponent : SomeComponent {
    override val stack: Value<ChildStack<Config, ...>> = ...

    private sealed class Config {
        data class Data1(val data: Int) : Config()
        data class Data2(val data: Int): Config()
    }
}

@arkivanov arkivanov added the question Further information is requested label Aug 29, 2023
@Serpov
Copy link
Author

Serpov commented Aug 29, 2023

Unfortunately, we cannot use sealed class for config declaration, because Data classes can be provided in different feature modules.

@Serpov
Copy link
Author

Serpov commented Aug 29, 2023

Something like this

interface Feature1Contract {
       data class State(val data: Int)
       data class Config(val data: Int)
}

interface Feature2Contract {
       data class State(val data: Int)
       data class Config(val data: Int)
}

@arkivanov
Copy link
Owner

I see. It's advised to keep child configurations as implementation details in the parent component responsible for navigation. The parent is responsible for instantiating its children, and so it's the parent's responsibility to store all required information for child instantiation.

E.g. consider the following case.

class SomeChild(text: String)
class Parent1Child(...) {
    private val navigation = StackNavigation<Config>()

    val stack: Value<ChildStack<*, SomeChild>> = childStack(source = navigation, ...) { config, ctx ->
        SomeChild(text = config.text)
    }

    fun push(text: String) {
        navigation.push(Config(text = text))
    }

    private data class Config(val text: String) : Parcelable
}
class Parent1Child(...) {
    private val navigation = SlotNavigation<Config>()

    val stack: Value<ChildSlot<*, SomeChild>> = childSlot(source = navigation, ...) { config, ctx ->
        SomeChild(text = "Some text")
    }

    fun show() {
        navigation.activate(Config)
    }

    private data object Config : Parcelable
}

As you can see, it might be necessary to have different kind of configurations for the same child component. So it's definitely a responsibility of the parent.

Regarding the key generation. The best thing we can do is to add a selector function for Children, which will allow you to specify a stable key for each configuration.

fun Children(
    ...,
    key: (C) -> String = {  <default impl here> },
    ...
)

Would this work for you?

@Serpov
Copy link
Author

Serpov commented Aug 29, 2023

Selector function for Children is a good idea and I think suitable for any cases with key generation. Thanks a lot

@arkivanov
Copy link
Owner

After further investigation, it appears that for JS we can use this::class.js.name (which gives unqiue JS name), then for other platforms we can use this::class.qualifiedName (which returns a full class name including package). I will investigate this further and improve. Thanks for raising!

@arkivanov arkivanov added enhancement New feature or request and removed question Further information is requested labels Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants