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 EmojiPicker crash when getting LayoutParams #741

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

thomashorta
Copy link
Contributor

This crash was happening when flinging upwards in the Stories Compose screen, which should open the Emoji picker, because it was trying to get a Coordinator LayoutParams from a RelativeLayout (as it was using the wrong view).

Testing steps:

  1. Install the demo app
  2. Select / record a video
  3. Swipe up from the bottom of the screen
  4. Verify app doesn't crash and emojis are shown

This crash was happening when flinging upwards in the Stories Compose screen,
which should open the Emoji picker, because it was trying to get a Coordinator
LayoutParams from a RelativeLayout (as it was using the wrong view).
@thomashorta thomashorta added the [Type] Crash The app stops unexpectedly. label Sep 12, 2023
@thomashorta thomashorta self-assigned this Sep 12, 2023
@ParaskP7 ParaskP7 self-assigned this Sep 13, 2023
Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @thomashorta !

I have reviewed and tested this PR as per the instructions, everything works as expected, nice fix! 🌟


EXTRAS

  1. Warning (⚠️): This is not related this this fix, but rather this emoji swiping feature overall. Initially, I couldn't replicate the crash, just to then verify that the fix actually works. Me not being able to replicate the crash, meant that I wasn't even able to replicate the emoji feature. I then realized that this might be because of me using the Gesture navigation system, that is, instead of using the 3-button navigation system. Thus, after me changing to the 3-button navigation system, I was able to replicate the crash, and then its fix. This made me think:
    1. FWIW, I think we should deal with this issue at some point. I am sure we have lots of user using the Gesture navigation system nowadays. Wdyt? 🤔
    2. Also, this discussion I had with @mzorz on emoji support now makes more sense to me as it seems I couldn't test this feature back then... 🤷

@thomashorta
Copy link
Contributor Author

thomashorta commented Sep 13, 2023

FWIW, I think we should deal with this issue at some point. I am sure we have lots of users using the Gesture navigation system nowadays. Wdyt? 🤔

@ParaskP7 I was able to reproduce it even with Gesture Navigation on, it's just trickier because your finger has to be close enough to the bottom to satisfy our onFling conditions but not enough that it triggers the system gestures. 😅

Honestly, I don't think this emoji feature was supposed to be available to the user as we don't really instruct them about it. It seems to me this was a left-over code from some point in the past and I simply wanted to make sure it at least doesn't crash, especially because it is actually active in Jetpack stories feature.

I checked Sentry for the past 90 days and only found 5 events of that crash in Jetpack, which I believe were users probably trying to do the System Gesture and getting the Emoji picker instead.

I'm merging the fix for now, but I wonder if this should just be turned off since it doesn't seem to be something we actually intended to be visible to users (based on the fact that there's no visible button or instruction regarding the gesture).

@thomashorta thomashorta merged commit e1d5088 into trunk Sep 13, 2023
2 checks passed
@thomashorta thomashorta deleted the fix/crash-emoji-picker-params branch September 13, 2023 12:12
@ParaskP7
Copy link
Contributor

👋 @thomashorta and I agree with everything you said above, thanks for the reply! 💯

@ParaskP7 I was able to reproduce it even with Gesture Navigation on, it's just trickier because your finger has to be close enough to the bottom to satisfy our onFling conditions but not enough that it triggers the system gestures. 😅

For the life of me, I just couldn't reproduce it, my fingers seems to be too fat... 🤣

...I wonder if this should just be turned off since it doesn't seem to be something we actually intended to be visible to users...

Personally, I am voting in favor of that, so yes! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Crash The app stops unexpectedly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants