Used Dialog Fragment to create Remove Group Dialog #3839

SaumiaSinghal · 2020-05-17T14:34:26Z

Work towards #3612

What has been done to verify that this works as intended?

This PR contains the fix for Remove Response Dialog. I tested it.

Why is this the best possible solution? Were any other approaches considered?

I shifted the creating of Alert Dialog to Dialog Fragment and wrote tests for the same.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

No regression risks.

Do we need any specific form for testing your changes? If so, please attach one.

nested-repeats.xml would work (nested-repeats.xml.txt)

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No

Before submitting this PR, please make sure you have:

run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

lognaturel

Small naming change! I think the bigger change to how the activity and fragment communicate can come later.

lognaturel · 2020-06-04T20:58:54Z

..._app/src/main/java/org/odk/collect/android/formentry/repeats/DeleteRepeatDialogFragment.java

+    }
+
+    public interface DeleteRepeatDialogCallback {
+        void deleteGroup();


This should be deleteRepeat because it does not apply to general groups.

By the way, the delete_repeat string resource and other related ones are super confusing because they use the word "Group". I think we should change that but it's a separate thing.

..._app/src/main/java/org/odk/collect/android/formentry/repeats/DeleteRepeatDialogFragment.java

collect_app/src/main/java/org/odk/collect/android/activities/FormEntryActivity.java

lognaturel

ViewModel structure causes activity leak.

lognaturel · 2020-06-16T20:30:18Z

PMD issue and then this is ready for QA!

SaumiaSinghal · 2020-06-16T20:34:08Z

Hi @lognaturel! Sorry for the pmd issue, just fixed that.

mmarciniak90 · 2020-06-19T08:49:08Z

@SaumiaSinghal I'm a bit confused. Am I right that this fix concerns only Remove Group dialog not to 'Remove Response'? What about Remove response dialog? Will you fix it as described?

Also, Collect crashes when I click on the Remove Group or Cancel button.

mmarciniak90 · 2020-06-19T08:49:35Z

@getodk-bot unlabel "needs testing"

SaumiaSinghal · 2020-06-21T21:18:59Z

Hi @mmarciniak90! Sorry for the confusing title. This PR concerns the Remove Group dialog. But I didn't get any crash on my device when I pressed Remove Group or Cancel button. I tested it on Android 9, and on the emulator as well. Can you attach the form used?

I will open a PR for Remove Response dialog separately.

mmarciniak90 · 2020-06-22T07:05:05Z

Sure @SaumiaSinghal, here is the form: nested-repeats.xml.txt

And it's a stacktrace from Android 9.0

SaumiaSinghal · 2020-06-22T16:43:51Z

Hi @mmarciniak90! Can you test again please? Thanks

mmarciniak90 · 2020-06-22T20:52:07Z

@getodk-bot label "needs testing"

mmarciniak90 · 2020-06-23T07:56:06Z

@SaumiaSinghal I can confirm that crash does not occur but right now when I click on 'Remove Group' accepting button, nothing happens. The group which I try to remove is still there.

@getodk-bot unlabel "needs testing"

SaumiaSinghal · 2020-06-23T17:43:34Z

Hi @mmarciniak90! I tested it again and it works on my device.:/ I'm attaching the video of the screen recorder here.

mmarciniak90 · 2020-06-23T18:14:40Z

@SaumiaSinghal here is my case:

SaumiaSinghal · 2020-06-23T20:01:34Z

.../src/test/java/org/odk/collect/android/formentry/repeats/DeleteRepeatDialogFragmentTest.java

+    }
+
+    public static class TestActivity extends FragmentActivity implements DeleteRepeatDialogFragment.DeleteRepeatDialogCallback {
+


@lognaturel I changed the tests a little bit so as to avoid using protected scope for callback. I like this approach better than directly approaching the interface in the fragment.

SaumiaSinghal · 2020-06-23T20:02:36Z

Sorry @mmarciniak90! I missed something. Thanks for attaching the gif. I hope it works fine now.

kkrawczyk123 · 2020-07-08T09:13:17Z

@SaumiaSinghal I have noticed that when following scenario with don't keep activities turned on:

Display Remove group dialog
Minimize Collect
Go back to Collect

crash is visible, stacktrace:
2020-07-08 11:10:13.753 12515-12515/org.odk.collect.android E/AndroidRuntime: FATAL EXCEPTION: main Process: org.odk.collect.android, PID: 12515 java.lang.RuntimeException: Unable to start activity ComponentInfo{org.odk.collect.android/org.odk.collect.android.activities.FormHierarchyActivity}: java.lang.RuntimeException: Cannot create an instance of class org.odk.collect.android.formentry.FormEntryViewModel at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3270) at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3409) at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:83) at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135) at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2016) at android.os.Handler.dispatchMessage(Handler.java:107) at android.os.Looper.loop(Looper.java:214) at android.app.ActivityThread.main(ActivityThread.java:7356) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930) Caused by: java.lang.RuntimeException: Cannot create an instance of class org.odk.collect.android.formentry.FormEntryViewModel at androidx.lifecycle.ViewModelProvider$NewInstanceFactory.create(ViewModelProvider.java:221) at androidx.lifecycle.ViewModelProvider$AndroidViewModelFactory.create(ViewModelProvider.java:278) at androidx.lifecycle.SavedStateViewModelFactory.create(SavedStateViewModelFactory.java:106) at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.java:185) at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.java:150) at org.odk.collect.android.formentry.repeats.DeleteRepeatDialogFragment.onAttach(DeleteRepeatDialogFragment.java:29) at androidx.fragment.app.Fragment.performAttach(Fragment.java:2672) at androidx.fragment.app.FragmentStateManager.attach(FragmentStateManager.java:263) at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1170) at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1356) at androidx.fragment.app.FragmentManager.moveFragmentToExpectedState(FragmentManager.java:1434) at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1497) at androidx.fragment.app.FragmentManager.dispatchStateChange(FragmentManager.java:2625) at androidx.fragment.app.FragmentManager.dispatchCreate(FragmentManager.java:2571) at androidx.fragment.app.FragmentController.dispatchCreate(FragmentController.java:236) at androidx.fragment.app.FragmentActivity.onCreate(FragmentActivity.java:315) at androidx.appcompat.app.AppCompatActivity.onCreate(AppCompatActivity.java:106) at org.odk.collect.android.activities.CollectAbstractActivity.onCreate(CollectAbstractActivity.java:45) at org.odk.collect.android.activities.FormHierarchyActivity.onCreate(FormHierarchyActivity.java:126) at android.app.Activity.performCreate(Activity.java:7825) at android.app.Activity.performCreate(Activity.java:7814) at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1306) at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3245) at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3409) at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:83) at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135) at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2016) at android.os.Handler.dispatchMessage(Handler.java:107) at android.os.Looper.loop(Looper.java:214) at android.app.ActivityThread.main(ActivityThread.java:7356) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930) Caused by: java.lang.InstantiationException: java.lang.Class<org.odk.collect.android.formentry.FormEntryViewModel> has no zero argument constructor at java.lang.Class.newInstance(Native Method) at androidx.lifecycle.ViewModelProvider$NewInstanceFactory.create(ViewModelProvider.java:219) at androidx.lifecycle.ViewModelProvider$AndroidViewModelFactory.create(ViewModelProvider.java:278) at androidx.lifecycle.SavedStateViewModelFactory.create(SavedStateViewModelFactory.java:106) at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.java:185) at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.java:150) at org.odk.collect.android.formentry.repeats.DeleteRepeatDialogFragment.onAttach(DeleteRepeatDialogFragment.java:29) at androidx.fragment.app.Fragment.performAttach(Fragment.java:2672) at androidx.fragment.app.FragmentStateManager.attach(FragmentStateManager.java:263) at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1170) at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1356) at androidx.fragment.app.FragmentManager.moveFragmentToExpectedState(FragmentManager.java:1434) at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1497) at androidx.fragment.app.FragmentManager.dispatchStateChange(FragmentManager.java:2625) at androidx.fragment.app.FragmentManager.dispatchCreate(FragmentManager.java:2571) at androidx.fragment.app.FragmentController.dispatchCreate(FragmentController.java:236) at androidx.fragment.app.FragmentActivity.onCreate(FragmentActivity.java:315) at androidx.appcompat.app.AppCompatActivity.onCreate(AppCompatActivity.java:106) at org.odk.collect.android.activities.CollectAbstractActivity.onCreate(CollectAbstractActivity.java:45) at org.odk.collect.android.activities.FormHierarchyActivity.onCreate(FormHierarchyActivity.java:126) at android.app.Activity.performCreate(Activity.java:7825) at android.app.Activity.performCreate(Activity.java:7814) at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1306) at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3245) at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3409) at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:83) at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135) at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2016) at android.os.Handler.dispatchMessage(Handler.java:107) at android.os.Looper.loop(Looper.java:214) at android.app.ActivityThread.main(ActivityThread.java:7356) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)

