Fixed Dialog Button for Experimental Magenta Theme #3851
Conversation
|
Hi @seadowg! There are many other dialogs in the code, where we are having this problem, as we are not using the |
|
@SaumiaSinghal I think it's enough to fix one for the moment! Maybe you could comment on the issue (#3849) about the other ones you've found? That way you can move away from dialogues :) |
| @@ -683,7 +684,7 @@ public void onClick(DialogInterface dialog, int i) { | |||
| } | |||
|
|
|||
| private void createProgressDialog() { | |||
| progressDialog = new ProgressDialog(this); | |||
| progressDialog = new AlertDialog.Builder(this).create(); | |||
There was a problem hiding this comment.
Could this not use a ProgressDialogFragment instead?
There was a problem hiding this comment.
Oh I didn't use ProgressDialogFrgament because I thought that the lifecycle changes are handled here by the ViewModel, so, I just used an AlertDialog instead. But, as I have read in most of the articles like here, that using a dialog instead of fragment, and handling lifecycle changes by the use of some variable, is a bad practice, so I'll just switch to ProgressDialogFragment here.
| viewModel.setProgressDialogShowing(false); | ||
| } catch (IllegalArgumentException e) { | ||
| Timber.i("Attempting to close a dialog that was not previously opened"); | ||
| Fragment dialogFragment = getSupportFragmentManager().findFragmentByTag(ProgressDialogFragment.COLLECT_PROGRESS_DIALOG_TAG); |
There was a problem hiding this comment.
This is the right thing to do, but we have a helper that does it for you! You can use DialogUtils.dismissDialog should help you out here.
| } | ||
| createProgressDialog(); | ||
| refreshFormListDialogFragment = new RefreshFormListDialogFragment(); | ||
| refreshFormListDialogFragment.show(getSupportFragmentManager(), ProgressDialogFragment.COLLECT_PROGRESS_DIALOG_TAG); |
There was a problem hiding this comment.
Like with dismissing you can use DialogUtils.showIfNotShowing. Sorry I should have pointed those out before leading you down the ProgressDialogFragment path!
There was a problem hiding this comment.
That's okay! Actually I have used DialogUtils.showIfNotShowing before 😞
|
@SaumiaSinghal are you able to rerun the Circle CI tests? It looks like the process crashed not any tests failed. Also can you confirm you've run |
|
Hi @seadowg! I checked locally. Any of the newly added unit tests does not fail, but |
|
@SaumiaSinghal master should be fixed now. Could you rebase this on top of master and check again. Post any failures you're seeing! |
|
Tested with success Verified on Android: 6.0, 7.0, 8.1, 9.0 Verified cases:
Screenshots from 3 modes to compare
|
SaumiaSinghal
force-pushed
the
fixed_dialog_button_for_magenta_theme
branch
from
aeb8df0 to
24c295b
Compare
|
Verified on Androids 5.1, 7.1 and 10. @getodk-bot unlabel "needs testing" |
Work towards #3849
What has been done to verify that this works as intended?
I tested it.
Why is this the best possible solution? Were any other approaches considered?
I replaced Progress dialog with Alert dialog, and inflated the view with
progress_dialog.xmlfile.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
Do we need any specific form for testing your changes? If so, please attach one.
No
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:
./gradlew checkAlland confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTestlocally.