Navigation subgraphs do not use the arguments of their startDestination [109505019]

Fixed

Bug

androidx-bug-bounty

Status Update

No update yet.

Description

sk...@gmail.com

created issue #1

Jun 3, 2018 11:25PM

Component used: Navigation, SafeArgs
Version used: 1.0.0-alpha01
Devices/Android versions reproduced on: N/A

To reproduce:
1. Construct a navigation graph with a fragment and mark it as the startDestination for the graph.
2. Construct a subgraph with another fragment that requires an argument and mark it as the startDestination for the subgraph.
3. Construct an action from the first fragment to the subgraph.
4. Use the generated classes to construct the action. Observe that the argument is not required.
5. Move the argument from the fragment to the subgraph.
4. Use the generated classes to construct the action. Observe that the argument *is* required.

I believe that in both cases, the argument should be required because the purpose of the SafeArgs generated classes is to prevent missing arguments, which would occur if the subgraph did not require the arguments of its startDestination.

Comments

il...@google.com <il...@google.com> #2Jun 4, 2018 04:32PM

Assigned to se...@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

jb...@google.com <jb...@google.com> #3Jun 4, 2018 04:35PM

This will have some tools impact. Please reassign to me or create another bug when you know what the approach will be.

sk...@gmail.com <sk...@gmail.com> #4Jun 4, 2018 04:50PM

My current workaround is to duplicate the arguments. That lets me use the correct names of classes for sending and receiving the bundles.

al...@gmail.com <al...@gmail.com> #5Jul 12, 2018 05:46AM

I'm having a similar issue. If I include a NavGraph from another file, and its home destination has arguments, these aren't generated for the NavDirections to the sub graph. If I nest a set of destinations in the same NavGraph file, then I can set global arguments for the nested sub graph and these will be generated for the NavDirections.

I agree with the original post as well as ensuring the SafeArgs will be generated regardless if the sub graph is located in another file, or the same file. I've included a demo app that demonstrates the issue (main_nav_graph.xml, nested_nav_graph.xml, and OrangeFragment.kt)

safe-args-bug-master.zip

137 KB

Download

il...@google.com <il...@google.com> Jul 12, 2018 11:21PM

Reassigned to da...@google.com.

j....@gmail.com <j....@gmail.com> #6Jul 9, 2019 08:10AM

To solve this issues I'm using the same workaround

xa...@gmail.com <xa...@gmail.com> #7Aug 2, 2019 05:32PM

Same behaviour here with '2.1.0-beta02' version. Nested navigation in the same file makes safe-args add the argument while referencing another file doesn't.

xa...@gmail.com <xa...@gmail.com> #8Aug 2, 2019 05:47PM

Another possible workaround would be to add the <argument> within the very same <action> who targets the other graph (different file). Is this a workaround or the official solution? I guess it would be much better that safe-args would check and add that argument into the auto-generated directions based on the starting destination of the other graph.

[Deleted User] <[Deleted User]> #9Mar 4, 2020 01:09AM

+1 any updates?

il...@google.com <il...@google.com> Mar 4, 2020 11:41PM

Reassigned to jb...@google.com.

be...@gmail.com <be...@gmail.com> #10Oct 20, 2020 08:11PM

Any updates?

da...@khol.me <da...@khol.me> #11Oct 23, 2020 09:04AM

The workaround to put an <argument> directly to the root of the <navigation> holding the destination <fragment> does not work, at least not in version 2.3.0.

The other workaround to copy <argument> to the source <fragment>'s <action> works but it defeats the purpose of SafeArgs being safe. With that approach, you potentially have to update arguments in multiple source <fragment>s that navigate to the same destination <fragment>. If you forget to update some of the sources, the app will compile just fine but crash at runtime. That's just UnsafeArgs.

je...@gmail.com <je...@gmail.com> #12Nov 6, 2020 12:31PM

Any updates? Don't like the approach to UnsafeArgs either

og...@gmail.com <og...@gmail.com> #13Dec 5, 2020 04:53AM

Any updates?

mt...@parsyl.com <mt...@parsyl.com> #14Jan 7, 2021 01:40AM

Any updates on this? This has been open for 2.5 years.

g....@gmail.com <g....@gmail.com> #15Jan 10, 2021 02:58PM

