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

This requirement for addObserver to be called on the main thread is part of the Lifecycle 2.3.0-alpha06 release notes, which Fragment/Activity depend on:

LifecycleRegistry now verifies that its methods are called on main thread. It was always a requirement for lifecycles of activities, fragments etc. An addition of observers from non-main threads resulted in hard to catch crashes in runtime.

It affects all APIs that use Lifecycle, including Navigation.

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

Thanks! Somehow I've missed that.

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

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit 76230d4cbcf193ad3c01e2fb317a1bb3c571b1de
Author: Ian Lake <ilake@google.com>
Date: Tue Apr 06 12:53:25 2021

Mark navigate/setGraph as @MainThread

With the upgrade to Lifecycle 2.3.1,
Lifecycle now enforces that all creation and
changes to the Lifecycle state (including adding
and removing observers) should be done on the
main thread.

By marking navigate() and setGraph() as
`@MainThread`, we can give developeres better
visibility that these operations should only
be done on the main thread.

Relnote: "Navigation now depends on
[Lifecycle `2.3.1`](/jetpack/androidx/releases/lifecycle#2.3.1)
and now marks `setGraph()` and `navigate()`, the
methods that update the `NavBackStackEntry` `Lifecycle`,
as `@MainThread`, aligning Navigation with the main thread
enforcement introduced in Lifecycle `2.3.0`."
Test: existing and updated tests pass
BUG: 171125856

Change-Id: Ifcbe407d9cb186b50f05cbbd8bc5e11e19115a82

M navigation/navigation-dynamic-features-fragment/src/androidTest/java/androidx/navigation/dynamicfeatures/fragment/DynamicNavHostFragmentTest.kt
M navigation/navigation-runtime/api/current.txt
M navigation/navigation-runtime/api/public_plus_experimental_current.txt
M navigation/navigation-runtime/api/restricted_current.txt
M navigation/navigation-runtime/build.gradle
M navigation/navigation-runtime/src/main/java/androidx/navigation/NavController.kt

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

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

For the upcoming Navigation 2.4.0-alpha01 release, we've made it explicit that setGraph() and navigate() should only be called on the main thread (by adding @MainThread annotations to the methods), thus aligning with the main thread enforcement of Lifecycle 2.3.

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

Project: platform/frameworks/support
Branch: androidx-main

commit 00c608a86c44b066198bd8c1dc7e2aef6088d4e1
Author: Ian Lake <ilake@google.com>
Date: Mon Apr 12 14:04:54 2021

Mark popBackStack and navigateUp as @MainThread

In addition to the navigate() and setGraph()
methods updated in

https://android-review.googlesource.com/1665821
the methods for popping the back stack
should also be marked as @MainThread.

Relnote: "Navigation now depends on
[Lifecycle `2.3.1`](/jetpack/androidx/releases/lifecycle#2.3.1)
and now marks all of the methods that update the
`NavBackStackEntry` `Lifecycle`, including `setGraph()`,
`navigate()`, `popBackStack()`, and `navigateUp()`,
as `@MainThread`, aligning Navigation with the main thread
enforcement introduced in Lifecycle `2.3.0`."
Test: existing and updated tests pass
BUG: 171125856

Change-Id: Ib8bedbb3fe384b28c7441cbd57a0751eba307a2b

M navigation/navigation-runtime/api/current.txt
M navigation/navigation-runtime/api/public_plus_experimental_current.txt
M navigation/navigation-runtime/api/restricted_current.txt
M navigation/navigation-runtime/src/main/java/androidx/navigation/NavController.kt

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

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

Re #6 - please file a new bug with a sample project that reproduces your issue.