The recommended substitute for clearTask of popUpTo with popUpToInclusive only works from start destination actions [116831650]

Fixed

Bug

Status Update

No update yet.

Description

[Deleted User]

created issue #1

Sep 28, 2018 10:32AM

I was previously using clearTask in my navigation actions to make sure that the user could not navigate back to the previous destination (useful for splash screens and login screens). After it was deprecated I decided to try the recommended approach of instead setting popUpTo pointing to the root of the graph together with setting popUpToInclusive="true", but this didn't have the effect I wanted.

This approach only seem to work on actions originating from the graph's start destination, and fails on actions between any other destinations. I created a sample project which reproduces the issue:

https://github.com/hsson/android-navigation-component-alpha06-bug

Here is the project's README detailing the issue:

# Bug in Android Navigation Component (alpha06)
(Note: I have only verified this in alpha06, I don't know if it exists in
previous versions)

## The problem
Basically the problem is that the NavOption "clearTask" was removed in
alpha02, and the recommended substitute was to use app:popUpTo pointing to
the root of the graph/the graph ID, together with app:popUpToInclusive="true".
However, this method does not work. It only seems to work for actions
going from the graph's start destination. If we have the following graph:

x y
A ---> B ---> C

Where A, B, and C are fragments, A is the start destination of the grapg,
and x, y are actions going from A to B and B to C respectively. Let's say
the graph's ID is "@+id/graph"

Then, if both actions x and y have
app:popUpTo="@+id/graph"
app:popUpToInclusive="true"

Then when navigating from A to B through x, and then hitting the back button
will exit the app (as expected). But if navigating from A to B to C through
x and y, and then hitting the back button we will navigate back to B — this
is not what was expected. The expected behavior with this graph is to always
exit the app when clicking the back button

## Environment
This was verified using:
Android Studio 3.2 RC 3
Build #AI-181.5540.7.32.4987877, built on August 31, 2018
JRE: 1.8.0_152-release-1136-b06 x86_64
JVM: OpenJDK 64-Bit Server VM by JetBrains s.r.o
macOS 10.13.6

Comments

[Deleted User] <[Deleted User]> #2Oct 1, 2018 08:14AM

Project: platform/frameworks/support
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

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

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

il...@google.com <il...@google.com> Oct 1, 2018 10:56PM

Assigned to il...@google.com.

[Deleted User] <[Deleted User]> #3Oct 10, 2018 09:20AM

The following changes were cherrypicked through Coastguard (

https://android-build.googleplex.com/coastguard/)
Release Track: (

https://android-build.googleplex.com/coastguard/dashboard/6093116828680192/#/cherrypickHistory/all)
Created: 2019-10-14 17:23:42.610596+00:00
Changes: aosp/1123258

il...@google.com <il...@google.com> #4Oct 18, 2018 06:05PM

Project: platform/frameworks/support
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 (

b/140759491

).
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

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

[Deleted User] <[Deleted User]> #5Oct 19, 2018 11:44AM

Alright, interesting! So that means there is no generic way to say that "clear everything of the backstack before moving to this next destination"? Rather, you need to keep track of every individual case? What if, for example, you have to following graph:
x y z
A --> B --> C --> D

Where
- A could be a splash screen and the start destination of the graph
- B could be some welcome screen, prompting the user to sign in
- C could be the actual login screen where the user enters his/hers details and signs in
- D could be the main content screen

Obviously the user should not be allowed to click the back button from B to go back to the splash screen, and neither should the user be allowed to click the back button from D to go back to the login screen. In both of these cases the user should instead exit the app. However, it's perfectly fine for the user to click back from C and go back to the welcome screen B.

Now, to the problem; with your suggested workaround we can indeed set popUpTo="@+id/a" on x, but if we set popUpTo="@+id/c" on z then the user can still click the back button, but they will go to B instead. The desired behaviour is for the user to exit the app when clicking back when in D. The workaround stops working as we don't pop the back stack when going from B --> C, as we want to allow the user to navigate back to B from C.

mi...@gmail.com <mi...@gmail.com> #6Oct 19, 2018 12:22PM

It would be interesting to have a "popAll" option, where the whole stack will be erased, so we don't have to keep track who's on the stack to be popped or not. It would speed up things and bring less confusion as well.

[Deleted User] <[Deleted User]> #7Oct 19, 2018 12:24PM

I agree completely, similarly to how clearTask worked before it was deprecated.

il...@google.com <il...@google.com> #8Oct 19, 2018 03:36PM

A couple of points:
1) There really is a bug here - app:popUpTo="@+id/graph" should work as well - that's why I said 'workaround' above, not solution. We will be fixing that bug.

2) Popping everything off the stack is always a sign that you've built your graph incorrectly. That's on us though as we haven't documented well enough how to do conditional navigation (for example, the B->C sign in flow).

