NavController.OnNavigatedListener not called when navigating to an activity destination [118670572]

Fixed

Feature Request

Status Update

No update yet.

Description

gi...@gmail.com

created issue #1

Oct 30, 2018 09:16PM

Component used: Navigation (fragment-ktx and ui-ktx packages)
Version used: 1.0.0-alpha07
Devices/Android versions reproduced on: HTC One, API 21

When the defined destination for an action in a given graph is another activity, the NavController.OnNavigatedListener registered for the origin Activity NavController is never called.

Although the new Activity has its own NavController, I think this navigation event is also relevant for the origin Activity NavController.

Comments

il...@google.com <il...@google.com> #2Oct 30, 2018 09:26PM

Status: Won't Fix (Intended Behavior)

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

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

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

gi...@gmail.com <gi...@gmail.com> #3Oct 31, 2018 09:11AM

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> #4Nov 1, 2018 12:24AM

Assigned to il...@google.com.

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> Nov 30, 2018 09:39PM

Accepted by il...@google.com.

ap...@google.com <ap...@google.com> #5Dec 3, 2018 09:13PM

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

ap...@google.com <ap...@google.com> #6Dec 4, 2018 12:08AM

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> #7Dec 4, 2018 12:10AM

Marked as fixed.

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.