Refactoring dialogs

[grzesiek2010]

There are lots of dialogs in our app. I think we really need to refactor this part of code as it tooks much code and it's really annoying.
First of all we should use DialogFragment of course (thanks to that we don't have to worry about activity recreation - our dialog is still visible and we don't need additional code to ensure that).

The second thing is as I mentioned we have lots of dialogs and I'm not sure creating separate class for each is a good idea. I proposed this solution https://github.com/grzesiek2010/collect/blob/c074d12ac6058db3760844fd55a1a558a658752a/collect_app/src/main/java/org/odk/collect/android/utilities/ODKDialogFragment.java it wasn't finished it was only a proposal (one creator class).

I found something intresting on github as well:
https://github.com/michael-rapp/AndroidMaterialDialog

Let's discuss!

[UrjaPawar]

I would like to work on this issue @grzesiek2010

[grzesiek2010]

@UrjaPawar sure but it's rather for discussion now. We have to chose the best option for us and then we can start work with the code. Please review options mentioned above and let us know what you think. Maybe you can add your own proposal - don't hasitate if you know a better solution.

[shobhitagarwal1612]

@grzesiek2010 We should also provide an option to create custom dialogs. For that we would need to extend DialogFragments and then override the setContentView.

[UrjaPawar]

@grzesiek2010 Yes, let me consider each of them carefully. Since app is going to be updated according to new design as stated here, I think this https://github.com/michael-rapp/AndroidMaterialDialogone should be preferred

[grzesiek2010]

@shobhitagarwal1612 I'm not sure it depends on te approach we choose. For example that solution provide a wide range of dialogs so maybe we can releace all our dialogs using one of ready-made solutions.

[shobhitagarwal1612]

@grzesiek2010 I agree that using an open source library is more handy than writing our own code for the implemented solution.But the library you proposed doesn't have many stars or forks. That means that library might not be fully tested and may contain potential bugs.

I found another library on github which is very popular.
https://github.com/afollestad/material-dialogs

[grzesiek2010]

@shobhitagarwal1612 I saw that solution as well and I tested a sample app but in that solution dialogs are not visible after activity recreation. It's a problem but maybe we can change that I don't know.

@UrjaPawar you can test that solution as well.

[shobhitagarwal1612]

@grzesiek2010 Can you please describe how to recreate the issue that you encountered while using the sample app

[grzesiek2010]

@shobhitagarwal1612
You need to use this sample ap https://github.com/afollestad/material-dialogs/blob/master/sample/sample.apk
then open the first dialog and change your device orientation. Then the dialog disappears.

[UrjaPawar]

@grzesiek2010 I checked out the sample app, seems we need to override onConfigurationChanged method and find out more of these issues

[grzesiek2010]

@UrjaPawar if we use dialogs (with DialogFragment) it should work out of the box. The solution attahced by me https://github.com/michael-rapp/AndroidMaterialDialog works well.

[UrjaPawar]

@grzesiek2010 Yes! I saw that thing now

[lognaturel]

I'm not convinced of the need to introduce an external dependency for this. There are some risks in doing so. For example, we change the minSdkVersion slowly so we may end up not being able to update the lib anymore but find we need updated behavior. Or maybe they'll decide to also search for aliens in their library and we'll end up with a big dependency when all we needed was a good set of abstractions for representing dialogs.

I do think they have nice inheritance hierarchies for dialogs that we could consider borrowing ideas from.

I tend not to like having a single creator because it groups together unrelated behavior. I think there is some value in hiding some of the implementation details for dialog types that are used over and over again but I believe it should be up to the clients of those abstractions to do things like specify the actions of the dialog buttons. This specific comment is in reaction to resolveButtonAction here.

@nribeka @batkinson @yanokwa Would be interested in some additional opinions.

[nribeka]

I think we should take a step back and ask ourselves what are we trying to get with replacing the dialogs. Weigh in the pros and cons we're getting by replacing the dialogs.

