Navigation arguments: incorrect code is generated for actions which navigate to fragments with arguments with default values [123551990]

Fixed

Bug

Status Update

No update yet.

Description

du...@gmail.com

created issue #1

Jan 29, 2019 10:56AM

Component used: Navigation
Version used: android.arch.navigation:navigation-safe-args-gradle-plugin:1.0.0-alpha11
Devices/Android versions reproduced on: not related to device/api version, it is code generation problem
Bug description:

navigation file defined like this:
<?xml version="1.0" encoding="utf-8"?>
<navigation xmlns:android="

http://schemas.android.com/apk/res/android"
xmlns:app="

http://schemas.android.com/apk/res-auto"
xmlns:tools="

http://schemas.android.com/tools"
android:id="@+id/nav_graph"
app:startDestination="@id/homeFragment">

<fragment
android:id="@+id/homeFragment"
android:name="no.amedia.newsapp.android.WebPageFragment"
android:label="fragment_home"
tools:layout="@layout/fragment_web_page" >
<argument
android:name="web_url"
app:argType="string"
android:defaultValue="@string/url" />
</fragment>

<action android:id="@+id/action_global_homeFragment"
app:destination="@id/homeFragment" />

....
</navigation>

where url (referenced as "@string/url") is defined as <string name="url" translatable="false">

https://google.com</string> in strings

generates code like this:
class NavGraphDirections private constructor() {
private data class ActionGlobalHomeFragment(val webUrl: String = "@string/url") :
NavDirections {
override fun getActionId(): Int = no.amedia.newsapp.android.R.id.action_global_homeFragment

override fun getArguments(): Bundle {
val result = Bundle()
result.putString("web_url", this.webUrl)
return result
}
}

companion object {
fun actionGlobalHomeFragment(webUrl: String = "@string/url"): NavDirections =
ActionGlobalHomeFragment(webUrl)
}
}

so the problem is webUrl: String = "@string/url" instead of webUrl: String = "

https://google.com" or some dynamic reference to @string/url

the same is with WebPageFragmentDirections generated class:
class WebPageFragmentDirections private constructor() {
companion object {
fun actionGlobalHomeFragment(webUrl: String = "@string/url"): NavDirections =
NavGraphDirections.actionGlobalHomeFragment(webUrl)
}

when I try to use code like this
navController.navigate(WebPageFragmentDirections.actionGlobalHomeFragment())
or
navController.navigate(NavGraphDirections.actionGlobalHomeFragment())
argument value in the fragment is "@string/url" instead of "

https://google.com"
while if I navigate like this
navController.navigate(R.id.homeFragment)
default value from navigation xml is used correctly and I get "

https://google.com"

I hope that I manage to describe the problem in a proper way, it is very simple to reproduce, but if you have any questions just ask

Comments

il...@google.com <il...@google.com> #2Jan 29, 2019 10:06PM

Status: Won't Fix (Intended Behavior)

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

du...@gmail.com <du...@gmail.com> #3Jan 30, 2019 12:06PM

Ok thanks, I understand now, I wasn't aware of argType="reference". Anyway I think it is a bit strange that behavior is different if we navigate using safe args gen code (navController.navigate(WebPageFragmentDirections.actionGlobalHomeFragment())) and code without safe args (navController.navigate(R.id.homeFragment)) if the navigation xml file is the same.

il...@google.com <il...@google.com> #4Jan 31, 2019 07:15PM

Assigned to il...@google.com.

Yes, I think we can make it more explicit that only reference types can have references to other resources.

il...@google.com <il...@google.com> Jan 31, 2019 11:55PM

Accepted by il...@google.com.

ap...@google.com <ap...@google.com> #5Feb 1, 2019 09:15PM

Marked as fixed.

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

commit bb456e84a524d4e5cd241a6a839a1062297b9cf4
Author: Ian Lake <ilake@google.com>
Date: Thu Jan 31 15:46:55 2019

Enforce "reference" types for reference default values

In order to ensure that inflation of a navigation
graph results in the same arguments as Safe Args,
we now enforce usage of the "reference" type for any
argument that has a defaultValue that references
another resource (i.e., @string/app_name, etc).

This makes it much more obvious that the default value
in Safe Args is in the form of R.string.app_name and
any provided override for that should also be in the
form of R.string.some_value.

In these cases, it is expected that developers either
1) Use the ids directly (for example, passing an
R.string to a method that takes a resource id)
2) Specifically call context.getString() with the
resource id they retrieve from their NavArgs instance.

Test: updated NavInflaterTest
Change-Id: I06b8c9a0d1420821b8c0064c22f176788da43d7a
Fixes: 123551990

M navigation/runtime/src/androidTest/java/androidx/navigation/NavInflaterTest.kt
M navigation/runtime/src/androidTest/res/navigation/nav_default_arguments.xml
A navigation/runtime/src/androidTest/res/navigation/nav_invalid_argument_arg_type.xml
A navigation/runtime/src/androidTest/res/navigation/nav_invalid_argument_default_value.xml
M navigation/runtime/src/main/java/androidx/navigation/NavInflater.java

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

https://goto.google.com/android-sha1/bb456e84a524d4e5cd241a6a839a1062297b9cf4

il...@google.com <il...@google.com> #6Feb 1, 2019 09:43PM

Navigation now enforces use of the "reference" type if you have a defaultValue referencing another resource. This ensures that Safe Args and Navigation's runtime behavior are always in sync.

Please star

https://issuetracker.google.com/issues/36994900 to track allowing build time placeholders in XML files.

du...@gmail.com <du...@gmail.com> #7Feb 4, 2019 09:40AM

Ok, great, now it is much more straightforward. Thanks!

ne...@gmail.com <ne...@gmail.com> #8Feb 9, 2019 09:31AM

This "fix" broke our app and our navigation XML, the file looked like the one described in the first message by OP and that's exactly how we wanted it to work.

Let me describe the use case.

Our Fragments load content based on the argument "url" (coming from backend) and the default value is required to load the initial content when the user starts the app.

Previously we had this default value stored in string resources where all the app strings can be easily accessed and reused across the app.

After this "fix" in beta01, we have to hardcode the default path in the navigation XML file and we have lost the ability to reference the same default path in the code (now the string is duplicated in string resources and in the navigation XML file).
Another undesired side-effect is that another argument used the default value of @string/null_ and now this doesn't work anymore forcing us to use an ugly empty string with a space character and in the Fragment check for blank string.

Could you enable resources references again just for default XML arguments for other argTypes?

il...@google.com <il...@google.com> #9Feb 11, 2019 09:56PM

Re #8:
- String, object, and array argument types support using android:defaultValue="@null" for null default values. This continues to be the case.

https://issuetracker.google.com/issues/120627532 tracks adding documentation around that fact
- I've filed

https://issuetracker.google.com/issues/124248602 for supporting an empty (i.e., 0) default value for reference types

Resources cannot be statically resolved at build time - they are inherently only resolvable at runtime, so there's no way for Safe Args to know what default value to add to the generate code. Given that, there's no plans on supporting resource references on anything other than "reference" types.

mo...@gmail.com <mo...@gmail.com> #10Jan 24, 2023 09:28AM

you need to remove the default value from the nav_graph and figure out an alternative logic to set it .. I do not think that would be so hard to do.

[Deleted User] <[Deleted User]> #11
Restricted+
Sep 26, 2024 02:58PM

Comment has been deleted.