Wrong behavior when Deep Link contains get query parameters [110057514]

Fixed

Bug

Status Update

No update yet.

Description

dj...@gmail.com

created issue #1

Jun 12, 2018 10:43AM

Component used: Android Architecture Components Navigation
Version used: 1.0.0-alpha02
Devices/Android versions reproduced on:
Nexus 5 w 7.1 (Lineage OS)

In my project I created navigation graph as follows: Pages List -> Single Page -> Page Comments.
Each of destinations have their own Deep Link with some parameters in path:
1) Pages List: <deepLink app:uri="

myapp.com" />
2) Single Page: <deepLink app:uri="

myapp.com/{title}" /> (note that {title} matches destination argument)
3) Page Comments: <deepLink app:uri="

myapp.com/{title}/comments" /> (note that {title} matches destination argument)
And with those path arguments all is OK.

Then if we try to replace path arguments with get query parameters - strange things will begin to happen.
For example if we change Single Page deep link to this `<deepLink app:uri="

myapp.com/test/path/param?title={title}" />` - it'll always open root destinations (which is just

myapp.com).
Same for Page Comments, if we would add atleast one get query param - it'll always open root page (Pages List) when opened from matched Deep Link.
Here is link for demo project:

https://github.com/Ekalips/DeepUrlNavigatorTest/tree/broked_get_param (please note that it's special branch where deep link navigation is broken because of GET query parameter)

Comments

il...@google.com <il...@google.com> Jun 20, 2018 08:01PM

Assigned to il...@google.com.

il...@google.com <il...@google.com> Aug 14, 2018 08:36PM

Reassigned to wk...@google.com.

[Deleted User] <[Deleted User]> #2Sep 29, 2018 01:05AM

Activity destinations should be considered exit points for your navigation graph - they are not part of the NavController's back stack and therefore not being sent to OnNavigatedListeners is working as intended.

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

Accepted by il...@google.com.

ap...@google.com <ap...@google.com> #3Oct 12, 2018 10:04AM

Thank you for your explanation, but although that is the intended behavior, it is still confusing.
In my opinion, the developer expects to be notified when any navigation action defined in the graph is performed, regardless the destination.

Although that action is considered an exit point, it is an action defined within the graph and it is indeed a navigation action happening in that graph, so I don't understand why it is not notified to the "OnNavigated" listener when it has indeed navigated, regardless if it affects or not the back stack.

I that is the intended behaviour, then it should be rather called something like "NavController.OnBackStackChangedListener".

il...@google.com <il...@google.com> #4Oct 12, 2018 04:38PM

Marked as fixed.

Yeah, naming is hard :)

Right now, NavController.OnNavigatedListener gives you a NavDestination that always equals navController.getCurrentDestination(). It doesn't give you what action you triggered, arguments, or anything else (including operations that don't change getCurrentDestination(), such as singleTop actions or activity destinations), so I could definitely see this more as a OnDestinationChangedListener.

I'll reopen this to track renaming the listener to something more appropriate to what it actually is doing or updating the documentation to make its role more clear.

il...@google.com <il...@google.com> #5Oct 12, 2018 04:39PM

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

commit fcc0c6059e31f7e53396db8cce0ac62d76a1c2d8
Author: Ian Lake <ilake@google.com>
Date: Fri Nov 30 13:14:58 2018

Rename OnNavigatedListener

The callback to onNavigated() is not only
called in result to a navigate() call, but
is actually called whenever any navigation
event happens, be it single top navigation
events or popBackStack events.

Therefore the name should reflect what is
actually happening.

Test: existing tests still pass
BUG: 118670572
Change-Id: Iad45d646e859428a4f744992821916906ca2d34f

M navigation/integration-tests/testapp/src/main/java/androidx/navigation/testapp/NavigationActivity.kt
M navigation/runtime/api/1.0.0-alpha08.txt
M navigation/runtime/api/current.txt
M navigation/runtime/src/main/java/androidx/navigation/NavController.java
M navigation/ui/src/main/java/androidx/navigation/ui/AbstractAppBarOnDestinationChangedListener.java
M navigation/ui/src/main/java/androidx/navigation/ui/ActionBarOnDestinationChangedListener.java
M navigation/ui/src/main/java/androidx/navigation/ui/CollapsingToolbarOnDestinationChangedListener.java
M navigation/ui/src/main/java/androidx/navigation/ui/NavigationUI.java
M navigation/ui/src/main/java/androidx/navigation/ui/ToolbarOnDestinationChangedListener.java

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

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

lu...@gmail.com <lu...@gmail.com> #6Nov 30, 2018 12:45PM

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

commit 1a5551e0e3145b1e309577a1282d28fa1b14e956
Author: Ian Lake <ilake@google.com>
Date: Mon Dec 03 13:22:31 2018

Correct naming of add/removeOnDestinationChangedListener

Ensure that the add and remove methods
use the same naming convention as the
listener class itself

Test: ./gradlew bOS
BUG: 118670572
Change-Id: I71dd174f7abd3a870c12b26c0aef64ff1512ceea

M navigation/integration-tests/testapp/src/main/java/androidx/navigation/testapp/NavigationActivity.kt
M navigation/runtime/api/1.0.0-alpha08.txt
M navigation/runtime/api/current.txt
M navigation/runtime/src/main/java/androidx/navigation/NavController.java
M navigation/ui/src/main/java/androidx/navigation/ui/AbstractAppBarOnDestinationChangedListener.java
M navigation/ui/src/main/java/androidx/navigation/ui/CollapsingToolbarOnDestinationChangedListener.java
M navigation/ui/src/main/java/androidx/navigation/ui/NavigationUI.java
M navigation/ui/src/main/java/androidx/navigation/ui/ToolbarOnDestinationChangedListener.java

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

https://goto.google.com/android-sha1/1a5551e0e3145b1e309577a1282d28fa1b14e956

il...@google.com <il...@google.com> #7Nov 30, 2018 04:53PM

We've renamed the listener to OnDestinationChangedListener to make it more clear that it is only called when NavController's getCurrentDestination() or its arguments change, precluding activity destinations.