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.

Sign up for GitHub

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

Fixes Crash On Double Click Of Space FABs #7102

Merged
merged 3 commits into from
Sep 14, 2022
Merged

Fixes Crash On Double Click Of Space FABs #7102

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8177d97
Fixes crash on double click of space fabs
ericdecanini Sep 12, 2022
db83099
Adds changelog file
ericdecanini Sep 12, 2022
34303c4
Removes inject constructor in NewChatBottomSheet
ericdecanini Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/7102.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixes crash when quickly double clicking FABs in the new app layout
9 changes: 4 additions & 5 deletions vector/src/main/java/im/vector/app/features/home/NewHomeDetailFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,13 +201,12 @@ class NewHomeDetailFragment :
private fun setupFabs() {
showFABs()

views.newLayoutCreateChatButton.setOnClickListener {
newChatBottomSheet.show(requireActivity().supportFragmentManager, NewChatBottomSheet.TAG)
views.newLayoutCreateChatButton.debouncedClicks {
newChatBottomSheet.takeIf { !it.isAdded }?.show(requireActivity().supportFragmentManager, NewChatBottomSheet.TAG)
}

views.newLayoutOpenSpacesButton.setOnClickListener {
// Click action for open spaces modal goes here
spaceListBottomSheet.show(requireActivity().supportFragmentManager, SpaceListBottomSheet.TAG)
views.newLayoutOpenSpacesButton.debouncedClicks {
spaceListBottomSheet.takeIf { !it.isAdded }?.show(requireActivity().supportFragmentManager, SpaceListBottomSheet.TAG)
Copy link
Member

Choose a reason for hiding this comment

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

I think the root cause of the issue is to instantiate the BottomSheets in advance at this point. I would have fixed this issue differently. Using debouncedClicks is OK, but using takeIf { !it.isAdded } looks a bit like a workaround.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Also there is no need to inject the constructor here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the approach you're leading towards is to instantiate the bottom sheet as we show it? (SpaceListBottomSheet.show())

I considered this approach but doing so would just reduce the bug from a crash to having two bottom sheets show together. debouncedClicks may help with this, but I wanted to add the !it.isAdded check as a guaranteed method.

Research also shows me that making this check (or using bottomSheet.isShowing in other cases) is a common practice.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks for cleaning the constructor.

Copy link
Member

Choose a reason for hiding this comment

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

One more point:
When spaceListBottomSheet is displayed, if the user rotates it's device, the Fragment SpaceListBottomSheet will be restored by the System (the BottomSheet will still be visible), but NewHomeDetailFragment will not have a correct reference to this Fragment, since it will just instantiate a new instance in the constructor.

So this code:
https://github.com/vector-im/element-android/blob/develop/vector/src/main/java/im/vector/app/features/home/NewHomeDetailFragment.kt#L197

will not work (I did not check though).

You should try to get the reference (by tag for instance) to the restored bottom sheet Fragment instead of creating a new one.

}
}

Expand Down