fix: Added a progress dialog while deleting the forms. #1348

dr0pdb · 2017-08-16T14:13:21Z

Although the recommended way of obtaining this result is to create the ProgressDialog in the AsyncTask's onPreExecute method but I wasn't able to get the right Context for the ProgressDialog. Hence I created the ProgressDialog in the DataManagerList class itself.

FIX #1258

lognaturel · 2017-08-16T17:23:32Z

Thanks, @srv-twry! I've tagged it as needs testing so we can get a couple of people to try it out and verify the behavior. In the future, please make sure to follow the provided template for pull requests! 😊

It looks like checkstyle is currently failing. You can see the issues by clicking on Details next to the circleci check notice or by running ./gradlew checkstyle locally.

Fixed checkstyle error

grzesiek2010 · 2017-08-17T07:15:44Z

Don't know if this pr makes a sense since it doesn't use DialogFragment. @srv-twry you may don't know but we have started work on replacing all our dialogs using DialogFragment:
#541
#1269
#1275

grzesiek2010 · 2017-08-17T06:56:29Z

collect_app/src/main/java/org/odk/collect/android/fragments/DataManagerList.java

        deleteInstancesTask = null;
        getListView().clearChoices(); // doesn't unset the checkboxes
        for (int i = 0; i < getListView().getCount(); ++i) {
            getListView().setItemChecked(i, false);
        }
        deleteButton.setEnabled(false);
+
+        //Dismiss the progress dialog.


This comment isn't necessary as it's obvious that progressDialog.dismiss() dismiss the progress dialog :)

grzesiek2010 · 2017-08-17T06:59:26Z

collect_app/src/main/java/org/odk/collect/android/fragments/DataManagerList.java

+            * For solving #1258.
+            * Create the progress dialog before starting the background task.
+            * Could have been done in the DeleteInstancesTask's onPreExecute() but was unable to get the context correctly.
+            * */


I would get rid of this comment too as it's pretty obvious what we do here. We should add comments only if something is hard to understand, tricky or fixes some strange issue. If you want to keep the code clear a better solution is to move this part of code to a separate method.

grzesiek2010 · 2017-08-17T07:01:42Z

collect_app/src/main/java/org/odk/collect/android/fragments/DataManagerList.java

+            * Could have been done in the DeleteInstancesTask's onPreExecute() but was unable to get the context correctly.
+            * */
+            progressDialog = new ProgressDialog(getContext());
+            progressDialog.setMessage("Deleting selected forms...");


it shouldn't be hardcoded, you should place it in strings.xml file

YuraLaguta · 2017-08-17T08:07:30Z

@srv-twry maybe for consistency and better user experience it should be similar to the screen shot attached showing progress with amount of files that has been already deleted, because if it takes 1.5 minute people will leave application or even worse go back. If you post progress inside DeleteInstancesTask, it shouldn't be to much effort but will be great UX.

dr0pdb · 2017-08-17T15:22:15Z

@Yurii-Laguta I had initially planned to implement it the same way that you are suggesting , but i was unable to get the Context in the AsyncTask for the ProgressDialog hence had to implement it inside the Fragment itself which means i don't have access to the variables which can be used to display the progress since they are inside the AsyncTask only.
Any suggestion on how to get the Context inside the AsyncTask would be very helpful. Then i would do it again for better User experience.

lognaturel · 2017-08-17T17:59:27Z

I think it's appropriate to match what InstanceUploaderActivity does using a ProgressDialog for now. This solves the immediate problem in a consistent way and all ProgressDialogs can be replaced together at some later date. I do agree that it would be better if the count could be updated so perhaps @srv-twry and @Yurii-Laguta you can do some brainstorming in #collect-code and others with ideas can also jump in.

The progress indicator is choppy on my device, it doesn't spin smoothly. Is there processing happening on the UI thread that shouldn't be by any chance?

The Progress dialog now correctly shows the index of the form currently being deleted. Fix #1258

dr0pdb · 2017-08-17T20:19:37Z

@lognaturel @grzesiek2010 @Yurii-Laguta Kindly review this PR. I added an interface method which would update the dialog from time to time using the PublishProgress method inside the AsyncTask. The ProgressDialog now shows the progress of the task as well. Finally ! 😄

lognaturel · 2017-08-17T20:56:13Z

collect_app/src/main/res/values/strings.xml

