Dialogs - general approach

[grzesiek2010]

@lognaturel this is my general proposal for managing dialogs. It contains one example - I replaced one dialog from admin settings to show how it works.

@shobhitagarwal1612 you are interested in this as well so please look at it and let me know what you think.

[shobhitagarwal1612]

@grzesiek2010 I think we can move ahead with this approach 👍

[srsudar]

How are different actions accounted for with this approach? For example if you wanted to redo the admin password reset dialog, would you extend CollectDialogFragment and add an action, or would all actions be added to the base Action enum in CollectDialogFragment?

[grzesiek2010]

@srsudar this approach assumes that if you want to add new dialog you have to add new action here: https://github.com/opendatakit/collect/pull/1226/files#diff-172d9a1f2eb7a4e8c8032f12eea2fb07R29 and new case here https://github.com/opendatakit/collect/pull/1226/files#diff-172d9a1f2eb7a4e8c8032f12eea2fb07R80

[lognaturel]

I would prefer that the actions be defined where the dialogs are created, perhaps with something like this. That said, if you're both happy with the switch/case approach, it works for me! I'm much happier with where buildResetSettingsFinalDialog now is.

[srsudar]

Interesting. That seems little odd to me, but I do think it would be better than it currently exists.

The things that I find odd about it:

1. It feels weird to me to be putting such different functionality in one place. Code to change the admin password will have to live in the same class as code to reset settings, which isn't as strong a separation of concerns as I would like. I could see it leading to situations where making a change to the reset settings code could end up breaking the reset admin password code, eg. In the worse case maybe someone forgets a break; and the switch falls through and resets the admin password to an empty string or something. The risk could be mitigated with tests, but it puts a lot of responsibility in a single class either way.
2. It isn't obvious to me how to give the containing Activity/Fragment hooks to interact with the dialog. It works well enough with the reset settings example, since it just asks the activity to recreate() itself. If there needed to be a more incremental change, like just changing a display name, or kicking off another action, it might have to do something like ((MyActivity) getActivity()).someAction(), and that seems a little odd to me as well. I'd expect instead something like a callback somewhere.

I am understanding the design, @grzesiek2010 ? What do you think?

[lognaturel]

Very well described, @srsudar, thanks so much for the really precise description of concerns.

Are you happy with introducing a new Activity base class? That's the other thing I feel uncomfortable with.

[srsudar]

I'm not opposed to Activity base classes, and I've used them in the past. In general I prefer avoiding them, however. From what I remember there used to be various classes like ListActivity, FragmentActivity, SupportFragmentActivity, and extending from a single common class locked you in. AppCompatActivity might combine all this functionality, so maybe it isn't as much of a concern anymore. When I stopped using Fragments I got to stop worrying about the vagaries of getSupportFragmentManager(), etc. I would believe that I am under-informed on this.

The code in CollectAbstractActivity looks straight forward enough to me that it doesn't need to exist in a superclass. If this becomes more complex, we might be thankful to have it in a single base Activity. For now though I'd rather see a dialog refresh not require changing the class hierarchy of all activities. If it turns out that not having a base class ends up creating a ton of duplicate null checking code in each activity, it might be worth revisiting or adding a util class or something to DRY it out.

[lognaturel]

Thanks so much @srsudar for taking some time out of your refactoring to help with ours! 😊 It's always nice to have a couple of opinions.

@grzesiek2010, @srsudar just finished his PhD at University of Washington 👏 and has been involved in ODK in various ways. I find he has exceptional code taste so that's why I like to pull him into conversations like this one. How about trying out a callback system for actions and not introducing CollectAbstractActivity and we call all see how that feels?

[srsudar]

Thanks for the kind words, @lognaturel ! Makes me think I should go fix all the problems in that repo I linked.

Thanks for letting me chime in. I'm also happy to butt out and shut up since I'm writing my thesis and I won't be the one implementing it. =)

[lognaturel]