The crash is not visible for the same scenario and remove response dialog. It is also not visible on master, Could you please take a look?

SaumiaSinghal · 2020-07-08T17:29:16Z

Hi @kkrawczyk123! Thanks for reporting the crash. It is fixed now. I tested it.

mmarciniak90 · 2020-07-09T09:58:23Z

@SaumiaSinghal I have the latest changes and right now removing the group seems to be broken. Could you please take a look?

mmarciniak90 · 2020-07-09T10:17:39Z

@getodk-bot unlabel "needs testing"

lognaturel · 2020-07-09T16:49:33Z

@SaumiaSinghal are you making sure to run the connected tests for repeat removal after you make a change? Are there tests that catch these regressions? If not, please add coverage as you make the fixes to reduce the risk of future regressions.

SaumiaSinghal · 2020-07-09T19:07:51Z

I'm sorry @mmarciniak90 for multiple testings requests. I really hope everything is fine now.

mmarciniak90 · 2020-07-14T12:44:23Z

@SaumiaSinghal I noticed one more crash.

Repro steps:

Don't keep activities is turned on
User starts form (nested-repeats.xml)
User adds Friend, puts name
User opens 'Remove group' dialog and minimizes
User returns to Collect and clicks Cancel on dialog
User goes to next view and clicks on Add button to add pet
App crashes

