fix(#376): fix issue of the app crashing in some phones #377
Conversation
…le to find caller Activity
There was a problem hiding this comment.
Just a couple of questions
How did you reproduce this error in phones? Following Binod's steps?
src/main/java/org/medicmobile/webapp/mobile/OpenSettingsDialogFragment.java
Outdated
Show resolved
Hide resolved
| @@ -59,6 +61,8 @@ public void setup() { | |||
| doNothing().when(openSettingsDialogFragment).startActivity(argsStartActivity.capture()); | |||
|
|
|||
| openSettingsDialogFragment.onCreate(null); | |||
| openSettingsDialogFragment.onActivityCreated(null); | |||
| shadowOf(Looper.getMainLooper()).idle(); | |||
There was a problem hiding this comment.
Is this test already covering the error when the fragment is unavailable, and do the retry?
There was a problem hiding this comment.
Not at the moment. I saw that the tests in this file were related to the behavior of the Fragment and decided not to add the test as it's an internal functionality. I could add it if it's needed.
There was a problem hiding this comment.
Is it possible to add a unit test to catch this bug in particular? It can be a good addition to prevent regressions and also as an example for similar future implementations
There was a problem hiding this comment.
The setRetainInstance is deprecated in androidx, this unit test can help the developer to consider that case when migrating to a more recent androidx lib.
There was a problem hiding this comment.
Sure, let me add a test for that.
|
|
||
| import java.time.Clock; | ||
| import java.time.Instant; | ||
| import java.time.ZoneOffset; | ||
| import java.util.stream.IntStream; | ||
|
|
||
| @RunWith(RobolectricTestRunner.class) | ||
| @RunWith(Enclosed.class) | ||
| @SuppressWarnings("PMD.TestClassWithoutTestCases") |
There was a problem hiding this comment.
@latin-panda I've created a nested class to enclose the testing of the setupViewWithRetry method because the mocks of the previous test cases were not reusable for it. Also, I've added this warning suppress statement because PMD was raising an error as to the outer class not having any test cases.
There was a problem hiding this comment.
Thank you for the fix @sugat009 ⭐
I was able to replicate the error using a Samsung Galaxy A01 phone with Android 10 on master branch.
androidCrash_master.mov
And it was fixed on 376-google-reporting-app-crashes-on-some-phones branch
fix.mov
| @@ -33,8 +37,44 @@ public boolean onTouch(View view, MotionEvent event) { | |||
| @Override | |||
| public void onCreate(@Nullable Bundle savedInstanceState) { | |||
| super.onCreate(savedInstanceState); | |||
| setRetainInstance(true); | |||
There was a problem hiding this comment.
So, I am probably missing something here, but in my testing, adding this line here in the onCreate was all that I needed to avoid the error in question.
In my debugging it seemed like without this line, the OpenSettingsDialogFragment.onCreate method was getting called a bunch of times when entering/exiting split-screen mode before the EmbeddedBrowserActivity.onCreate method is even called. When this happened, the wbvMain view was still null (presumably because the activity was not created yet). Eventually, the EmbeddedBrowserActivity.onCreate method is called for the new activity and that triggers the OpenSettingsDialogFragment.onCreate method to be called again (and this time the wbvMain view exists.
However, if I set setRetainInstance(true); here in OpenSettingsDialogFragment.onCreate and leave everything else as it is in master, it prevents all the extra initial calls to EmbeddedBrowserActivity.onCreate that were happening before EmbeddedBrowserActivity.onCreate and so avoids the error. When the EmbeddedBrowserActivity.onCreate function is called, that triggers a call to OpenSettingsDialogFragment.onCreate which then can access the wbvMain view at that point in the lifecycle.
I am not 100% sure about exactly what is happening here. The EmbeddedBrowserActivity.onCreate is not getting called multiple times. My theory is that there is some weird connection between the activity lifecycle and the fragment lifecycle that is causing the OpenSettingsDialogFragment.onCreate function to get called at other points during the activity lifecycle as the original EmbeddedBrowserActivity is destroyed. Setting setRetainInstance seems to decouple the fragment from the activity lifecycle so that this does not happen.
There was a problem hiding this comment.
That's a strange one. I tried this one as well. My Android studio was crashing with the debug option. \o/. So, I did the classic debugging using logging. I did not get the EmbeddedBrowserActivity.onCreate being called multiple times or EmbeddedBrowserActivity.onCreate not being called and only OpenSettingsDialogFragment.onCreate being called. I've included screenshots of this below.
Nevertheless, just adding the setRetainInstance(true); to line 38 on the master branch for OpenSettingsDialogFragment class did work. (Tested on virtual and physical phones).
Setting the setRetainInstance(true); does detach the fragment from its activity which is perfect for this use case. Docs.
I'll refactor the changes to include just the setRetainInstance(true); line in the OpenSettingsDialogFragment. I'll do this in a different branch and send another PR if this change is ever required in the future.
|
Hey @sugat009 any update on this? |
I've just started to continue looking into this. |
fixes: #376
The issue seems to be a case when during some events the
OpenSettingsDialogFragmentis not able to find its calling activity which in this case isEmbeddedBrowserActivityand since it can't findEmbeddedBrowserActivityit's also not able to find the viewwbvMainwhich in turn is unable to set thesetOnTouchListeneronto the view because the view isnull. So, I've added a fallback and retry mechanism to give time for theEmbeddedBrowserActivityto load and be "findable" and also retain the instance ofOpenSettingsDialogFragmentbecause it's just a background listener that listens for taps and swipes.