Hah, I certainly appreciate any advice I can get. Like you said, there are a lot of dialogs so any thoughts on refining the approach are valuable. Sending you good thesis vibes!

[grzesiek2010]

@lognaturel @srsudar

@grzesiek2010, @srsudar just finished his PhD at University of Washington 👏 and has been involved in ODK in various ways. I find he has exceptional code taste so that's why I like to pull him into conversations like this one.

sure nice to met you @srsudar and please accept my congratulations!

I've just improved my previous solution so please review again. I kept the base class so it might be the only thing you don't like but generally it looks better now I hope.

[srsudar]

Can callbacks go into Bundle/Serializable? If it doesn't throw an error, I'd expect any variables outside of the callback closure, which the callback would be referring to, might not be preserved if the Fragment was recreated. Or if they were, I don't think they would be hooked up to objects in the new parent Activity.

Take this, for example:

    private void buildResetSettingsFinalDialog(String message) {
        // final object to use it in the callback closure
        final MyObject myObject = createMyObjSomehow(this);
        CollectDialogBundle.Builder dialogBuilder = new CollectDialogBundle.Builder();
        dialogBuilder
                .setIcon(android.R.drawable.ic_dialog_info)
                .setDialogTitle(getContext().getString(R.string.reset_app_state_result))
                .setDialogMessage(message)
                .setPositiveButtonText(getContext().getString(R.string.ok))
                .setOnPositiveButtonClickCallback(new CollectDialogBundle.SingleButtonCallback() {
                    @Override
                    public void onClick(Dialog dialog) {
                        // This would be a problem on rotation
                        myObject.doSomethingFancy();
                        dialog.dismiss();
                        ((AdminPreferencesActivity) context).recreate();
                    }
                })
                .setCancelable(false);

        ((CollectAbstractActivity) context).buildDialog(dialogBuilder.build());
    }

I think that the call to myObject.doSomethingFancy() would be a problem if the CollectDialogFragment had been recreated due to rotation. The myObject variable might be hanging around, and would be referring to an old Activity, which we'd have leaked.

Am I wrong about this? I haven't run it and I'd believe I'm missing something.

How does the material-dialogs project handle callbacks? Do they use DialogFragments?

[srsudar]

It looks like the Android documentation recommends making use of onAttach(Activity activity) to re-attach listeners inside DialogFragments. In at least one example Signal takes the same approach.

That approach could lead to a lot of subclasses, although they could still make use of the consolidated Bundle code for consistency. The Signal example uses a static inner class for the dialog, which I don't love.

Top level subclasses would be my preferred approach, I think, as it would lend itself to tests. To be fair, though, I haven't used Dialogs or Fragments recently and I could be trying to steer people down a tortuous path.

If people didn't want to rely on subclasses, CollectDialogFragment might just expose a way to interact with the callbacks without going through the Bundle. The parent Activity would have to keep track of that, however. By relying on onAttach() all the functionality could live in the Fragment subclasses themselves.

In the model I'm imagining for a subclass approach, each Activity containing a dialog that needs to communicate with the Activity would have a specific callback interface, like resetSettings(). This would look something like:

public class ChangePasswordDialogFragment extends CollectDialogFragment {
  public interface ChangePasswordCallbacks {
    void onDialogPositiveClick(String pwd);
  }

  ChangePasswordCallbacks callbacks = null;

  // snip
  
  @Override
  public void onAttach(Activity activity) {
    super.onAttach(activity);
    // Android docs wrap this in a try catch
    callbacks = (ChangePasswordCallbacks) activity;
  }

  @Override
  public void onCreateDialog(Bundle savedInstanceState) {
    // snip
    builder.setPositiveButton(resourceId, new DialogInterface.OnClickListener() {
      @Override
       public void onClick(DialogInterface dialog, int id) {
         if (callbacks == null) { return; }
         callbacks.onDialogPositiveClick(getPassword());
       }
     });
     // snip
}

public class AdminPreferencesActivity implements ChangePasswordCallbacks {
 PasswordManager passwordMgr; // see note below

