Fixed
Status Update
Comments
gg...@google.com <gg...@google.com>
il...@google.com <il...@google.com>
yb...@google.com <yb...@google.com>
at...@gmail.com <at...@gmail.com> #2
Yigit, do you have time to fix it?
reemission of the same liveData is racy
reemission of the same liveData is racy
ma...@gmail.com <ma...@gmail.com> #3
yea i'll take it.
at...@gmail.com <at...@gmail.com> #4
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.
il...@google.com <il...@google.com> #5
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()
}
}
@Test
fun raceTest() {
val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData)
emitSource(subLiveData) //crashes
}
subject.addObserver().apply {
testScope.advanceUntilIdle()
}
}
ma...@gmail.com <ma...@gmail.com> #6
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>
il...@google.com <il...@google.com> #7
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} )
}
I actually have a WIP fix for it:
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} )
}
ap...@google.com <ap...@google.com> #8
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
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
ma...@gmail.com <ma...@gmail.com> #9
Nicely done, works just as expected. Thank you!
at...@gmail.com <at...@gmail.com> #10
Is it possible to see when I can use this code officially?
il...@google.com <il...@google.com> #11
It'll be part of the upcoming 1.0.0-alpha06 release.
at...@gmail.com <at...@gmail.com> #12
Can you show me your CustomBottomSheetDialofFragment and the associated layout please. I get the error message "The view is not a child of CoordinatorLayout".
at...@gmail.com <at...@gmail.com> #13
This is what would work if you use a simple bottom sheet layout with a ConstraintLayout as root view as pointed out in comment #3 :
if (parent instanceof DrawerLayout) {
((DrawerLayout) parent).closeDrawer(navigationView);
} else {
BottomSheetBehavior bottomSheetBehavior =
BottomSheetBehavior.from((View) navigationView.getParent().getParent());
if (bottomSheetBehavior != null) {
bottomSheetBehavior.setState(BottomSheetBehavior.STATE_HIDDEN);
}
}
but only
BottomSheetBehavior bottomSheetBehavior =
BottomSheetBehavior.from(navigationView);
would expect, that the navigationView is a direct child of a CoordinatorLayout and would have the app:layout_behavior="com.google.android.material.bottomsheet.BottomSheetBehavior" attribute. That can't work or did I miss something?
if (parent instanceof DrawerLayout) {
((DrawerLayout) parent).closeDrawer(navigationView);
} else {
BottomSheetBehavior bottomSheetBehavior =
BottomSheetBehavior.from((View) navigationView.getParent().getParent());
if (bottomSheetBehavior != null) {
bottomSheetBehavior.setState(BottomSheetBehavior.STATE_HIDDEN);
}
}
but only
BottomSheetBehavior bottomSheetBehavior =
BottomSheetBehavior.from(navigationView);
would expect, that the navigationView is a direct child of a CoordinatorLayout and would have the app:layout_behavior="com.google.android.material.bottomsheet.BottomSheetBehavior" attribute. That can't work or did I miss something?
il...@google.com <il...@google.com> #14
Yeah, we should avoid BottomSheetBehavior.from()'s overzealous throwing of exceptions and handle cases like BottomSheetDialogFragment where the NavigationView is a few layers below the CoordinatorLayout.
ap...@google.com <ap...@google.com> #15
Project: platform/frameworks/support
Branch: androidx-master-dev
commit 4fb1acaf8ee002979bc161e362c1e83f89195205
Author: Ian Lake <ilake@google.com>
Date: Tue Sep 18 13:38:07 2018
Fix NavigationUI for use with BottomSheetDialogFragment
Replace using BottomSheetBehavior.from() which
throws IllegalArgumentExceptions to a safe
method that searches up the view hierarchy
for a CoordinatorLayout to determine if the
NavigationView is within a bottom sheet.
Test: example added to the integration-tests/testapp
BUG: 112158843
Change-Id: I774991ef1b23085e075e6bac6fa820ee3e001a31
M navigation/integration-tests/testapp/build.gradle
M navigation/integration-tests/testapp/src/main/java/androidx/navigation/testapp/HelpActivity.kt
M navigation/integration-tests/testapp/src/main/res/layout/activity_help.xml
A navigation/integration-tests/testapp/src/main/res/layout/bottom_bar_menu.xml
M navigation/ui/src/main/java/androidx/navigation/ui/NavigationUI.java
https://android-review.googlesource.com/760505
https://goto.google.com/android-sha1/4fb1acaf8ee002979bc161e362c1e83f89195205
Branch: androidx-master-dev
commit 4fb1acaf8ee002979bc161e362c1e83f89195205
Author: Ian Lake <ilake@google.com>
Date: Tue Sep 18 13:38:07 2018
Fix NavigationUI for use with BottomSheetDialogFragment
Replace using BottomSheetBehavior.from() which
throws IllegalArgumentExceptions to a safe
method that searches up the view hierarchy
for a CoordinatorLayout to determine if the
NavigationView is within a bottom sheet.
Test: example added to the integration-tests/testapp
BUG: 112158843
Change-Id: I774991ef1b23085e075e6bac6fa820ee3e001a31
M navigation/integration-tests/testapp/build.gradle
M navigation/integration-tests/testapp/src/main/java/androidx/navigation/testapp/HelpActivity.kt
M navigation/integration-tests/testapp/src/main/res/layout/activity_help.xml
A navigation/integration-tests/testapp/src/main/res/layout/bottom_bar_menu.xml
M navigation/ui/src/main/java/androidx/navigation/ui/NavigationUI.java
il...@google.com <il...@google.com> #16
Thanks for catching the oversight! The new code should work considerably better for other bottom sheet cases without the IllegalArgumentExceptions :)
Description
The Navigation Component has this very useful function to sync up menu items from a NavigationView with the NavController. It makes sure that the selected item is checked, can close the navigation drawer on selection if a DrawerLayout is used, or set the title of an ActionBar/Toolbar according to the current destination.
With the addition of the bottom app bar in Material Design we've also begun using the bottom navigation drawer (
public class BottomDrawerFragment extends BottomSheetDialogFragment {
@Override
public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
View view = inflater.inflate(R.layout.fragment_bottom_drawer, container, false);
NavigationView navigationView = view.findViewById(R.id.bottom_drawer_navigation);
NavigationUI.setupWithNavController(navigationView, Navigation.findNavController(getActivity(), R.id.nav_host_fragment));
return view;
}
}
This works well, but because NavigationUI currently doesn't dismiss the BottomSheetDialogFragment when a menu item has been selected like it closes the DrawerLayout, the bottom navigation drawer remains open and needs to be dismissed in another way (which comes with additional code overhead and issues).
Since AndroidX is now part of AOSP, I took a look at the implementation and I think that support for this could be added with two lines of code (see '+' characters at beginning of lines below) by doing for the DialogFragment what is already done for the DrawerLayout (excerpt from androidx-master-dev/navigation/ui/src/main/java/androidx/navigation/ui/NavigationUI.java).
public static void setupWithNavController(@NonNull final NavigationView navigationView,
@NonNull final NavController navController) {
navigationView.setNavigationItemSelectedListener(
new NavigationView.OnNavigationItemSelectedListener() {
@Override
public boolean onNavigationItemSelected(@NonNull MenuItem item) {
boolean handled = onNavDestinationSelected(item, navController, true);
if (handled) {
ViewParent parent = navigationView.getParent();
if (parent instanceof DrawerLayout) {
((DrawerLayout) parent).closeDrawer(navigationView);
+ } else if (parent instanceof BottomSheetDialogFragment) {
+ ((DialogFragment) parent).dismiss();
}
}
return handled;
}
});
navController.addOnNavigatedListener(new NavController.OnNavigatedListener() {
@Override
public void onNavigated(@NonNull NavController controller,
@NonNull NavDestination destination) {
Menu menu = navigationView.getMenu();
for (int h = 0, size = menu.size(); h < size; h++) {
MenuItem item = menu.getItem(h);
item.setChecked(matchDestination(destination, item.getItemId()));
}
}
});
}
The most active owner of the Navigation Component is Ian Lake and it he's been adding support for more and more UI components to the NavigationUI class in the past months, so I think he's the person to talk to about this. Assuming this doesn't introduce any issues, he could probably get it done in a couple of minutes.
Alternatively, I could implement this and the accompanying test myself, contribute it and try to get it reviewed.