AndroidX Navigation 2.6.0 broke dynamic app bar title for non String arguments [316676794]

Fixed

Bug

Status Update

No update yet.

Description

si...@gmail.com

created issue #1

Dec 16, 2023 04:56PM

Component used: Navigation
Version used: androidx.navigation:navigation-*:2.6.0 and above
Devices/Android versions reproduced on: Not applicable

This commit b1ac7d6 introduced a regression in how the title is dynamically rendered, and more precisely how the Bundle containing parameters is accessed.

Previously, it used the default Bundle.get(key) method:

if (argType == NavType.ReferenceType) {
    val value = context.resources.getString(arguments[argName] as Int)
    title.append(value)
} else {
    title.append(arguments[argName].toString())
}

But after this commit, it now uses Bundle.getString(key):

if (argType == NavType.ReferenceType) {
    val value = context.getString(bundle.getInt(argName))
    builder.append(value)
} else {
    builder.append(bundle.getString(argName))
}

Unfortunately, this breaks basic usage, with other types than String, since Bundle.getString(key) will return null.

Here is a very simple example:

<navigation>
    <fragment android:label="Album #{albumId}">
        <argument android:name="albumId" app:argType="long" />
    </fragment>
</navigation>

Previously it would have correctly set the title to "Album #42" (for example).
But since this update, it will render "Album #null"!

Comments

si...@gmail.com <si...@gmail.com> #2Dec 16, 2023 05:20PM

Yigit, do you have time to fix it?

reemission of the same liveData is racy

il...@google.com <il...@google.com> Dec 19, 2023 09:37PM

Assigned to il...@google.com.

ap...@google.com <ap...@google.com> #3Dec 19, 2023 10:21PM

Marked as fixed.

yea i'll take it.

il...@google.com <il...@google.com> #4Dec 19, 2023 10:26PM

Thanks for the detailed analysis. This may not be an issue anymore since we've started using Main.immediate there but I' not sure; I'll try to create a test case.

si...@gmail.com <si...@gmail.com> #5Dec 19, 2023 10:44PM

just emitting same live data reproduces the issue.

@Test
fun raceTest() {
val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData)
emitSource(subLiveData) //crashes
}
subject.addObserver().apply {
testScope.advanceUntilIdle()
}
}

na...@google.com <na...@google.com> #6Jan 24, 2024 07:53PM

With 2.2.0-alpha04 (that use Main.immediate), the issue seems to be still there (I tested it by calling emitSource() twice, like your test case)

si...@gmail.com <si...@gmail.com> #7Jan 27, 2024 11:57AM

yea sorry immediate does not fix it.
I actually have a WIP fix for it:

https://android-review.googlesource.com/c/platform/frameworks/support/+/1112186

if your case is the one i found (emitting same LiveData multiple times, as shown in #5) you can work around it by adding a dummy transformation.

val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData.map {it })
emitSource(subLiveData.map {it} )
}

il...@google.com <il...@google.com> #8Jan 31, 2024 12:08AM

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

commit af12e75e6b4110f48e44ca121466943909de8f06
Author: Yigit Boyar <yboyar@google.com>
Date: Tue Sep 03 12:58:11 2019

Fix coroutine livedata race condition

This CL fixes a bug in liveData builder where emitting same
LiveData source twice would make it crash because the second
emission registry could possibly happen before first one is
removed as source.

We fix it by using a suspending dispose function. It does feel
a bit hacky but we cannot make DisposableHandle.dispose async
and we do not want to block there. This does not mean that there
is a problem if developer disposes it manually since our emit
functions take care of making sure it disposes (and there is
no other way to add source to the underlying MediatorLiveData)

Bug: 140249349
Test: BuildLiveDataTest#raceTest_*

Change-Id: I0b464c242a583da4669af195cf2504e2adc4de40

M lifecycle/lifecycle-livedata-ktx/api/2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/current.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_current.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_current.txt
M lifecycle/lifecycle-livedata-ktx/src/main/java/androidx/lifecycle/CoroutineLiveData.kt
M lifecycle/lifecycle-livedata-ktx/src/test/java/androidx/lifecycle/BuildLiveDataTest.kt

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

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

si...@gmail.com <si...@gmail.com> #9Feb 3, 2024 09:01AM

Understood!

We can manually add it to the release notes though.

It's up to you to decide if it's worth mentioning :)

il...@google.com <il...@google.com> #10Feb 5, 2024 10:58PM

We've added it to the 2.8.0-alpha01 release notes. Thanks again for the contribution!

ju...@gmail.com <ju...@gmail.com> #11Dec 2, 2024 05:47PM

juma