@@ -507,4 +507,7 @@
    <string name="not_supported_offline_layer_format">Selected offline layer file uses the PBF format which is not supported!</string>
    <string name="sort_by">Sort by</string>
    <string name="server">Server</string>
+    <string name="form_delete_message">Deleting selected forms</string>
+    <string name="deleting_form_dialog_first">Deleting form:\u0020</string>


The dialog message should be a single string and should take in the current and total number of forms as parameters. This is important because not all languages are left-to-right and translators must have the option of repositioning the two form counts relative to each other.

lognaturel · 2017-08-17T20:57:53Z

Nicely done, that looks like the right approach. I made a quick comment about the string.

Any ideas about the choppiness of the dialog? I noticed that it has to do with the number of forms there are to delete. If there are 1000 forms, the status indicator is really slow but as the number to delete goes down the status spinner speeds up. That seems fishy to me. It doesn't need to be handled in this PR but would be good to investigate.

Fix #1258

dr0pdb · 2017-08-18T07:49:32Z

@lognaturel I have changed the String resource as suggested.
I don't think i have any idea about the choppiness of the dialog. Maybe someone else can come up with the possible explaination. 😄

YuraLaguta · 2017-08-18T11:28:41Z

Looks great, one minor formatting related comment. I'm surprised it's not picked up by checkstyle.

shobhitagarwal1612 · 2017-08-18T22:00:37Z

I got the following stacktrace in debugger, although no crashes:

08-19 03:24:08.080 5281-5281/org.odk.collect.android E/WindowManager: android.view.WindowLeaked: Activity org.odk.collect.android.activities.InstanceUploaderActivity has leaked window DecorView@44c68a0[Upload Results] that was originally added here
                                                                          at android.view.ViewRootImpl.<init>(ViewRootImpl.java:418)
                                                                          at android.view.WindowManagerGlobal.addView(WindowManagerGlobal.java:331)
                                                                          at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:93)
                                                                          at android.app.Dialog.show(Dialog.java:322)
                                                                          at org.odk.collect.android.activities.InstanceUploaderActivity.createAlertDialog(InstanceUploaderActivity.java:402)
                                                                          at org.odk.collect.android.activities.InstanceUploaderActivity.uploadingComplete(InstanceUploaderActivity.java:278)
                                                                          at org.odk.collect.android.tasks.InstanceUploader.onPostExecute(InstanceUploader.java:51)
                                                                          at org.odk.collect.android.tasks.InstanceUploader.onPostExecute(InstanceUploader.java:40)
                                                                          at android.os.AsyncTask.finish(AsyncTask.java:667)
                                                                          at android.os.AsyncTask.-wrap1(AsyncTask.java)
                                                                          at android.os.AsyncTask$InternalHandler.handleMessage(AsyncTask.java:684)
                                                                          at android.os.Handler.dispatchMessage(Handler.java:102)
                                                                          at android.os.Looper.loop(Looper.java:154)
                                                                          at android.app.ActivityThread.main(ActivityThread.java:6119)
                                                                          at java.lang.reflect.Method.invoke(Native Method)
                                                                          at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
                                                                          at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)

Steps to reproduce -

Select multiple instances and click the send selected button
Minimize the app
Keep a track on debugger to know when all instances have been uploaded
After all of the instances have been uploaded, reopen the app

Possible solution: Dismiss the progress dialog when the activity gets paused and redisplay it on resuming the activity.

dr0pdb · 2017-08-19T08:21:11Z

@shobhitagarwal1612 I think the stacktrace is related to Uploading the saved forms i.e. the InstanceUploaderActivity , while my PR is about deleting the saved forms.
Although if you want me to fix this issue in this PR itself ,i can do that.

lognaturel · 2017-08-21T03:36:53Z

Interesting one, @shobhitagarwal1612! @srv-twry is absolutely right that this should be its own issue -- could you please file it?

shobhitagarwal1612 · 2017-08-21T04:33:57Z

@srv-twry This should be fixed separately. I must have been confused at the time of reporting this. Thanks for noticing it.
@lognaturel Sure

mmarciniak90 · 2017-09-06T10:42:16Z

Issue verified on Android: 4.1, 4.4, 5.1, 6.0, 7.0.
User sees dialog :

User is informed that some action is in progress, which is great.

I have only doubts about the appearance of this dialog.
I expected to see a 'dialog name' like eg. Deleting Form.
This would be consistent with other dialogs such as:

Additionally, when user send forms he sees gray background under the dialog.
@lognaturel Should we take care of those appearance details?

mmarciniak90 · 2017-10-03T15:04:46Z

@opendatakit-bot unlabel "needs testing"

fix: Added a progress dialog while deleting the forms.

82e9db1

FIX #1258

lognaturel added the needs testing label Aug 16, 2017

lognaturel requested a review from grzesiek2010 August 16, 2017 17:20

lognaturel added the reviews wanted label Aug 16, 2017

fix: #1258

70133ff

Fixed checkstyle error

grzesiek2010 reviewed Aug 17, 2017

View reviewed changes

This was referenced Aug 17, 2017

Crash when scanning for many instances #1350

Closed

Crash when deleting many instances #1351

Closed

dr0pdb added 2 commits August 18, 2017 01:18

fix: Added status to the progress dialog

7beb124

The Progress dialog now correctly shows the index of the form currently being deleted. Fix #1258

fix: Fixed checkstyle error

cc79289

lognaturel reviewed Aug 17, 2017

View reviewed changes

fix: Using string resources with placeholders

72187af

Fix #1258

shobhitagarwal1612 mentioned this pull request Aug 21, 2017

Window leaked exception while uploading instances #1361

Closed

Merge branch 'master' into fix#1258

554e82e

lognaturel approved these changes Aug 22, 2017

View reviewed changes

lognaturel merged commit 751e859 into getodk:master Aug 22, 2017

dr0pdb deleted the fix#1258 branch August 22, 2017 19:24

getodk-bot removed the needs testing label Oct 3, 2017

snavarreteimmap pushed a commit to snavarreteimmap/collect that referenced this pull request Oct 2, 2020

fix: Added a progress dialog while deleting the forms. (getodk#1348)

a2ba335

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Added a progress dialog while deleting the forms. #1348

fix: Added a progress dialog while deleting the forms. #1348

dr0pdb commented Aug 16, 2017

lognaturel commented Aug 16, 2017

grzesiek2010 commented Aug 17, 2017

grzesiek2010 Aug 17, 2017

grzesiek2010 Aug 17, 2017

grzesiek2010 Aug 17, 2017

YuraLaguta commented Aug 17, 2017

dr0pdb commented Aug 17, 2017 •

edited

Loading

lognaturel commented Aug 17, 2017

dr0pdb commented Aug 17, 2017

lognaturel Aug 17, 2017

lognaturel commented Aug 17, 2017

dr0pdb commented Aug 18, 2017

YuraLaguta commented Aug 18, 2017

shobhitagarwal1612 commented Aug 18, 2017 •

edited

Loading

dr0pdb commented Aug 19, 2017 •

edited

Loading

lognaturel commented Aug 21, 2017

shobhitagarwal1612 commented Aug 21, 2017

mmarciniak90 commented Sep 6, 2017

mmarciniak90 commented Oct 3, 2017

fix: Added a progress dialog while deleting the forms. #1348

fix: Added a progress dialog while deleting the forms. #1348

Conversation

dr0pdb commented Aug 16, 2017

lognaturel commented Aug 16, 2017

grzesiek2010 commented Aug 17, 2017

grzesiek2010 Aug 17, 2017

Choose a reason for hiding this comment

grzesiek2010 Aug 17, 2017

Choose a reason for hiding this comment

grzesiek2010 Aug 17, 2017

Choose a reason for hiding this comment

YuraLaguta commented Aug 17, 2017

dr0pdb commented Aug 17, 2017 • edited Loading

lognaturel commented Aug 17, 2017

dr0pdb commented Aug 17, 2017

lognaturel Aug 17, 2017

Choose a reason for hiding this comment

lognaturel commented Aug 17, 2017

dr0pdb commented Aug 18, 2017

YuraLaguta commented Aug 18, 2017

shobhitagarwal1612 commented Aug 18, 2017 • edited Loading

dr0pdb commented Aug 19, 2017 • edited Loading

lognaturel commented Aug 21, 2017

shobhitagarwal1612 commented Aug 21, 2017

mmarciniak90 commented Sep 6, 2017

mmarciniak90 commented Oct 3, 2017

dr0pdb commented Aug 17, 2017 •

edited

Loading

shobhitagarwal1612 commented Aug 18, 2017 •

edited

Loading

dr0pdb commented Aug 19, 2017 •

edited

Loading