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 a crash which happens when user shakes the phone #ANDROID-15102 #43

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

juangardi21
Copy link
Contributor

@juangardi21 juangardi21 commented Aug 27, 2024

🥅 What's the goal?

Fix a crash which happens when user shakes the phone, tweaks activity is shown, goes back and then shakes again.

🚧 How do we do it?

  • Ensure navigation happens when the state is in the proper lifecycle: Instead of calling navigate directly from the shake detector, check the NavController state to make sure it's ready to navigate.
  • Use a lifecycle-aware state: Try handling navigation in response to a state within the Composable flow, rather than inside LaunchedEffect
  • Create new function to not depends on NavController

📘 Documentation changes?

  • README updated

🧪 How can I test this?

  • Before:
4_5985619573649446339.mp4
  • After:
4_5985619573649446337.mp4

@@ -94,14 +96,18 @@ open class Tweaks : TweaksContract {
@Composable
fun NavController.navigateToTweaksOnShake() {
Copy link
Member

Choose a reason for hiding this comment

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

This feature depends on a NavController, although in some cases we might not have a NavController.

In B2P we have this launched effect that does not depend on it, and also allows a custom action to launch the tweaks:

https://github.com/Telefonica/b2p-germany-android/blob/4a9f90436777833358e891f4864d75ad9308309e/app/src/debug/java/com/telefonica/germany/b2p/core/tweaks/ShakeDetectorWrapper.kt#L18

I think this approach would be very helpful to open tweaks from the command line into a different activity (same as SmartWifi does)

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've created another function which does not depends on navcontroller

Copy link
Member

Choose a reason for hiding this comment

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

Cool! Thanks!

@juangardi21 juangardi21 marked this pull request as ready for review September 11, 2024 12:22
@Composable
fun NavController.navigateToTweaksOnShake() {
DetectShakeAndNavigate {
navigate(TWEAKS_NAVIGATION_ENTRYPOINT)
Copy link
Member

Choose a reason for hiding this comment

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

You can set this as default value for onOpenTweaks and leave only one method

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 don't think I can, because navigate is callable only within the NavController scope

Copy link
Member

Choose a reason for hiding this comment

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

Ok I see, one method depends on the NavController and the other one doesn't

@juangardi21 juangardi21 changed the title Crash - replace launchedEffect by DisposableEffect #ANDROID-15102 Fix a crash which happens when user shakes the phone #ANDROID-15102 Sep 11, 2024
@juangardi21 juangardi21 requested review from a team, dpastor and DevPabloGarcia and removed request for a team September 11, 2024 14:35
Copy link

@DevPabloGarcia DevPabloGarcia left a comment

Choose a reason for hiding this comment

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

Nice catch!

@juangardi21 juangardi21 merged commit 4f89d4a into main Sep 12, 2024
3 checks passed
@juangardi21 juangardi21 deleted the bug/ANDROID-15102-crash-when-shaking-twice branch September 12, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants