SafeArg Builder classes doesn't contain the default values [140123727]

Fixed

Bug

Status Update

No update yet.

Description

an...@gmail.com

created issue #1

Aug 27, 2019 08:16PM

Component used: Navigation
Version used: 2.1.0-rc01
Devices/Android versions reproduced on:

Proposal: Safe Arg generated Builder Classes should have default values inside.

This is actually not a bug for navigation component but a kind of side effect which can make this feature more versatile.
In my scenario, we were trying to use generated safe arg Builder classes to pass arguments to the fragments in our project, as the normal bundle arguments are not type safe and kinda messy to work with.

The project I'm working on is relative large, in our team we decided to migrate gradually to navigation component.
As a first step we wanted to put the fragment in a nav graph just to generate safe args so that we can use those classes to safely pass arguments to our existing fragment with fragment transaction and eventually use the graph in future point. So we are trying to use Safe arg as mentioned below:

ChoosePackageFragmentArgs args =
new ChoosePackageFragmentArgs.Builder("testName", "testPackage")
.setIsExtend(true)
.build();
ChoosePackageFragment fragment = new ChoosePackageFragment();
fragment.setArguments(args.toBundle());

getSupportFragmentManager().beginTransaction()
.replace(R.id.container, fragment)
.addToBackStack(null)
.commit();

As you can see from the above example the "isExtend" property is optional and has a default value false which is set in the graph like this,

<argument
android:name="isExtend"
app:argType="boolean"
android:defaultValue="false" />

However if we use the generated safe arg builder class (ChoosePackageFragmentArgs) to pass the arguments to ChoosePackageFragment, without setting "isExtend" the ChoosePackageFragmentArgs.fromBundle(requireArguments()).getIsExtend() throws a NullPointerException because the builder doesn't set the default value when we call the args.toBundle() on the builder instance.

Moreover, when we try to call the getter method it can't find a value from the map, so it tries to cast a null value to (boolean), thus throwing the NullPointerException. Remember we're not using a NavController, just trying to take advantage of SafeArg feature.

This is the code from ChoosePackageFragmentArgs.Builder:

public class ChoosePackageFragmentArgs implements NavArgs {
....
@SuppressWarnings("unchecked")
public boolean getIsExtend() {
return (boolean) arguments.get("isExtend");
}
...
}

We were wondering why the default value is not present in the object. After digging for a while we've found that NavController.navigate() pulls the default values from NavAction.getDefaultArguments(); as we're not using NavComponent we're actually not getting the default value from Builder. Here's the code that pulls default args in NavComponent Library.

Code from NavController.java:

final NavAction navAction = currentNode.getAction(resId);
Bundle combinedArgs = null;
if (navAction != null) {
if (navOptions == null) {
navOptions = navAction.getNavOptions();
}
destId = navAction.getDestinationId();
Bundle navActionArgs = navAction.getDefaultArguments();
if (navActionArgs != null) {
combinedArgs = new Bundle();
combinedArgs.putAll(navActionArgs);
}
}

Suggestion:
If the safe arg plugin added the default values to the Builder while generating those SafeArgs Builder classes, the conventional fragment approach could get huge benefit out of it, and as these classes are accessible apis, I believe it makes more sense to have predictable behaviour for Builder classes as that seems like a standard predictable behaviour.

Comments

il...@google.com <il...@google.com> #2Aug 28, 2019 06:24PM

Assigned to jb...@google.com.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

an...@gmail.com <an...@gmail.com> #3Aug 28, 2019 09:00PM

I just checked with kotlin codegen, it is working fine. I agree, it seems like a problem solely with java codegen. It doesn't comply with the Kotlin generated SafeArg data classes.

ap...@google.com <ap...@google.com> #4Oct 4, 2019 07:16PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit 749f41de3d69a818277551fdbe11c3856cb9dfb9
Author: Jeremy Woods <jbwoods@google.com>
Date: Fri Oct 04 10:48:26 2019

Generate default values for safe_args in Java

Currently the safe args generation for java code does not contain
default Arg values. The default arg values are provided by the Kotlin
safe args generation and we should be as uniform as possible across both
languages.

Test: ./gradlew --rerun-tasks navigation:navigation-safe-args-gradle-plugin:test
navigation:navigation-safe-args-generator:test
BUG: 140123727
Change-Id: I43447755e29d6640e227b5608ca2fd3ea1a45e0b

M navigation/navigation-safe-args-generator/src/main/kotlin/androidx/navigation/safe/args/generator/java/JavaNavWriter.kt
M navigation/navigation-safe-args-generator/src/tests/test-data/expected/java_nav_writer_test/MainFragmentArgs.java

https://android-review.googlesource.com/1133164

https://goto.google.com/android-sha1/749f41de3d69a818277551fdbe11c3856cb9dfb9

jb...@google.com <jb...@google.com> #5Oct 4, 2019 07:17PM

Marked as fixed.

This has been fixed internally and will be available in the Navigation 2.2.0-beta01 release.

an...@gmail.com <an...@gmail.com> #6Oct 7, 2019 04:34PM

Many many thanks for fixing this. Waiting for the release.

ra...@gmail.com <ra...@gmail.com> #7Nov 18, 2019 04:34PM

Null Default value for a string, returns "@null"
<argument
android:name="deepLinkToken"
android:defaultValue="@null"
app:argType="string"
app:nullable="true" />
```
And if I do
```
val args by navArgs<FragmentArgs>()
args.deepLinkToken // returns "@null@
```
Using '2.2.0-rc02'

jb...@google.com <jb...@google.com> #8Nov 18, 2019 07:14PM

Re #7 - please file a separate bugs with a project that reproduce your separate issue. If the issue you are mentioning is covered by

https://issuetracker.google.com/issues/141613546, feel free to +1.