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
Hi,
This looks really bad but w/o a repro app, we really cannot do anything about it. Is it possible for you to provide a sample app that reproduces your problem ?
This looks really bad but w/o a repro app, we really cannot do anything about it. Is it possible for you to provide a sample app that reproduces your problem ?
ma...@gmail.com <ma...@gmail.com> #3
Attached is a simple testing app. It inserts 10000 items into the db when activity starts and then displays them using paging. The db is also updated every 1 second. On the screen, if you drag the fast scroll bar up and down quickly a few times, it will eventually crash with this IndexOutOfBoundsException error.
at...@gmail.com <at...@gmail.com> #4
Thanks for the great repro case, tracked it down, investigating a fix.
Problematic behavior:
1) AsyncPagedListDiffer receives new list, takes snapshot for diffing (ui thread)
2) At roughly the same time:
2a) Diff snapshot of old snapshot vs new snapshot (bg thread)
2b) New data arrives in new list, way earlier in the list, so the list is essentially: new page, lots of empty pages, initial load
3) AsyncPagedListDiffer updates lastLoad position with diffutil position mapping
In 3, we try to map the lastLoad position from old snapshot to new snapshot, but fail to distinguish between new list vs new snapshot. Because of all the empty pages, that's a huge discrepancy. This is why the huge scrollbar + lots of swaps triggers this well - there are lots of swaps to new PagedLists while loads are happening very far away from initial load positions.
Will need to account for pages loaded in between time of snapshot, and time of swap.
Problematic behavior:
1) AsyncPagedListDiffer receives new list, takes snapshot for diffing (ui thread)
2) At roughly the same time:
2a) Diff snapshot of old snapshot vs new snapshot (bg thread)
2b) New data arrives in new list, way earlier in the list, so the list is essentially: new page, lots of empty pages, initial load
3) AsyncPagedListDiffer updates lastLoad position with diffutil position mapping
In 3, we try to map the lastLoad position from old snapshot to new snapshot, but fail to distinguish between new list vs new snapshot. Because of all the empty pages, that's a huge discrepancy. This is why the huge scrollbar + lots of swaps triggers this well - there are lots of swaps to new PagedLists while loads are happening very far away from initial load positions.
Will need to account for pages loaded in between time of snapshot, and time of swap.
il...@google.com <il...@google.com> #5
ma...@gmail.com <ma...@gmail.com> #6
Awesome! Can not wait for the fix to drop.
il...@google.com <il...@google.com>
il...@google.com <il...@google.com> #7
I saw a new WorkManager version was dropped today but no new paging. Any ideas when this fix will be released? Days or weeks? Thanks.
ap...@google.com <ap...@google.com> #8
no promises but it is already scheduled so should be days hopefully.
ma...@gmail.com <ma...@gmail.com> #9
Released with Paging 2.0.0
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.