@getodk-bot unlabel "needs testing"

SaumiaSinghal · 2020-07-14T16:01:09Z

@mmarciniak90 I fixed the crash, seems like it was because I was using @Inject Analytics in the fragment, and on coming back to the Activity, it was giving NPE. Sorry for the multiple crashes.

mmarciniak90 · 2020-07-15T08:29:16Z

@getodk-bot label "needs testing"

mmarciniak90 · 2020-07-21T10:00:36Z

Tested with success

Verified on Android: 7.0, 8.1, 9.0

Verified cases:

rotate Remove This Group dialog on question view
rotate Remove This Group dialog on hierarchy view
select Cancel and Remove group
minimization with Don't keep activities from both views
light/ dark mode
verified previously described cases which caused crashes
verified previously described bugs

kkrawczyk123 · 2020-07-21T10:04:26Z

Verified on Androids 5.1, 7.1 and 10.

@getodk-bot unlabel "needs testing"
@getodk-bot label "behavior verified"

lognaturel requested changes Jun 4, 2020

View reviewed changes

SaumiaSinghal force-pushed the fix_remove_response_dialog branch from bff2d4e to f88d23a Compare June 8, 2020 15:53

SaumiaSinghal requested a review from lognaturel June 8, 2020 16:21

lognaturel reviewed Jun 16, 2020

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/activities/FormEntryActivity.java Outdated Show resolved Hide resolved

lognaturel requested changes Jun 16, 2020

View reviewed changes

SaumiaSinghal force-pushed the fix_remove_response_dialog branch 2 times, most recently from 85fce01 to b9910e4 Compare June 16, 2020 17:52

SaumiaSinghal requested a review from lognaturel June 16, 2020 17:52

lognaturel approved these changes Jun 16, 2020

View reviewed changes

lognaturel added the needs testing label Jun 16, 2020

getodk-bot removed the needs testing label Jun 19, 2020

SaumiaSinghal changed the title ~~Used Dialog Fragment to create Remove Response Dialog~~ Used Dialog Fragment to create Remove Group Dialog Jun 21, 2020

SaumiaSinghal force-pushed the fix_remove_response_dialog branch from 622c28f to 14005c4 Compare June 21, 2020 22:31

getodk-bot added the needs testing label Jun 22, 2020

getodk-bot removed the needs testing label Jun 23, 2020

SaumiaSinghal commented Jun 23, 2020

View reviewed changes

lognaturel added the needs testing label Jun 30, 2020

SaumiaSinghal added 6 commits July 8, 2020 21:42

make dialogIsNotCancellable() test green

158bbc1

unit tests refactor

af3abe4

fix pmd

c59ce33

fix crash

f870c9c

add unit test to check the activity implemts dialog callback interface

248efd8

fix crash on minimizing app

02f974f

SaumiaSinghal force-pushed the fix_remove_response_dialog branch from 9b9c631 to 02f974f Compare July 8, 2020 17:27

getodk-bot removed the needs testing label Jul 9, 2020

fix deleteGroup() in FormEntryActivity

84f57bc