 // snip
 void onDialogPositiveClick(String pwd) {
   // The Activity itself isn't responsible for saving the password in my ideal world.
   // This way no code lives in the Activity other than creating objects and
   // hooking up callbacks. This makes it easier to write tests without needing to
   // spin up a UI, which would have to happen if the Activity itself saves the code.
   passwordMgr.setPassword(pwd);
 }
}

Does that look reasonable or bizarre? I've only looked at a tiny subset of the dialogs that exist in the app. Maybe this is inappropriate/overkill for most of them. This could also be a bigger change than is being aimed at with this PR.

Edit: added call to super.onAttach.

[srsudar]

Looking at this again, it looks like the example I've been using of resetting the admin password might not fit with this more general Dialog refresh. The preferences use PreferenceActivity, which isn't from the support library and consequently doesn't have getSupportFragmentManager(). It wouldn't be able to extend from the Activity base class without a refactor to make preferences rely solely on PreferenceFragmentor PreferenceFragmentCompat. This is also why I'm not extending AppCompatDialogFragment in the example below.

Nevertheless, here is a an example of what my subclass-heavy approach would look like. I opted to not provide callbacks to the hosting activity and instead do everything inside the DialogFragment. If callbacks were needed, it would follow the onAttach() approach I describe above.

There are several things that I like about doing it this way.

1. All the non-ui logic lives in PreferenceAccessor, which makes tests straight forward. If the logic for saving the admin password moves out of a dialog to an edit text or something, the logic for saving the password will be unchanged. Getting logic out of UI classes like Activity/Fragments is the best thing that can happen to an Android codebase, I think.
2. The code for setting up the change password dialog is now cut out of AdminPreferencesFragment. All that Fragment is responsible for now is creating an instance and showing it. This makes me slightly nervous as Fragments used to have a bunch of problems with the SupportFragmentManager if you had nested fragments, although maybe those were problems of my own making. I'm running Android N and I would believe that Fragments having child Fragments could cause trouble on lower API devices, but I could also believe they've fixed that by now.
3. It is relatively straight forward to add Robolectric tests for code like this. An Espresso test would no doubt be higher fidelity, and maybe people would prefer that. These are easy for me to read, however.

Note that to get the Robolectric tests to run I had to use storix's $MODULE_DIR$ fix described here.

[grzesiek2010]

@srsudar

I think that the call to myObject.doSomethingFancy() would be a problem if the CollectDialogFragment had been recreated due to rotation. The myObject variable might be hanging around, and would be referring to an old Activity, which we'd have leaked.

You are right and it may be a problem, that's why the previous solution was better (worked in a proper way in that case).

My aim was to prepare a solution to avoid creating separate class for each dialog (currently we have over 40 dialogs and it will continue to rise as we actively work on the app adding new features etc.). That's why I don't like the solution with onAttach. One activity may contain a few dialogs then we would need to create a few classes and implement a few interfaces.

I really like my current proposal but I agree with you that it may be faulty when an activity is recreated.

I was looking at implementation of https://github.com/afollestad/material-dialogs and it looks similar to mine but they just dissmis a dialog when an activity is recreated so they don't have this problem :) We can of course do the same but I really want to keep our dialogs.

Maybe a perfect solution doesn't exist...

[srsudar]

If the dialog is just displaying a message, I agree that a separate class might be overkill. If it has to do something, like changing a password, I don't think a separate class is a big deal. It could still employ a centralized builder approach to consolidate code and present a consistent style.

I agree that it could end up creating a lot of callback interfaces, but that is only an issue for when the container Activity really needs to be notified, which won't be all of them. By creating the dialogs in-line there is an implicit callback. Extracting it to a real interface formalizes it. In some ways its more verbose, but it is also a very clean seam, which I like. It also makes it easier to test, which of course we all love. =)

In the cases where material-dialogs needs to communicate with the container Activity/Fragment, it looks like they use a similar callback system. In a couple of closed issues (1, 2) it sounds like this is because they use onAttach().

All this being said, I don't have nearly as good a sense of the Collect codebase as the others here. I don't think it's a good idea to put callbacks in the Bundle as I think it will create leaks and strange bugs, but as for style I'm happy to defer to more informed minds.

[grzesiek2010]

@srsudar ok thank you very much for this discussion. I'll rethink everything again.

@lognaturel @shobhitagarwal1612 and the others your opinion is welcome as well.

[grzesiek2010]

Ok the third approach is ready. What do you think @srsudar @lognaturel

[srsudar]

Glancing through it (not building, running, rotating, etc), I think this structure looks great. There are a few things lurking in sections of the code that aren't the subject of this PR that I'm not sure about (saving a reference to the context in the ResetDialogPreference makes me nervous--in this instance it might be totally fine, but in general I avoid saving references to Contexts). But as a general way to approach Dialogs, I think this is great.

I could see a case being made for not actually using callbacks in this reset example, since the dialog could dismiss itself and then do getActivity().recreate() rather than delegating both tasks to the parent Activity. If something later changed, a callback might need to be added, however, so leaving it as you currently have it also makes sense.

Does the way you're getting AdminPreferencesActivity circumvent the FragmentManager/SupportFragmentManager problem I described above?

[grzesiek2010]

Does the way you're getting AdminPreferencesActivity circumvent the FragmentManager/SupportFragmentManager problem I described above?

Do you mean this?:

I think that the call to myObject.doSomethingFancy() would be a problem if the CollectDialogFragment had been recreated due to rotation. The myObject variable might be hanging around, and would be referring to an old Activity, which we'd have leaked.

You are right that dialog is not a good example so I added another one.

[srsudar]

When I made my example of how I like dialogs to look I ran into trouble with:

The preferences use PreferenceActivity, which isn't from the support library and consequently doesn't have getSupportFragmentManager(). It wouldn't be able to extend from the Activity base class without a refactor to make preferences rely solely on PreferenceFragmentor PreferenceFragmentCompat. This is also why I'm not extending AppCompatDialogFragment in the example below.

If your code builds and runs, then you got around that problem, which is great.

Do you still want opinions on #1259 ?

If a dialog just shows text and doesn't do anything, I don't think it needs its own class.

I don't know what will happen when some of the standard dialogs are pulled out into DialogFragments. Consider this dialog, for example. It would be a real pain to pull out into its own class. It is declared inline so that it can access the deleteInstances() method on the parent Fragment. That is a perfect example of how dialogs can lead to spaghetti code, I think--the dialog ties together with an AsyncTask, all communicating via the Fragment. That smells like trouble to me, but clearly it's working. Walking that class hierarchy, I don't see setRetainInstance(true) being called anywhere on the fragment, so I was surprised that the dialog stays shown when I rotate the device. Looking into it, this is because android:configChanges="orientation|screenSize is set in AndroidManifest.xml, so this particular Activity does not get recreated on orientation. All of this makes it hard for me to reason about, which makes me think there is a better way to handle this bit of code.

Will that behavior stay the same when moved to DialogFragments? I think so, but I'd want to verify it.

I don't think fixing all these problems is necessary for this PR. If you want to keep a straight forward, limited number of separate classes, approach to this PR like you did in #1259 in order to update the look of dialogs, I think that is reasonable. Longer term I would like to do things like pull out the dialog classes to less tightly coupled, more modular and more testable units.

Is that at all helpful? I worry I've really derailed this discussion. Dialogs are tricky. You have looked in depth at all the dialogs being used in Collect--I have not. I would love if we get to a place like my example, where all the dialog machinery is mostly self-contained and tested. That should maybe happen in individual PRs, however, not on this PR.

If the purpose of this PR is to solve the dialog problems, I think it will get too big.

If the purpose of this PR is to make dialogs use material design, using only your general approach, defining callbacks in-line as they are currently in the codebase, seems good to me. The only thing to be careful of here is that the new dialogs handles rotation. This will depend on what each dialog is doing and how it is currently set up.

[lognaturel]

Thank you @grzesiek2010 and @srsudar for your patience and willingness to experiment to get a solution that meets long-term needs.

Forgive me if this should be obvious but why is CollectDialogBundle needed in this implementation?

I have to admit that I'm not loving any of these general-purpose solutions yet and I'm wondering whether we may have better luck looking at individual categories of dialogs one at a time rather than trying to solve every problem at once ("boiling the ocean!").

For example, I really only have one big complaint with the look and feel of dialogs and I documented it in #1265. That kind of dialog with save/ignore/cancel shows up in 5 different places and I think perhaps handling those first might be a nice, focused way to start with immediate benefit. It shows up every single time a user exits a form through the back button. The other glitches like disappearing on rotation and inconsistent coloring are annoying but are not dramatically affecting usability, I think. Please correct me if I'm wrong but I believe that neither this proposal nor the one in #1259 will make it easier to create that kind of dialog.

I understand that @grzesiek2010's goal with this is more to clean up the code and I do think that's important but ideally it could be approached incrementally with other user benefits such as increased testing or better look and feel as @srsudar was getting at in his last message.

@srsudar's admin password example has a lot of properties I like:

1. The code in AdminPreferencesFragment is clean and clear. In this case, the dialog fragment name is sufficient to know what it does.
2. No change to the base class needed.
3. Nice illustration of a more whitebox style of test

@grzesiek2010, I think you want to minimize the number of classes but I think that a larger number of classes with more reduced scope and responsibility might actually help most with readability and maintainability (and yes, testability).

[grzesiek2010]

@srsudar @lognaturel

Do you still want opinions on #1259 ?

No thank you. That approach was not good as there was one general class and now I know that it's not possible to do that in a proper way using only one class for all dialogs.

The preferences use PreferenceActivity, which isn't from the support library and consequently doesn't have getSupportFragmentManager(). It wouldn't be able to extend from the Activity base class without a refactor to make preferences rely solely on PreferenceFragmentor PreferenceFragmentCompat. This is also why I'm not extending AppCompatDialogFragment in the example below.

I didn't investigate it carefully but I only made a quick investigation and I replaced this dialog using:

CollectDialogBundle.Builder dialogBuilder = new CollectDialogBundle.Builder();

