Add Toolbar equivalent to NavigationUI.setupActionBarWithNavController [109868820]

Fixed

Feature Request

Status Update

No update yet.

Description

be...@upactivity.com

created issue #1

Jun 7, 2018 09:47PM

Component used: android.arch.navigation:navigation-ui
Version used:1.0.0-alpha02
Devices/Android versions reproduced on: Huawei P10+ Android 8.0

I have a single activity application with several fragments. They don't all have the same toolbar behavior/size/whatever so the main activity layout is a coordinator layout only:

<?xml version="1.0" encoding="utf-8"?>
<android.support.design.widget.CoordinatorLayout
android:id="@+id/main_activity_coordinator_layout"
xmlns:android="

http://schemas.android.com/apk/res/android"
xmlns:app="

http://schemas.android.com/apk/res-auto"
xmlns:tools="

http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:fitsSystemWindows="true"
tools:context=".app.MainActivity">

<fragment
android:id="@+id/main_activity_nav_host"
android:name="androidx.navigation.fragment.NavHostFragment"
android:layout_width="match_parent"
android:layout_height="match_parent"
app:defaultNavHost="true"
app:layout_behavior="@string/appbar_scrolling_view_behavior"
app:navGraph="@navigation/main_activity"/>

</android.support.design.widget.CoordinatorLayout>

Each fragment layout has an app bar layout and a toolbar.
In the main activity onCreate method I have the following:

NavigationUI.setupActionBarWithNavController(this, navController)

navController is a reference to the navigation controller of this activity.
This creates a NPE at ActionBarOnNavigatedListener.onNavigated(NavigationUI.java:284) because there is no action bar yet.

E Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'void android.support.v7.app.ActionBar.setTitle(java.lang.CharSequence)' on a null object reference
E at androidx.navigation.ui.NavigationUI$ActionBarOnNavigatedListener.onNavigated(NavigationUI.java:284)
E at androidx.navigation.NavController.addOnNavigatedListener(NavController.java:206)
E at androidx.navigation.ui.NavigationUI.setupActionBarWithNavController(NavigationUI.java:152)
E at androidx.navigation.ui.NavigationUI.setupActionBarWithNavController(NavigationUI.java:130)
E at xxx.app.MainActivity.onCreate(MainActivity.kt:161)
E at android.app.Activity.performCreate(Activity.java:7372)
E at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1218)
E at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3147)
E ... 9 more

Comments

il...@google.com <il...@google.com> #2Jun 7, 2018 09:54PM

Assigned to il...@google.com.

Yigit, do you have time to fix it?

reemission of the same liveData is racy

be...@upactivity.com <be...@upactivity.com> #3Jun 7, 2018 10:20PM

yea i'll take it.

il...@google.com <il...@google.com> #4Jun 7, 2018 10:42PM

Thanks for the detailed analysis. This may not be an issue anymore since we've started using Main.immediate there but I' not sure; I'll try to create a test case.

be...@upactivity.com <be...@upactivity.com> #5Jun 7, 2018 11:06PM

just emitting same live data reproduces the issue.

@Test
fun raceTest() {
val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData)
emitSource(subLiveData) //crashes
}
subject.addObserver().apply {
testScope.advanceUntilIdle()
}
}

be...@upactivity.com <be...@upactivity.com> #6Jun 7, 2018 11:12PM

With 2.2.0-alpha04 (that use Main.immediate), the issue seems to be still there (I tested it by calling emitSource() twice, like your test case)

il...@google.com <il...@google.com> #7Jun 7, 2018 11:15PM

yea sorry immediate does not fix it.
I actually have a WIP fix for it:

https://android-review.googlesource.com/c/platform/frameworks/support/+/1112186

if your case is the one i found (emitting same LiveData multiple times, as shown in #5) you can work around it by adding a dummy transformation.

val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData.map {it })
emitSource(subLiveData.map {it} )
}

il...@google.com <il...@google.com> #8Jun 7, 2018 11:15PM

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

commit af12e75e6b4110f48e44ca121466943909de8f06
Author: Yigit Boyar <yboyar@google.com>
Date: Tue Sep 03 12:58:11 2019

Fix coroutine livedata race condition

This CL fixes a bug in liveData builder where emitting same
LiveData source twice would make it crash because the second
emission registry could possibly happen before first one is
removed as source.

We fix it by using a suspending dispose function. It does feel
a bit hacky but we cannot make DisposableHandle.dispose async
and we do not want to block there. This does not mean that there
is a problem if developer disposes it manually since our emit
functions take care of making sure it disposes (and there is
no other way to add source to the underlying MediatorLiveData)

Bug: 140249349
Test: BuildLiveDataTest#raceTest_*

Change-Id: I0b464c242a583da4669af195cf2504e2adc4de40

M lifecycle/lifecycle-livedata-ktx/api/2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/current.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_current.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_current.txt
M lifecycle/lifecycle-livedata-ktx/src/main/java/androidx/lifecycle/CoroutineLiveData.kt
M lifecycle/lifecycle-livedata-ktx/src/test/java/androidx/lifecycle/BuildLiveDataTest.kt

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

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

be...@upactivity.com <be...@upactivity.com> #9Jun 7, 2018 11:26PM

> I'm not saying that Google+ does things correctly. I'm saying that the discussions with the material design team has made it clear that the navigation drawer is a global navigation pattern that should be available everywhere (in that respect, very similar to how the bottom nav bar should always be visible - also not something apps do consistently).

Very interesting. Thanks for the insights.

> This isn't something Navigation would do automatically, but you can certainly add an OnNavigatedListener via

https://developer.android.com/reference/androidx/navigation/NavController#addonnavigatedlistener and take any action you want, including locking the DrawerLayout closed.

Thanks again. That's what I would have done, I was just wondering if the navigation API had something automatic here. But if the drawer is a global navigation pattern and should always be available, that would be useless.

About these two items, it's weird because none of the apps I have tested quickly today seem to have a global drawer: Inbox, Gmail, Google+, Google Music, Play Store, Messages, Maps, Photos — all of them hide/lock the drawer when opening an item in the app (email, post, track, app, etc).
The only two notable exceptions are docs (spreadsheets etc): they open a new task when opening a document, so that makes sense; and YouTube, they got rid of the drawer altogether.
So that's a bit contradictory with a global navigation pattern, and it doesn't make me want to have a global drawer everywhere.

> I'm proposing making this bug into a feature request for adding a setupToolbarWithNavController type of method if you're okay with that.

That would be awesome. For an activity-managed ActionBar, use `NavigationUI.setupActionBarWithNavController()`; and for a custom Toolbar, use `NavigationUI.setupToolbarWithNavController()`. Both do the same: get information from the navigation graph (label, whether it's the start destination or not, menus, etc).
Is that right?
If so, please convert this to a feature request.

Thanks Ian! (I guess this is you :))

be...@upactivity.com <be...@upactivity.com> #10Jun 8, 2018 10:44AM

Please disregard my remark about the drawer and most apps, I guess I was decaffeinated because I was wrong for almost all of them.

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

Accepted by il...@google.com.

il...@google.com <il...@google.com> #11Jun 26, 2018 03:28PM

Marked as fixed.

A NavigationUI.setupWithNavController(Toolbar, NavController, DrawerLayout) method has been added internally and will be available in alpha03