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

Add fromSavedState handle method to generated nav args #122

Conversation

simonschiller
Copy link
Contributor

Proposed Changes

  • Generate a fromSavedState method for all nav args classes, similar to the fromBundle one.

Testing

Test: Adapted existing tests to also include the new method

Issues Fixed

Fixes: 136967621

Copy link
Member

@danysantiago danysantiago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this PR!

@@ -248,6 +248,52 @@ class JavaNavWriter(private val useAndroidX: Boolean = true) : NavWriter<JavaCod
addStatement("return $N", result)
}.build()

val fromSavedStateHandleMethod = MethodSpec.methodBuilder("fromSavedStateHandle").apply {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of shared code between this method spec and fromBundleMethod, lets try to combine it by creating a local function with the common code and then invoking it with the distinct, something like this:

fun MethodSpec.Builder.commonFromArgGetCode(
    fromVariableName: String,
    resultVariableName: String,
    arg: Argument,
    argGetBlock: MethodSpec.Builder.() -> Unit
) {
    beginControlFlow("if ($N.contains($S))", fromVariableName, arg.name).apply {
        argGetBlock()
        addNullCheck(arg, arg.sanitizedName)
        addStatement(
            "$resultVariableName.$N.put($S, $N)",
            specs.hashMapFieldSpec,
            arg.name,
            arg.sanitizedName
        )
    }
    if (arg.defaultValue == null) {
        nextControlFlow("else")
        addStatement(
            "throw new $T($S)", java.lang.IllegalArgumentException::class.java,
            "Required argument \"${arg.name}\" is missing and does " +
                "not have an android:defaultValue"
        )
    } else {
        nextControlFlow("else")
        addStatement(
            "$resultVariableName.$N.put($S, $L)",
            specs.hashMapFieldSpec,
            arg.name,
            arg.defaultValue.write()
        )
    }
    endControlFlow()
}

then you can use it like this:

val fromSavedStateHandleMethod = MethodSpec.methodBuilder("fromSavedStateHandle").apply {
    addAnnotation(annotations.NONNULL_CLASSNAME)
    addModifiers(Modifier.PUBLIC, Modifier.STATIC)
    addAnnotation(specs.suppressAnnotationSpec)
    val savedStateHandle = "savedStateHandle"
    addParameter(
        ParameterSpec.builder(SAVED_STATE_HANDLE_CLASSNAME, savedStateHandle)
            .addAnnotation(specs.androidAnnotations.NONNULL_CLASSNAME)
            .build()
    )
    returns(className)
    val result = "__result"
    addStatement("$T $N = new $T()", className, result, className)
    args.forEach { arg ->
        commonFromArgGetCode(
            fromVariableName = savedStateHandle,
            resultVariableName = result,
            arg= arg
        ) {
            addStatement("$T $N", arg.type.typeName(), arg.sanitizedName)
            addStatement("$N = $N.get($S)", arg.sanitizedName, savedStateHandle, arg.name)
        }
    }
    addStatement("return $N", result)
}.build()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done for the JavaNavWriter.

I also checked the KotlinNavWriter and the only thing that could make sense here is the else branch where we read default values, but given that it's only ~7 lines I'd keep it duplicated because I think it makes it more readable that way.

addStatement("$T $N = new $T()", className, result, className)
args.forEach { arg ->
beginControlFlow("if ($N.contains($S))", savedStateHandle, arg.name).apply {
addStatement("$T $N", arg.type.typeName(), arg.sanitizedName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the variable declaration and initialization can be done in the same statement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not anymore with the newest changes using the common addReadSingleArgBlock function 😄

Copy link
Member

@danysantiago danysantiago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants