Status Update
Comments
jb...@google.com <jb...@google.com> #2
Branch: androidx-master-dev
commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019
Change the way to detect mismatch between POJO and query
This fixes cursor mismatch warnings with expandProjection.
Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt
ja...@icloud.com <ja...@icloud.com> #3
jb...@google.com <jb...@google.com> #4
Branch: androidx-master-dev
commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020
Re-factor how errors are dismissed when query is re-written
This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically (
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.
This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.
Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f
M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
ja...@icloud.com <ja...@icloud.com> #5
jb...@google.com <jb...@google.com>
cl...@google.com <cl...@google.com> #6
Do you have a sample app with your specific refactors to reproduce that error?
I followed the steps to repro and I do get a long list in backStackStates
where About
page is stored multiple times. But that is because restoreState = false by default when navigating to About
. If you set something like this in Title.kt
view.findViewById<Button>(R.id.about_btn).setOnClickListener {
findNavController().navigate(R.id.action_title_to_about, null, navOptions { restoreState = true })
}
Then backStackStates
gets cleared when you navigate back to About. I'm not sure if what you're seeing is different. If so, can you please upload your specific sample?
ja...@icloud.com <ja...@icloud.com> #7
Yes, if you change the navOptions
for the button it will be fine. But under the default case, I suppose the error scenario would be doing the repetition above to fill up the backStackStates
, then navigating to About, then to Leaderboard, and then press Home on the bottom nav bar. This would correctly restore the About page by taking one item from backStackStates
. However, the other About states will still stay in backStackStates
, and can in fact never be restored.
In the example with restoreState = true
, this problem is only avoided because only one About item is able to be added to backStackStates
(while popping up when navigating to Leaderboard) before it's restored (either by going Leaderboard > back Home > About, or Leaderboard > nav item Home).
The code below in executePopOperations
is where the problem is:
if (saveState) {
...
if (savedState.isNotEmpty()) {
...
val firstStateDestination = findDestination(firstState.destinationId)
generateSequence(firstStateDestination) { destination ->
if (destination.parent?.startDestinationId == destination.id) {
destination.parent
} else {
null
}
}.takeWhile { destination ->
// Only add the state if it doesn't already exist
!backStackMap.containsKey(destination.id)
}.forEach { destination ->
backStackMap[destination.id] = firstState.id
}
// And finally, store the actual state itself
backStackStates[firstState.id] = savedState
}
}
The firstState.id
is added to backStackMap
only if the state is not already saved, as per the comment. In the repetition case above, the About state will already be saved the second time navigating from About > Leaderboard, hence firstState.id
will not be saved in this case. However savedState
will still be added to backStackStates
regardless (the last line).
Both of the restoreStateInternal
functions contain:
val backStackId = backStackMap[id]
// Clear out the state we're going to restore so that it isn't restored a second time
backStackMap.values.removeAll { it == backStackId }
val backStackState = backStackStates.remove(backStackId)
...
val entries = instantiateBackStack(backStackState)
return executeRestoreState(entries, args, navOptions, navigatorExtras)
The first line gets the (single) saved id that is the key to the saved state array in backStackStates
, then the fourth line removes this (single) saved state from backStackStates
. This is the only place in NavController
in which anything is removed from backStackStates
(besides activity recreation). Since there is never going to be any backStackId
that is the key for any of the other saved backStackStates
for About, they will remain unreachable/unrestorable. The keys for these never got saved in backStackMap
thanks to the takeWhile
in executePopOperations
above.
Obviously this is problematic because all of these states are just kept in memory for the life of the NavController
object even though it is impossible that they will ever be used again. And in fact they are even all saved and all restored in saveState
and restoreState
if the activity is recreated.
I assume the solution would be something similar to:
val firstStateDestination = findDestination(firstState.destinationId)
generateSequence(firstStateDestination) { destination ->
if (destination.parent?.startDestinationId == destination.id) {
destination.parent
} else {
null
}
}.takeWhile { destination ->
// Only add the state if it doesn't already exist
!backStackMap.containsKey(destination.id)
}.forEach { destination ->
backStackMap[destination.id] = firstState.id
}
if (backStackMap.values.contains(firstState.id)) {
backStackStates[firstState.id] = savedState
}
ap...@google.com <ap...@google.com> #8
Branch: androidx-main
commit f8c57f0c5825b0d10783fa4786b34860e02cc847
Author: Clara Fok <clarafok@google.com>
Date: Wed Dec 13 15:37:04 2023
Fix backStackState leak when saveState
We now store an BackStackState only if the entry it is associated with has been actually added to the BackStackMap. This helps prevent leaking backStackStates that never gets restored.
Test: ./gradlew navigation:navigation-runtime:cC
Bug: 309559751
Relnote: "Fixed BackStackState leak where multiple saveState calls on a destination would result in multiple states to be saved, but only the first one could be restored."
Change-Id: I598b01c379f61818e96126fb8e3d3ef1f9d084cd
M navigation/navigation-runtime/src/androidTest/java/androidx/navigation/NavControllerTest.kt
M navigation/navigation-runtime/src/main/java/androidx/navigation/NavController.kt
cl...@google.com <cl...@google.com> #9
Fixed internally and will be available in navigation 2.8.0-alpha01
.
il...@google.com <il...@google.com>
na...@google.com <na...@google.com> #10
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.navigation:navigation-runtime:2.8.0-alpha01
Description
Version used: 2.5.3
Devices/Android versions reproduced on: Any
When popping the back stack with saveState=true, destinations are only added to backStackMap if they are not already present (courtesy of the takeWhile operations in popBackStackInternal). The savedState array is subsequently added to backStackStates regardless. In the case that no destinations are added to backStackMap, the savedState array subsequently becomes unreachable/unrestorable, as there is no reference to the back stack name/uuid key anywhere.
To reproduce the issue:
(1) Launch the NavigationAdvancedSample app in debug mode
(2) Repeat the following several times:
(a) Tap the "About" button to navigate to the about screen
(b) Tap the "Leaderboard" bottom navigation item to navigate to the leaderboard screen
(b) Tap the back button or swipe back to navigate to the title screen again
(3) After several repeats, inspect the navController's backStackStates (which will be sizeable) and backStackMap (the values of which will not contain most of backStackStates' keys, rendering them indefinitely unreachable in restoreStateInternal
Potential fix:
Make "backStackStates[firstState.id] = savedState" conditional on at least one addition to backStackMap