Are we just trying to replace the dialogs approach because different activities have their own dialogs or is actual problem caused by we're using too much dialogs inside the apps that can be replaced with different approach?

I'm not a fan of introducing a lot of external dependencies in apps too. And if we decided to introduce dependency, we need to look at how mature the dependency are (how many forks, how many stars, how many open issues, how active is the development cycle). I worked with a lot of javascript lately and almost every day there's new library come out and also another library dead in the water 🤣

[grzesiek2010]

I didn't say that we have to use an external dependencies it's only an option we can introduce our own solution.
I can see two pros:
1.Our current approach tooks much code so we can reduce the code
2. If we use DialogFragment we don't need to implement additional code to create dialogs after activity recreation.

[yanokwa]

@grzesiek2010 And the cons?

[grzesiek2010]

@yanokwa At the moment I don't know any con. It's only time we have to spend :)

[nikhilbalyan]

Hi @grzesiek2010 i would like to give a piece of thought regarding this issue have a look at #610

[nikhilbalyan]

@grzesiek2010 and @lognaturel inside the FileChooserList activity from Line 217 to Line 219

   217.         mAlertDialog.setCancelable(false);
   218.         mAlertDialog.setButton(getString(R.string.ok), errorListener);
   219.         mAlertDialog.show();

In the line 218 instead of setButton(Deprecated) it should be setPositiveButton. http://stackoverflow.com/questions/9723302/alert-dialog-inside-an-onclick-listener-method

[yanokwa]

I want to take another step back! Our goal is to reduce redundancy across the code base. I'd rather not use an external dependency for this if we don't have to, but before we decide, we need to know what we have.

1. Review the dialogs that we use. Make a list of the different types (text, select one, select multiple, progress, etc) and a count.

2. Build an abstraction that uses the stock Android frameworks to encapsulate the various types. My guess is that there won't be a lot of code there and it'll give us more control. If you need inspiration, look at the suggested libraries.

@grzesiek2010 Can you take the lead on this?

[grzesiek2010]

@yanokwa sure
I agree with you. An initial investigation what we use is a good first step. Does someone want to do that?

[grzesiek2010]

Here is a list of our dialogs https://docs.google.com/document/d/1pZnr7wll-Wiw7lAakkgQlSEH4i683HZMdfjc8OUt-yc/edit?usp=sharing

[lognaturel]

@grzesiek2010 That is super helpful. Good news is we really don't have that many different types.

I think it would be best to address this after we've started using the Android Support Libraries in #912, right? @shobhitagarwal1612

[grzesiek2010]

@lognaturel yes we can wait for #912

[lognaturel]

@grzesiek2010 @shobhitagarwal1612 I think making the look and feel of dialogs consistent will be an important part of making the UI feel fresh and modern. I've marked it as part of the UI Refresh project but that doesn't necessarily mean @shobhitagarwal1612 has to be the one to do it.

Now that #912 is merged, we can start thinking about what the best abstractions would be and when this should be done. Should it happen in master or ui-refresh?

[lognaturel]

@grzesiek2010 @shobhitagarwal1612 Dialogs are rather chaotic right now and I think making them consistent and matching material design guidelines seems it would be a good, non-controversial next UI improvement that would provide high value for the future by making it easier to add additional, consistent dialogs when needed.

Looking at the dialog list, have you decided some of the abstractions that you'd like to introduce? Do you have a code structure to propose? Changes to propose across the board (for example, perhaps the "information" icon that a lot of dialogs have isn't needed anymore)?

[grzesiek2010]

@lognaturel yes my proposal is ready #1226

[lognaturel]

For clarity, I want to make sure we're all on the same page regarding the goals here. I think the number 1 goal is to make sure dialogs have a consistent look and feel that matches modern guidelines. A secondary and also important goal is that the code for generating dialogs is consistent. This will help avoid errors moving forward and make it easier to add further dialogs as needed. Does everyone agree with those goals or want to make any changes to them?

