SafeArgs: partial default values in an action produce wrong parameters order [188855904]

Fixed

Bug

Status Update

No update yet.

Description

gm...@gmail.com

created issue #1

May 21, 2021 08:43AM

Component used: Navigation Version used: 2.3.5

It is often convenient to specify default argument values in an action, for example when multiple actions point to the same destination.

Sometimes we only specify values for some arguments:

<fragment
    android:id="@+id/firstFragment"
    android:name="package.FirstFragment">
    <action
        android:id="@+id/goToSecond"
        app:destination="@id/secondFragment">
        <argument
            android:name="argTwo"
            android:defaultValue="42" />
    </action>
</fragment>
<fragment
    android:id="@+id/secondFragment"
    android:name="package.SecondFragment">
    <argument
        android:name="argOne"
        app:argType="string" />
    <argument
        android:name="argTwo"
        app:argType="integer" />
</fragment>

We then would like to call it like this: FirstFragmentDirections.goToSecond("one!").

But unfortunately, SafeArgs generates code with parameters in the wrong order: argTwo (with default value), then argOne. See attached project.

I suppose better options would be to:

Respect arguments order from the destination (seems more intuitive), or
Always place default parameters last (looks nicer in Kotlin).

TestSafeArgs3.zip

141 KB

Download

Comments

il...@google.com <il...@google.com> #2May 21, 2021 06:51PM

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

ap...@google.com <ap...@google.com> #3Aug 18, 2021 07:19PM

Project: platform/frameworks/support
Branch: androidx-main

commit 008b8791f45b61a6f537aef62a12d775f5f55420
Author: Jeremy Woods <jbwoods@google.com>
Date: Thu Jul 29 18:23:30 2021

Make generated params with defaults last

When generating an action in kotlin from safe args, when there are
multiple parameters we should always make default arguments the last
parameters on the action.

RelNote: "When generating actions, safe args now puts arguments without
default values before those with default values as parameters."
Test: ./gradlew --rerun-tasks navigation:navigation-safe-args-generator:test
Bug: 188855904

Change-Id: Idb69783a3b0efe53de8f3e04a978dd6b979a0e04

M navigation/navigation-safe-args-generator/src/main/kotlin/androidx/navigation/safe/args/generator/kotlin/KotlinNavWriter.kt
M navigation/navigation-safe-args-generator/src/test/kotlin/androidx/navigation/safe/args/generator/KotlinNavWriterTest.kt
A navigation/navigation-safe-args-generator/src/test/test-data/expected/kotlin_nav_writer_test/MainFragmentDefaultParamDirections.kt
M navigation/navigation-safe-args-generator/src/test/test-data/expected/kotlin_nav_writer_test/Next.kt

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

jb...@google.com <jb...@google.com> #4Aug 18, 2021 07:22PM

Marked as fixed.

This has been fixed internally and will be available as part of the Navigation 2.4.0-alpha08 release.

gm...@gmail.com <gm...@gmail.com> #5Nov 10, 2021 03:11PM

Just tested with Navigation 2.4.0-beta02:

Project compiles and runs fine, but the Arctic Fox still highlights goToSecond("one!") as a type mismatch error. Cleaning the project does not help. Could you check it please?

gm...@gmail.com <gm...@gmail.com> #6Apr 25, 2022 09:03AM

Re-tested with 2.4.2 & 2.5.0-beta01 in Bumblebee:

It still works, but Lint now shows two warnings. The second one is in nav_graph.xml for action argument: Element argument is not allowed here

Issue 188855904