-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add fromSavedState handle method to generated nav args #122
Conversation
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Proposed Changes
fromSavedState
method for all nav args classes, similar to thefromBundle
one.Testing
Test: Adapted existing tests to also include the new method
Issues Fixed
Fixes: 136967621