Keeping those in mind, @grzesiek2010 @shobhitagarwal1612, can the two of you please write up a description of the proposed approach, alternatives you considered, how you made the decisions you made, examples you looked at and other such information so that it's easier to provide feedback and make sure that we are going in the right direction. This change has far-reaching consequences and we want to make sure it is approached in a thoughtful way.

A couple of specific questions I have:

• What alternatives did you consider to creating a new app-wide base activity class? In general, I think adding that kind of layer makes code harder to understand because it's unexpected. Could the context instead be passed in to factory methods?
• Having the code in buildResetSettingsFinalDialog and resolveButtonAction far from where the dialog is actually created and used makes it somewhat harder to understand what the dialog is for. What alternatives did you consider?

@srsudar We were talking about this briefly recently. Do you happen to know of any good examples that use DialogFragment and keep dialogs organized and consistent across a code base?

[grzesiek2010]

Does everyone agree with those goals or want to make any changes to them?

My order is a little bit different because I think the most important thing is to manage dialog easier and avoid some issues related to the current approach, layout is less important to me but generally I agree with you.

I know this approach from my last project (TaroWorks) and it is still used as I know and works well. I wasn't able to find any other good solution without using external dependencies (I know you want to avoid it and me too).

It's not necessary to keep buildResetSettingsFinalDialog method in abstract class, this was only my proposal we can also move it to the class it's call from (ResetDialogPreference). I moved that method so you can look at it now again).

@shobhitagarwal1612 what about you?

[srsudar]

I don't have any good pointers to follow. A static creator function as #1226 is common practice and makes sense to me, as long as it is testable which I think it would be. I've begun judiciously avoiding Fragments in my recent projects--maybe because I'm bad at them--so I don't feel like a good source of best practices. As long as the dialog solution that emerges can be testably integrated with parent UI elements I think I'll be happy with changes.

I looked at google-samples/android-architecture and android-architecture-components, but none of those projects appear to use dialogs. I'd love to rely less on dialogs, as I find them finicky and I don't like Fragments, but the great list that @grzesiek2010 put together shows how much Collect relies on them.

[YuraLaguta]

@grzesiek2010 have you considered BottomSheets as alternative to some dialogs?
With regards to Progress indication sometimes it could be part of the screen, for example just circular progress bar inside view, or for long running tasks < 10 seconds maybe Notification with Background running service.
I'm also in favour of Fragments.

[grzesiek2010]

@Yurii-Laguta at the moment we use BottomSheets only for #1242 but I'll keep it in mind.

[YuraLaguta]

As for DialogFragments I would suggest in order to avoid Interfaces hell to use event bus such as Otto and Builder pattern similar existing AlertDialog

@lognaturel

What alternatives did you consider to creating a new app-wide base activity class? In general, I think adding that kind of layer makes code harder to understand because it's unexpected.

I was surprised there is no Base activity. Because for example toolbar setup looks duplicated in all activities.

[srsudar]

Looks like the Otto README says it's deprecated in favor of rxjava and friends.

The times I've used rxjava I've loved it, but I'm not familiar enough with it to have good intuitions about how to use it for dialogs.

[YuraLaguta]

@srsudar that is interesting. After deeper reading of blog post about migrating from Otto to RxJava:
http://blog.kaush.co/2014/12/24/implementing-an-event-bus-with-rxjava-rxbus/
The noteworthy summary is:

So should I use RxBus instead of Otto/EventBus?

If you simply want to use an event bus with your Android app, you’re probably better off with a library like Otto (which i highly recommend) or EventBus. Otto has a clean api driven by annotations and is probably far more simpler to use.

If you’re familiar with Rx, already use RxJava in your app and want to get rid of an additional library, definitely try out the RxBus approach!

Given that this project doesn't have RxJava yet, and the 'steep learning curve' of it, I assume the main maintainers would prefer to go with Otto or EventBus. If it were my choice I would go with RxJava because nowadays it seems like mainstream.

@lognaturel @yanokwa ^^^ someone have to make a decision here.

[grzesiek2010]

I'm closing this issue, a lot has change since it was reported.