oldergod / android-architecture

MVI architecture Implementation of the ToDo app.
Apache License 2.0
669 stars 70 forks source link

Duplicate Intents triggered when onStart/onStop called multiple times #49

Closed ichrisanderson closed 5 years ago

ichrisanderson commented 5 years ago

For example, on the TasksFragment if you background the app and then restore you can end up with multiple RefreshIntents being processed, adding some logs you can see this below.

2018-11-09 10:20:52.061 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksFragment.onStart
2018-11-09 10:20:52.571 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: com.example.android.architecture.blueprints.todoapp.tasks.TasksIntent$InitialIntent@c49fab6
2018-11-09 10:21:01.870 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:09.377 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksFragment.onStop
2018-11-09 10:21:11.244 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksFragment.onStart
2018-11-09 10:21:11.258 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:11.264 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:14.746 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksFragment.onStop
2018-11-09 10:21:16.083 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksFragment.onStart
2018-11-09 10:21:16.128 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:16.132 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:16.133 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:18.552 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksFragment.onStop
2018-11-09 10:21:20.390 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksFragment.onStart
2018-11-09 10:21:20.397 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:20.402 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:20.406 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:23.480 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksFragment.onStop
2018-11-09 10:21:25.974 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksFragment.onStart
2018-11-09 10:21:25.982 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:25.998 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:29.020 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksFragment.onStop
2018-11-09 10:21:30.883 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksFragment.onStart
2018-11-09 10:21:30.904 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:30.910 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:30.912 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:30.913 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:30.914 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)
2018-11-09 10:21:30.916 30351-30351/com.example.android.architecture.blueprints.todomvp.mock D/todoapp: TasksViewModel.actionFromIntent. intent: RefreshIntent(forceUpdate=false)

The issue looks to be related to the processIntents method with the ViewModel, where intents are subscribed multiple times but the old subscription never gets disposed. Its possible to ignore the duplicates by using the distinct operator but there's maybe a question as to whether you want to keep the old subscription active in the ViewModel

oldergod commented 5 years ago

Thanks for reporting, I don't have much time to look into it right now.

radityagumay commented 5 years ago

@oldergod @chrisanderson79

Take a look this code

override fun onResume() {
    super.onResume()
    // conflicting with the initial intent but needed when coming back from the
    // AddEditTask activity to refresh the list.
    refreshIntentPublisher.onNext(TasksIntent.RefreshIntent(false))
  }

since refreshIntent() having a function mergeWith refreshPublishIntent, on callback onResume there is a emissions that emit a value to the publish subject.

This causing emissions happened multiple times

oldergod commented 5 years ago

Thank you @radityagumay , do you have a proposal to fix that?

oldergod commented 5 years ago

The problem was introduced trying to fix #46 I reverted the code so this issue should be fixed now.

radityagumay commented 4 years ago

@oldergod thank you!