lognaturel added the needs testing label Jul 10, 2020

mmarciniak90 mentioned this pull request Jul 14, 2020

Answer in repeat group is removed when user opens repeat group dialog but he does not decide to remove #3970

Closed

getodk-bot removed the needs testing label Jul 14, 2020

SaumiaSinghal added 2 commits July 14, 2020 20:58

remove FormEntryViewModel in DeleteRepeatDialogFramgent

9f52237

fix unit tests

06e4ac8

remove onCancelled()

6485392

getodk-bot added the needs testing label Jul 15, 2020

getodk-bot added behavior verified and removed needs testing labels Jul 21, 2020

grzesiek2010 merged commit 8e65915 into getodk:master Jul 21, 2020

SaumiaSinghal deleted the fix_remove_response_dialog branch July 21, 2020 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Used Dialog Fragment to create Remove Group Dialog #3839

Used Dialog Fragment to create Remove Group Dialog #3839

SaumiaSinghal commented May 17, 2020 •

edited

Loading

lognaturel left a comment

lognaturel Jun 4, 2020

lognaturel Jun 4, 2020

lognaturel left a comment

lognaturel commented Jun 16, 2020

SaumiaSinghal commented Jun 16, 2020

mmarciniak90 commented Jun 19, 2020

mmarciniak90 commented Jun 19, 2020

SaumiaSinghal commented Jun 21, 2020 •

edited

Loading

mmarciniak90 commented Jun 22, 2020

SaumiaSinghal commented Jun 22, 2020

mmarciniak90 commented Jun 22, 2020

mmarciniak90 commented Jun 23, 2020

SaumiaSinghal commented Jun 23, 2020

mmarciniak90 commented Jun 23, 2020

SaumiaSinghal Jun 23, 2020

SaumiaSinghal commented Jun 23, 2020

kkrawczyk123 commented Jul 8, 2020

SaumiaSinghal commented Jul 8, 2020

mmarciniak90 commented Jul 9, 2020 •

edited

Loading

mmarciniak90 commented Jul 9, 2020

lognaturel commented Jul 9, 2020

SaumiaSinghal commented Jul 9, 2020

mmarciniak90 commented Jul 14, 2020

SaumiaSinghal commented Jul 14, 2020

mmarciniak90 commented Jul 15, 2020

mmarciniak90 commented Jul 21, 2020

kkrawczyk123 commented Jul 21, 2020

		}

		public static class TestActivity extends FragmentActivity implements DeleteRepeatDialogFragment.DeleteRepeatDialogCallback {

Used Dialog Fragment to create Remove Group Dialog #3839

Used Dialog Fragment to create Remove Group Dialog #3839

Conversation

SaumiaSinghal commented May 17, 2020 • edited Loading

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

lognaturel left a comment

Choose a reason for hiding this comment

lognaturel Jun 4, 2020

Choose a reason for hiding this comment

lognaturel Jun 4, 2020

Choose a reason for hiding this comment

lognaturel left a comment

Choose a reason for hiding this comment

lognaturel commented Jun 16, 2020

SaumiaSinghal commented Jun 16, 2020

mmarciniak90 commented Jun 19, 2020

mmarciniak90 commented Jun 19, 2020

SaumiaSinghal commented Jun 21, 2020 • edited Loading

mmarciniak90 commented Jun 22, 2020

SaumiaSinghal commented Jun 22, 2020

mmarciniak90 commented Jun 22, 2020

mmarciniak90 commented Jun 23, 2020

SaumiaSinghal commented Jun 23, 2020

mmarciniak90 commented Jun 23, 2020

SaumiaSinghal Jun 23, 2020

Choose a reason for hiding this comment

SaumiaSinghal commented Jun 23, 2020

kkrawczyk123 commented Jul 8, 2020

SaumiaSinghal commented Jul 8, 2020

mmarciniak90 commented Jul 9, 2020 • edited Loading

mmarciniak90 commented Jul 9, 2020

lognaturel commented Jul 9, 2020

SaumiaSinghal commented Jul 9, 2020

mmarciniak90 commented Jul 14, 2020

SaumiaSinghal commented Jul 14, 2020

mmarciniak90 commented Jul 15, 2020

mmarciniak90 commented Jul 21, 2020

kkrawczyk123 commented Jul 21, 2020

SaumiaSinghal commented May 17, 2020 •

edited

Loading

SaumiaSinghal commented Jun 21, 2020 •

edited

Loading

mmarciniak90 commented Jul 9, 2020 •

edited

Loading