Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ANR when showing bottom sheets #680

Merged
merged 2 commits into from
Aug 26, 2021

Conversation

jossiwolf
Copy link
Collaborator

Our internalShow() method could end up in an infinite loop sometimes (yay, recursion!). We now use the official show API which should work. Unfortunately, our show calls all get cancelled but Swipeable still settles in the closest state. This seems to always be the expanded/half-expanded state. We look at the sheetState's targetValue in our try's finally block to figure out if the state will settle in an expanded state and call the onSheetShown callback based on that.

I'm still investigating why the show calls are getting cancelled in the first place and am working on a test for the ANR as well but would like to merge the fix to not keep #660 waiting for too long.

Fixes #660

@google-cla google-cla bot added the cla: yes label Aug 25, 2021
Our `internalShow` method could end up in an infinite loop sometimes (yay, recursion!). We now use the official `show` API which *should* work. Unfortunately, our `show` calls all get cancelled but Swipeable still settles in the closest state. This seems to always be the expanded/half-expanded state. We look at the state's targetValue in our try's `finally` block to figure out if the state will settle in an expanded state and call the `onSheetShown` callback based on that.

Fixes google#660
Copy link
Collaborator

@ianhanniballake ianhanniballake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you already filed an issue against Jetpack Compose for show() getting cancelled? It would be nice to include a link to that issue next to this code.

@chrisbanes chrisbanes merged commit dd384f0 into google:main Aug 26, 2021
@jossiwolf
Copy link
Collaborator Author

@ianhanniballake Nope will do that as soon as I get my AndroidX/repo to work again and can file a reliable repro!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigating to bottom sheet and touching the sheet as it opens results in ANR
3 participants