JorgeCastilloPrz / ArrowAndroidSamples

Functional Programing Android architecture using http://arrow-kt.io/
426 stars 38 forks source link

What about Activity Configuration change handling? #14

Open bornest opened 6 years ago

bornest commented 6 years ago

First of all, thank you so much for such elaborate examples and the amount of effort you're putting into bringing FP to Kotlin!

I've noticed that in all 4 examples Activity just passes a reference to itself to the other part of the app ( e.g. by calling getSuperHeroes().run(heroesContext) in onResume() ), so when the data is fetched from the server, it is displayed by calling drawHeroes() method directly on that Activity reference:

private fun drawHeroes(view: SuperHeroesListView, success: List<CharacterDto>) {
  view.drawHeroes(success.map {
    SuperHeroViewModel(
        it.id,
        it.name,
        it.thumbnail.getImageUrl(PORTRAIT_UNCANNY),
        it.description)
  })
}

But what if this Activity is currently undergoing a Configuration change? (it can be destroyed and not yet initialized again) I assume in that case it may result in NPE or some other exception.

How do you actually handle this situation in such architecture?

P.S. I haven't been able to really check this, but I'm pretty sure that's how it is.

JorgeCastilloPrz commented 6 years ago

Hi!

Yes you are right, this project is still not implementing a solution for configuration changes. I have some ways on mind by using Option(view) since option.map {} is just "ignored" when the option is a None. But that would require having more shared state than the one I want to have, like keeping a reference to the already created context on the activity or the presenter, to be able to swap the view ref by a None or a Some() inside of it following the activity lifecycle methods onResume() and onPause(). Also to make the view reference a mutable var. That would remove any possible NPEs since the view will never try to render if we play with an option, and new data would be requested every single time in onResume(), but that's not a problem if we have our caches in place behind the repository. That's kind of transparent to our domain / presentation / view layers

But I quite don't like the need to keep a ref to the context and so on.

So I am still making my mind around how I should approach it in a functional style.

Thanks for your feedback!

bornest commented 6 years ago

Hi, thanks for detailed answer!

From what I've seen so far, I'm afraid there are no easy & beautiful ways of integrating complex Android framework elements like Activities and Fragments into pure functional architecture, because they're built with a completely different model in mind.

Personally, I think some kind of MVI approach e.g. as it's described by Hannes Dorfmann (also see flowchart) is the closest to truth in this respect:

We let Views (Activities & Fragments) have their usual lifecycle and use 2 Rx Subjects (input & output) as the way of communicating with it to make the rest of the app lifecycle-agnostic (and Android-agnostic in general). The only responsibility of the Views is to forward UI events to "out" bus and render the ViewStates from "in" bus. This way we can shift most of the logic to the rest of the app (Presenters, Repositories, etc.) and make it completely functional there.

emmano commented 6 years ago

Thanks a lot for the time you have put into this.

We have leveraged [onRetainCustomNonConfigurationInstance()](https://developer.android.com/reference/android/support/v4/app/FragmentActivity.html#onRetainCustomNonConfigurationInstance())/[getLastCustomNonConfigurationInstance()](https://developer.android.com/reference/android/support/v4/app/FragmentActivity.html#getLastCustomNonConfigurationInstance()) retain/retrieve the @PerActivity(or more accurately "PerScreen") scoped component (Dagger) that holds the dependencies for the screen in question. I think you can probably use this technique to retain GetHeroesContext and get the same dependencies as you had before the config change happened.

Do you plan to add tests to this repo?

P.S. just saw your comment on #15 regarding testing. I mean unit level tests. Since we are mostly dealing with pure functions you do not see the need to test them?

bornest commented 6 years ago

That's a very interesting approach and it can solve a lot of problems!

However, I can't see how it solves the problem of calling View's (Activity's/Fragment's) methods like drawHeroes() directly from other parts of the app that outlive the View (e.g. code that talks to db & server):

in order for it to be safe we need to make sure that we're calling methods of the Activity/Fragment instance that's currently alive and not the old one.

So even if we store somewhere a reference to Activity/Fragment and update it after configuration changes as soon as possible there's inherently a race condition over this shared state (i.e. Activity/Fragment reference) between code that updates this reference (i.e. the Activity/Fragment itself) and the code that uses it (i.e. the other part of the app like code that talks to server).

We may try to eliminate the race condition by using some kind of locking mechanism, but I'm not sure whether it's a viable approach. (and it's getting further and further away from the functional style :)

emmano commented 6 years ago

You are correct. My comment was regarding how to retain scoped dependencies.

To solve the issue of view being null during config changes you can probably make the data layer be lifecycle aware. Here is an example of how to do if for RxJava.

lukas1 commented 6 years ago

I also have one idea how to solve this problem. In fact, there actually is something, that I don't like about this example repo. I don't like the idea, that functions such as getSuperHeroes() are called directly from activity. I can test that method, but I can't really test that it is called in onResume() (testing android classes is painful).

I'd like to have a presenter class and I'd actually consider the presenter the 'UI edge' of the application - I'd allow side effects in the presenter.

How does this solve the issue about view being null and retaining instance during configuration changes? The first issue -> what about view being null, simple, view can get attached to presenter and detached from it. Retaining instance is bigger problem, but it can be solved using Loader. Loader is an Android concept, that will keep in memory instance of the class that it serves even across orientation changes, until it is no longer necessary.

Most Android developers will be familiar with Loaders probably via CursorLoader, that retained instance of Cursor even across configuration changes. There's this nice article that describes MVP using loaders: https://medium.com/@czyrux/presenter-surviving-orientation-changes-with-loaders-6da6d86ffbbf

Let me know what you think about this approach, especially what you think about the idea of adding one extra layer to Activity, that also allows functions with side effects. Of course I propose the Activity to be devoid of whatever logic (including preparation of context for the Reader etc.) and to keep side effects in presenters at bare minimum (for example I'd store values of text fields etc inside a Bundle using onSaveInstanceState within Activities, not presenters)

emmano commented 6 years ago

Loaders use the mechanism that I described above.