https://issuetracker.google.com/issues/111762510 is our tracking bug for updating the conditional navigation page (

https://developer.android.com/topic/libraries/architecture/navigation/navigation-conditional).

For example, in the example raised in #5, your start destination should be D.
- Splash screen should be using a splash theme (e.g.,

https://www.bignerdranch.com/blog/splash-screens-the-right-way/) and not be in your graph at all
- B+C should be handled as conditional navigation from D - D starts B and if you return to D without logging in, D calls finish().
- Having D as your startDestination is critical to having the right back stack when deep linking to somewhere in your graph

I realize that this is a bigger mental shift from how you've structured your apps and is a big reason why improving the documentation is an important focus for Navigation over the next few months.

mi...@gmail.com <mi...@gmail.com> #9Oct 19, 2018 03:50PM

So if I understand this correctly, D is a Fragment and it should call its Activity (the one and only one in the app) finish(), in case there's something wrong, like going back from the login screen?

I find this complicated and can lead to issues that resemble "callback hell", having multiple checks and flags everywhere, to know if we already navigates to some destination and if we are back and without a proper value... Can't we come up with a more streamlined solution?

il...@google.com <il...@google.com> #10Oct 19, 2018 04:13PM

D can do whatever it needs to if the user chooses not to log in, be it finishing the activity (if you know it is the root of your whole graph), calling navController.popBackStack(), or showing an inline prompt for the user to sign in. This is core business logic for your app.

One thing that you need to keep in mind is that the user can hit the home button at any point then come back to *exactly that same destination in your app* an arbitrary amount of time later. If your logins expire or if users can delete their accounts (just to offer two examples), then *every* login required destination needs to handle this exact case - punting users to your login flow then handling whether they log in or not before showing their protected content. By making that the *only* flow for handling invalid login cases in your app, you can do things the right way just once.

mi...@gmail.com <mi...@gmail.com> #11Oct 19, 2018 04:16PM

Good point.

dc...@gmail.com <dc...@gmail.com> #12Oct 23, 2018 05:19PM

About your comment on #8 (mentioned bellow):

>>> - B+C should be handled as conditional navigation from D - D starts B and if you return to D without logging in, D calls finish().

Basically D needs to know the outcome of a task that was accomplished in B, meaning D needs to know if it was reached through opening the app (since it's a startDestination) or through pressing back in B (meaning the user canceled the login process).

Wouldn't this be a break in abstraction? My understanding was that a destination should receive it's arguments and this should be enough for it to work but in this example D needs to detect it was reached through a BACK press in B.

I don't even know how I would implement such a thing unless I start passing arguments between destinations to know about my navigation history.

da...@gmail.com <da...@gmail.com> #13Oct 26, 2018 10:58PM

I made this work, kind of: splash to main activity; starting destination D. It inflates a layout only to save state and go to the back stack as I bounce to B. If I press back, I go through the steps of restoring D--which briefly makes an appearance on screen--only to finish the activity.

I actually like the thought of an incorrect graph if it lets me handle this stuff precisely without some of the overhead needed to make a generic solution.

il...@google.com <il...@google.com> #14Nov 30, 2018 11:21PM

Marked as fixed.

I've confirmed that this is fixed in alpha08 as a result of

https://android-review.googlesource.com/833717 and a number of other internal changes.

app:popUpTo="@+id/graph" should always work now.