           dialogBuilder
                    .setIcon(android.R.drawable.ic_dialog_info)
                    .setDialogTitle(getActivity().getString(R.string.reset_app_state_result))
                    .setDialogMessage("XXXX")
                    .setPositiveButtonText(getActivity().getString(R.string.ok));

            CollectDialogBundle collectDialogBundle = dialogBuilder.build();
            SimpleDialog dialogFragment = SimpleDialog.newInstance(collectDialogBundle);
            dialogFragment.show(((AdminPreferencesActivity) getActivity()).getSupportFragmentManager(), collectDialogBundle.getDialogTag());

and it works well so I think it's not a problem.

Forgive me if this should be obvious but why is CollectDialogBundle needed in this implementation?

It's not necessary in this approach but I think it's nice to have this class especially for SimpleDialog class (in other cases we can set most of elements in dialog class of course).
I made a quick investigation and looks like we will able to use this SimpleDialog class for about 15 dialogs (that don't need any callback) so we can reduce number of classes.

Generally I like this approach there is one class we can use for all simple dialogs that don't need any callbacks and for others we can create a separate class for each one.

I think this is what we need.

[lognaturel]

Replaced by #1269 for now. Some of the ideas explored here may come back as more complex dialog types are added in. @grzesiek2010 can you please keep the branch around so we can refer back to it?