SimpleDialog #1269

grzesiek2010 · 2017-07-21T06:57:06Z

No description provided.

srsudar

Overall looks great, this is exactly the PR I was hoping for!

A few small questions more than anything else, so I'm submitting as a comment rather than requesting changes.

srsudar · 2017-07-21T17:02:47Z

collect_app/src/main/java/org/odk/collect/android/preferences/ResetDialogPreference.java

-                alertDialog.show();
-            }
-        });
+        SimpleDialog simpleDialog = SimpleDialog.newInstance(getContext().getString(R.string.reset_app_state_result), android.R.drawable.ic_dialog_info, resultMessage, getContext().getString(R.string.ok));


Does Collect have any line length limits? This is longer than I keep in my own projects, but if there are no style guidelines this is fine.

Not yet. We were working on checkstyle but this one is still not ready.

I do think this is hard to read. I would write it as

String title = SimpleDialog.newInstance(getContext().getString(R.string.reset_app_state_result); int iconID = android.R.drawable.ic_dialog_info; String buttonTitle = getContext().getString(R.string.ok); SimpleDialog simpleDialog = SimpleDialog.newInstance(title, iconID, resultMessage, buttonTitle);

That more closely matches what you liked about using a Builder, @grzesiek2010.

srsudar · 2017-07-21T17:19:54Z

collect_app/src/main/java/org/odk/collect/android/preferences/ResetDialogPreference.java

-            }
-        });
+        SimpleDialog simpleDialog = SimpleDialog.newInstance(getContext().getString(R.string.reset_app_state_result), android.R.drawable.ic_dialog_info, resultMessage, getContext().getString(R.string.ok));
+        simpleDialog.show(((AdminPreferencesActivity) context).getSupportFragmentManager(), SimpleDialog.COLLECT_DIALOG_TAG);


Is there a reason you're using getContext() in the previous line, but the bare context field here? Saving references to a context like this class does makes me nervous, as in the past I've run into problems doing so. I'd prefer relying on the getContext() method in this line if it works.

Do you think it's safe to cast to android.support.v4.app.FragmentActivity rather than AdminPreferencesActivity? My intuition is to cast as high as possible in the hierarchy, but I don't have any good motivation for that. I'd be curious to hear what you think.

And what do you think of wrapping this cast in a try/catch like Android does in their dialog docs? If this fails it can just log an error maybe, rather than throw an exception. I don't know that it'd be worth crashing the app over if the wrong Context was used. I'm not sure about this one either, it just seems dangerous to trust a cast when contexts are involved.

srsudar · 2017-07-21T17:20:25Z

collect_app/src/main/java/org/odk/collect/android/fragments/dialogs/SimpleDialog.java

+                .setMessage(getArguments().getString(MESSAGE))
+                .setPositiveButton(getArguments().getString(BUTTON_TITLE), new DialogInterface.OnClickListener() {
+                    @Override
+                    public void onClick(DialogInterface dialog, int id) {


Does a no-op here in the onClick dismiss the dialog?

Yes dialog is dissmised if you click on the button.

srsudar · 2017-07-21T17:29:58Z

collect_app/src/main/java/org/odk/collect/android/fragments/dialogs/SimpleDialog.java

+        setCancelable(false);
+
+        return new AlertDialog.Builder(getActivity())
+                .setTitle(getArguments().getString(DIALOG_TITLE))


Is it worth doing any checks here to make sure the string isn't null, the icon IDs are >0, etc? I don't know that it should happen at all, or here or in the newInstance() method. We just should be aware that they can return default values that might behave. Maybe we're fine with it.

No if you pass a null value or 0 when it comes to icon that element is just invisible.

grzesiek2010 · 2017-07-24T09:59:41Z

@srsudar I added some small changes like try/catch block https://github.com/opendatakit/collect/pull/1269/files#diff-8be972fa817297fe2db3c718c69da61bR187 to be honest - I'm not sure we need that but we can just in case.

lognaturel

We're really close here! I would suggest making the one style change I described inline. Then we can first merge this and then @grzesiek2010 you can follow up with a second PR to move all of the other ~14 simple dialogs over to this structure.

lognaturel · 2017-07-24T22:08:35Z

collect_app/src/main/java/org/odk/collect/android/preferences/ResetDialogPreference.java

-                alertDialog.show();
-            }
-        });
+        SimpleDialog simpleDialog = SimpleDialog.newInstance(getContext().getString(R.string.reset_app_state_result), android.R.drawable.ic_dialog_info, resultMessage, getContext().getString(R.string.ok));