I recently got rid of all the optional arguments and default values, replacing them with Parcelable classes that offer the same functionality leveraging Kotlin's type system. Since there are no compile time checks for start destinations and I had to touch some of them, I quickly put together a python script that checks the arguments of the actions that have a <navigation> as destination.

I wrote the script with my use-case in mind, so the code makes some assumptions that are not generally true. These assumption should only cause false positives, but I really didn't think this through and I could be wrong. It's not perfect and there's definitely room for improvement, but it is enough for me for now.

Please remember that many are watching this issue and not everyone here care about this script. The source code is enough to seek for help elsewhere.

check_nav_graphs.py

5.8 KB

View

Download

nv...@gmail.com <nv...@gmail.com> #16May 14, 2021 03:31AM

A major fuck up for a documented use case.

nv...@gmail.com <nv...@gmail.com> #17May 14, 2021 03:40AM

From this SO, we have a workaround:

Copy <argument> to inside the <navigation> tag, i.e. make it a sibling of other <fragment>
Copy <argument> to inside the <action> of the start fragment. Yes normally <action> is a self close tag, and <argument> is for <fragment>.

And the workaround breaks navArgs(), because StartFragmentOfNestedGraphArgs is not generated.

ca...@gmail.com <ca...@gmail.com> #18Oct 11, 2021 12:10PM

We actually just had a crash in our app, fortunately detected by qa, due to using the unsafe workaround.

[Deleted User] <[Deleted User]> #19Jan 18, 2022 02:30PM

Any update on this? Almost 2.5 years without any real fix !!

ju...@gmail.com <ju...@gmail.com> #20May 19, 2022 12:29PM

Any update on this please ?
That's even more annoying since putting the arguments directly inside de <navigation> tag doesn't work. (using 2.4.1)

[Deleted User] <[Deleted User]> #21Jun 14, 2022 08:16AM

Any update on this please???? 4 years waiting for this!!!!

sp...@gmail.com <sp...@gmail.com> #22Sep 8, 2022 03:31PM

We are still waiting...

sh...@gmail.com <sh...@gmail.com> #23Sep 24, 2022 10:30AM

any update on this?

[Deleted User] <[Deleted User]> #24Feb 28, 2023 07:07PM

is there any update on this, it's been almost 5 years?

ap...@google.com <ap...@google.com> #25Feb 29, 2024 07:14PM

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

commit d1e16c5abced31a3738c9686121f3a2a50c3404e
Author: Clara Fok <clarafok@google.com>
Date: Mon Feb 26 12:18:38 2024

Fix startDestination route args not processed

Filled-in arguments in the route that is passed as a NavGraph's startDestination will now be processed and stored in the bundle.

These start route arguments will overwrite default values but not the others. The priority of args is as follows:
Restored/StartDestinationArgs > Start route args > Default args

Relnote: "You can now pass arguments to NavGraph's startDestination directly in the startDestination route without relying on defaultValue. This applies to nesteded NavGraph startDestinations as well."

Test: ./gradlew navigation:navigation-runtime:cC
Bug: 109505019
Bug: 188693139
Change-Id: I0e0b5a7c2bf2e77a8d7a46c91974509104e742a9

M navigation/navigation-common/src/main/java/androidx/navigation/NavGraphNavigator.kt
M navigation/navigation-runtime/src/androidTest/java/androidx/navigation/NavControllerRouteTest.kt

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

cl...@google.com <cl...@google.com> #26Feb 29, 2024 07:25PM

Marked as fixed.

Fixed internally, available in navigation-2.8.0-alpha05

Message last modified on Feb 29, 2024 08:16PM

jb...@google.com <jb...@google.com> Mar 20, 2024 04:18PM

Reassigned to cl...@google.com.

pr...@google.com <pr...@google.com> #27Mar 20, 2024 08:26PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.navigation:navigation-common:2.8.0-alpha05
androidx.navigation:navigation-runtime:2.8.0-alpha05

e....@gmail.com <e....@gmail.com> #28Nov 8, 2024 06:20AM

Has this really been fixed? SafeArgs still does not generate an action with the startDestination's argument for nested navigation graphs. Tested with navigation 2.8.3

Message last modified on Nov 8, 2024 06:22AM

cl...@google.com <cl...@google.com> #29Nov 12, 2024 10:50PM

comment#28 startDestnation args for nested navigation graphs might be a separate issue. Can you please file a separate bug and upload a minimal repro? Thanks.