From 6d71faa44d434493d4e2479cb9699339ad61638a Mon Sep 17 00:00:00 2001 From: Colin Lee Date: Sat, 16 Feb 2019 19:04:32 -0600 Subject: [PATCH] Fixes #541: Crash on Home Screen The Android Lifecycle Architecture component does not have fine-grained enough lifecycle event callbacks to safely manage Rx subscriptions in Fragment lifecycles. Added autodispose to simplify. --- app/build.gradle | 2 + .../mozilla/fenix/browser/BrowserFragment.kt | 20 ++++------ .../org/mozilla/fenix/home/HomeFragment.kt | 8 ++-- .../fenix/library/history/HistoryFragment.kt | 39 ++++++++----------- .../mozilla/fenix/search/SearchFragment.kt | 20 +++++----- architecture/build.gradle | 4 ++ .../org/mozilla/fenix/mvi/ActionBusFactory.kt | 11 ++++++ buildSrc/src/main/java/Dependencies.kt | 6 +++ 8 files changed, 59 insertions(+), 51 deletions(-) diff --git a/app/build.gradle b/app/build.gradle index 4141a2eaa..a86f57eef 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -165,6 +165,8 @@ dependencies { implementation Deps.androidx_fragment implementation Deps.android_arch_navigation implementation Deps.android_arch_navigation_ui + + implementation Deps.autodispose } diff --git a/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt b/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt index 41feda56a..811163303 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt @@ -4,7 +4,6 @@ package org.mozilla.fenix.browser -import android.annotation.SuppressLint import android.content.Context import android.content.Intent import android.os.Bundle @@ -35,16 +34,16 @@ import org.mozilla.fenix.DefaultThemeManager import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.components.FindInPageIntegration -import org.mozilla.fenix.ext.requireComponents -import org.mozilla.fenix.ext.share -import org.mozilla.fenix.mvi.ActionBusFactory -import org.mozilla.fenix.mvi.getSafeManagedObservable import org.mozilla.fenix.components.toolbar.SearchAction import org.mozilla.fenix.components.toolbar.SearchState import org.mozilla.fenix.components.toolbar.ToolbarComponent import org.mozilla.fenix.components.toolbar.ToolbarIntegration import org.mozilla.fenix.components.toolbar.ToolbarMenu import org.mozilla.fenix.components.toolbar.ToolbarUIView +import org.mozilla.fenix.ext.requireComponents +import org.mozilla.fenix.ext.share +import org.mozilla.fenix.mvi.ActionBusFactory +import org.mozilla.fenix.mvi.getAutoDisposeObservable class BrowserFragment : Fragment(), BackHandler { private lateinit var toolbarComponent: ToolbarComponent @@ -101,11 +100,10 @@ class BrowserFragment : Fragment(), BackHandler { return view } - @SuppressLint("CheckResult") - override fun onCreate(savedInstanceState: Bundle?) { - super.onCreate(savedInstanceState) + override fun onViewCreated(view: View, savedInstanceState: Bundle?) { + super.onViewCreated(view, savedInstanceState) - getSafeManagedObservable() + getAutoDisposeObservable() .subscribe { when (it) { is SearchAction.ToolbarTapped -> Navigation.findNavController(toolbar) @@ -116,10 +114,6 @@ class BrowserFragment : Fragment(), BackHandler { is SearchAction.ToolbarMenuItemTapped -> handleToolbarItemInteraction(it) } } - } - - override fun onViewCreated(view: View, savedInstanceState: Bundle?) { - super.onViewCreated(view, savedInstanceState) sessionId = BrowserFragmentArgs.fromBundle(arguments!!).sessionId diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt index 62ced7646..ab4c718fa 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -4,7 +4,6 @@ package org.mozilla.fenix.home -import android.annotation.SuppressLint import android.content.res.Resources import android.graphics.drawable.BitmapDrawable import android.os.Bundle @@ -20,8 +19,8 @@ import kotlinx.android.synthetic.main.fragment_home.view.* import mozilla.components.browser.menu.BrowserMenu import mozilla.components.browser.session.Session import mozilla.components.browser.session.SessionManager -import org.mozilla.fenix.DefaultThemeManager import org.mozilla.fenix.BrowsingModeManager +import org.mozilla.fenix.DefaultThemeManager import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.ext.requireComponents @@ -32,8 +31,8 @@ import org.mozilla.fenix.home.tabs.TabsComponent import org.mozilla.fenix.home.tabs.TabsState import org.mozilla.fenix.isPrivate import org.mozilla.fenix.mvi.ActionBusFactory +import org.mozilla.fenix.mvi.getAutoDisposeObservable import org.mozilla.fenix.mvi.getManagedEmitter -import org.mozilla.fenix.mvi.getSafeManagedObservable import kotlin.math.roundToInt class HomeFragment : Fragment() { @@ -57,14 +56,13 @@ class HomeFragment : Fragment() { return view } - @SuppressLint("CheckResult") override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) (activity as AppCompatActivity).supportActionBar?.hide() setupHomeMenu() - getSafeManagedObservable() + getAutoDisposeObservable() .subscribe { when (it) { is TabsAction.Select -> { diff --git a/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt b/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt index dc625881e..d8ce40e3a 100644 --- a/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt @@ -4,7 +4,6 @@ package org.mozilla.fenix.library.history -import android.annotation.SuppressLint import android.os.Bundle import android.view.LayoutInflater import android.view.Menu @@ -21,13 +20,13 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.launch -import org.mozilla.fenix.HomeActivity import mozilla.components.support.base.feature.BackHandler +import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.mvi.ActionBusFactory +import org.mozilla.fenix.mvi.getAutoDisposeObservable import org.mozilla.fenix.mvi.getManagedEmitter -import org.mozilla.fenix.mvi.getSafeManagedObservable import kotlin.coroutines.CoroutineContext class HistoryFragment : Fragment(), CoroutineScope, BackHandler { @@ -45,32 +44,15 @@ class HistoryFragment : Fragment(), CoroutineScope, BackHandler { ): View? { val view = inflater.inflate(R.layout.fragment_history, container, false) historyComponent = HistoryComponent(view.history_layout, ActionBusFactory.get(this)) - return view } - @SuppressLint("CheckResult") override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) job = Job() setHasOptionsMenu(true) (activity as AppCompatActivity).supportActionBar?.show() - - getSafeManagedObservable() - .subscribe { - when (it) { - is HistoryAction.Select -> selectItem(it.item) - is HistoryAction.EnterEditMode -> getManagedEmitter() - .onNext(HistoryChange.EnterEditMode(it.item)) - is HistoryAction.AddItemForRemoval -> getManagedEmitter() - .onNext(HistoryChange.AddItemForRemoval(it.item)) - is HistoryAction.RemoveItemForRemoval -> getManagedEmitter() - .onNext(HistoryChange.RemoveItemForRemoval(it.item)) - is HistoryAction.BackPressed -> getManagedEmitter() - .onNext(HistoryChange.ExitEditMode) - } - } } private fun selectItem(item: HistoryItem) { @@ -97,14 +79,27 @@ class HistoryFragment : Fragment(), CoroutineScope, BackHandler { override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) - val eventEmitter = ActionBusFactory.get(this) + getAutoDisposeObservable() + .subscribe { + when (it) { + is HistoryAction.Select -> selectItem(it.item) + is HistoryAction.EnterEditMode -> getManagedEmitter() + .onNext(HistoryChange.EnterEditMode(it.item)) + is HistoryAction.AddItemForRemoval -> getManagedEmitter() + .onNext(HistoryChange.AddItemForRemoval(it.item)) + is HistoryAction.RemoveItemForRemoval -> getManagedEmitter() + .onNext(HistoryChange.RemoveItemForRemoval(it.item)) + is HistoryAction.BackPressed -> getManagedEmitter() + .onNext(HistoryChange.ExitEditMode) + } + } launch(Dispatchers.IO) { val items = requireComponents.core.historyStorage.getVisited() .mapIndexed { id, item -> HistoryItem(id, item) } launch(Dispatchers.Main) { - eventEmitter.emit(HistoryChange::class.java, HistoryChange.Change(items)) + getManagedEmitter().onNext(HistoryChange.Change(items)) } } } diff --git a/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt b/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt index 108fe8db0..818751f81 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt @@ -4,7 +4,6 @@ package org.mozilla.fenix.search -import android.annotation.SuppressLint import android.os.Bundle import android.view.LayoutInflater import android.view.View @@ -15,17 +14,17 @@ import androidx.navigation.Navigation import kotlinx.android.synthetic.main.fragment_search.view.* import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R -import org.mozilla.fenix.mvi.ActionBusFactory -import org.mozilla.fenix.mvi.getManagedEmitter -import org.mozilla.fenix.mvi.getSafeManagedObservable -import org.mozilla.fenix.search.awesomebar.AwesomeBarAction -import org.mozilla.fenix.search.awesomebar.AwesomeBarChange -import org.mozilla.fenix.search.awesomebar.AwesomeBarComponent -import org.mozilla.fenix.search.awesomebar.AwesomeBarState import org.mozilla.fenix.components.toolbar.SearchAction import org.mozilla.fenix.components.toolbar.SearchState import org.mozilla.fenix.components.toolbar.ToolbarComponent import org.mozilla.fenix.components.toolbar.ToolbarUIView +import org.mozilla.fenix.mvi.ActionBusFactory +import org.mozilla.fenix.mvi.getAutoDisposeObservable +import org.mozilla.fenix.mvi.getManagedEmitter +import org.mozilla.fenix.search.awesomebar.AwesomeBarAction +import org.mozilla.fenix.search.awesomebar.AwesomeBarChange +import org.mozilla.fenix.search.awesomebar.AwesomeBarComponent +import org.mozilla.fenix.search.awesomebar.AwesomeBarState class SearchFragment : Fragment() { private lateinit var toolbarComponent: ToolbarComponent @@ -54,7 +53,6 @@ class SearchFragment : Fragment() { return view } - @SuppressLint("CheckResult") override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) @@ -66,7 +64,7 @@ class SearchFragment : Fragment() { view.toolbar_wrapper.clipToOutline = false - getSafeManagedObservable() + getAutoDisposeObservable() .subscribe { when (it) { is SearchAction.UrlCommitted -> transitionToBrowser() @@ -76,7 +74,7 @@ class SearchFragment : Fragment() { } } - getSafeManagedObservable() + getAutoDisposeObservable() .subscribe { when (it) { is AwesomeBarAction.ItemSelected -> transitionToBrowser() diff --git a/architecture/build.gradle b/architecture/build.gradle index 80978e148..91fc85f03 100644 --- a/architecture/build.gradle +++ b/architecture/build.gradle @@ -37,4 +37,8 @@ dependencies { implementation Deps.rxAndroid implementation Deps.rxKotlin + + implementation Deps.autodispose + implementation Deps.autodispose_android + implementation Deps.autodispose_android_aac } diff --git a/architecture/src/main/java/org/mozilla/fenix/mvi/ActionBusFactory.kt b/architecture/src/main/java/org/mozilla/fenix/mvi/ActionBusFactory.kt index be13b7b7d..440eb7786 100644 --- a/architecture/src/main/java/org/mozilla/fenix/mvi/ActionBusFactory.kt +++ b/architecture/src/main/java/org/mozilla/fenix/mvi/ActionBusFactory.kt @@ -26,6 +26,9 @@ import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleObserver import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.OnLifecycleEvent +import com.uber.autodispose.AutoDispose.autoDisposable +import com.uber.autodispose.ObservableSubscribeProxy +import com.uber.autodispose.android.lifecycle.AndroidLifecycleScopeProvider import io.reactivex.Observable import io.reactivex.Observer import io.reactivex.rxkotlin.merge @@ -103,6 +106,11 @@ class ActionBusFactory private constructor(val owner: LifecycleOwner) { return if (map[clazz] != null) map[clazz] as Observable else create(clazz) } + fun getAutoDisposeObservable(clazz: Class): ObservableSubscribeProxy { + return getSafeManagedObservable(clazz) + .`as`(autoDisposable(AndroidLifecycleScopeProvider.from(owner))) + } + fun getManagedEmitter(clazz: Class): Observer { return if (map[clazz] != null) map[clazz] as Observer else create(clazz) } @@ -135,6 +143,9 @@ inline fun LifecycleOwner.emit(event: T) = inline fun LifecycleOwner.getSafeManagedObservable(): Observable = ActionBusFactory.get(this).getSafeManagedObservable(T::class.java) +inline fun LifecycleOwner.getAutoDisposeObservable(): ObservableSubscribeProxy = + ActionBusFactory.get(this).getAutoDisposeObservable(T::class.java) + inline fun LifecycleOwner.getManagedEmitter(): Observer = ActionBusFactory.get(this).getManagedEmitter(T::class.java) diff --git a/buildSrc/src/main/java/Dependencies.kt b/buildSrc/src/main/java/Dependencies.kt index 841102b19..66f8a4e0e 100644 --- a/buildSrc/src/main/java/Dependencies.kt +++ b/buildSrc/src/main/java/Dependencies.kt @@ -28,6 +28,8 @@ private object Versions { const val espresso_core = "2.2.2" const val android_arch_navigation = "1.0.0-beta02" + + const val autodispose = "1.1.0" } @Suppress("unused") @@ -109,5 +111,9 @@ object Deps { const val android_arch_navigation = "android.arch.navigation:navigation-fragment:${Versions.android_arch_navigation}" const val android_arch_navigation_ui = "android.arch.navigation:navigation-ui:${Versions.android_arch_navigation}" + const val autodispose = "com.uber.autodispose:autodispose:${Versions.autodispose}" + const val autodispose_android = "com.uber.autodispose:autodispose-android:${Versions.autodispose}" + const val autodispose_android_aac = "com.uber.autodispose:autodispose-android-archcomponents:${Versions.autodispose}" + const val autodispose_android_aac_test = "com.uber.autodispose:autodispose-android-archcomponents-test:${Versions.autodispose}" }