I do think this is hard to read. I would write it as

String title = SimpleDialog.newInstance(getContext().getString(R.string.reset_app_state_result); int iconID = android.R.drawable.ic_dialog_info; String buttonTitle = getContext().getString(R.string.ok); SimpleDialog simpleDialog = SimpleDialog.newInstance(title, iconID, resultMessage, buttonTitle);

That more closely matches what you liked about using a Builder, @grzesiek2010.

grzesiek2010 · 2017-07-25T06:53:27Z

@lognaturel fixed.

srsudar · 2017-07-25T18:50:14Z

This looks good to me. I agree that I'm not sure if the try/catch around the cast is necessary. I don't know that it will hurt, though, aside from adding a bit of complexity. If people are happy with it I'd vote to keep it.

grzesiek2010 · 2017-07-25T18:52:35Z

I'm not sure but I believe most of dialogs are placed in activities so we don't need try/catch there in other cases we can use it just in case.

lognaturel · 2017-07-25T23:12:34Z

Thanks again, @grzesiek2010, @srsudar.

mmarciniak90 · 2017-07-27T10:42:36Z

Tested with success.
Tested on Android 4.1, 4.2, 4.4, 5.1, 6.0
'Reset results' dialog does not disappear if device orientation is changed.
User is able to scroll on the dialog.
Locking the screen does not affect the dialog.
Dialog does not disappear even if 'Don't keep activities' option is turned on.

mmarciniak90 · 2017-10-04T06:20:54Z

@opendatakit-bot unlabel "needs testing"

srsudar reviewed Jul 21, 2017

View reviewed changes

SimpleDialog

b065931

grzesiek2010 force-pushed the simpleDialog branch from 2209dd1 to b065931 Compare July 24, 2017 09:57

lognaturel reviewed Jul 24, 2017

View reviewed changes

lognaturel added the needs testing label Jul 24, 2017

This was referenced Jul 24, 2017

Wip Collect 541 2 #1267

Closed

Dialogs - general approach #1226

Closed

Code improvements

447fe2f

lognaturel merged commit 318b887 into getodk:master Jul 25, 2017

mmarciniak90 mentioned this pull request Jul 27, 2017

User is moved to main screen if user minimizes app on 'Reset result' dialog and returns to ODK in specific way #1280

Closed

grzesiek2010 mentioned this pull request Aug 17, 2017

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

Merged

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

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

Dialog abstraction for simple information-only dialogs (getodk#1269)

5b8508e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SimpleDialog #1269

SimpleDialog #1269

grzesiek2010 commented Jul 21, 2017

srsudar left a comment

srsudar Jul 21, 2017

grzesiek2010 Jul 21, 2017

lognaturel Jul 24, 2017

srsudar Jul 21, 2017

srsudar Jul 21, 2017

grzesiek2010 Jul 21, 2017

srsudar Jul 21, 2017

grzesiek2010 Jul 21, 2017

grzesiek2010 commented Jul 24, 2017

lognaturel left a comment

lognaturel Jul 24, 2017

grzesiek2010 commented Jul 25, 2017

srsudar commented Jul 25, 2017

grzesiek2010 commented Jul 25, 2017

lognaturel commented Jul 25, 2017

mmarciniak90 commented Jul 27, 2017 •

edited

Loading

mmarciniak90 commented Oct 4, 2017

SimpleDialog #1269

SimpleDialog #1269

Conversation

grzesiek2010 commented Jul 21, 2017

srsudar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzesiek2010 commented Jul 24, 2017

lognaturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzesiek2010 commented Jul 25, 2017

srsudar commented Jul 25, 2017

grzesiek2010 commented Jul 25, 2017

lognaturel commented Jul 25, 2017

mmarciniak90 commented Jul 27, 2017 • edited Loading

mmarciniak90 commented Oct 4, 2017

mmarciniak90 commented Jul 27, 2017 •

edited

Loading