From e0ccb6e8e2a5552108ca68385e22b64f2f63e970 Mon Sep 17 00:00:00 2001 From: Colin Lee Date: Tue, 6 Aug 2019 16:36:31 -0500 Subject: [PATCH] For #4529, #4427: Resuming after restoring instance state breaks UI --- CHANGELOG.md | 6 + .../fenix/browser/BaseBrowserFragment.kt | 328 ++++++++++-------- .../mozilla/fenix/browser/BrowserFragment.kt | 116 +++---- .../settings/DeleteBrowsingDataFragment.kt | 10 +- 4 files changed, 247 insertions(+), 213 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bf4ae9b0abbc..f206b247e561 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - #4137 - Adds pagination to the history view - #3695 - Made search suggestions for other tabs clickable + +### Changed - Remove forced focus of toolbar on homescreen +- #4529 - Fixed an issue where the app would sometimes return to a blank toolbar +- #4427 - Fixed an issue where the app would sometimes return to the home fragment + +### Removed ## [1.1.0 and earlier] - 2019-07-23 ### Added diff --git a/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt b/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt index f1ae8a80ad2a..7ef1c17ffc0d 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt @@ -24,9 +24,12 @@ import kotlinx.android.synthetic.main.fragment_browser.* import kotlinx.android.synthetic.main.fragment_browser.view.* import kotlinx.coroutines.Dispatchers.IO import kotlinx.coroutines.Dispatchers.Main +import kotlinx.coroutines.Job +import kotlinx.coroutines.MainScope import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import mozilla.components.browser.session.Session +import mozilla.components.browser.session.SessionManager import mozilla.components.browser.session.intent.EXTRA_SESSION_ID import mozilla.components.feature.app.links.AppLinksFeature import mozilla.components.feature.contextmenu.ContextMenuFeature @@ -72,8 +75,8 @@ import org.mozilla.fenix.utils.Settings * This class only contains shared code focused on the main browsing content. * UI code specific to the app or to custom tabs can be found in the subclasses. */ -@Suppress("TooManyFunctions") -abstract class BaseBrowserFragment : Fragment(), BackHandler { +@Suppress("TooManyFunctions", "LargeClass") +abstract class BaseBrowserFragment : Fragment(), BackHandler, SessionManager.Observer { protected lateinit var browserStore: BrowserStore protected lateinit var browserInteractor: BrowserInteractor protected lateinit var browserToolbarView: BrowserToolbarView @@ -91,6 +94,9 @@ abstract class BaseBrowserFragment : Fragment(), BackHandler { var customTabSessionId: String? = null + private var browserInitialized: Boolean = false + private var initUIJob: Job? = null + @CallSuper override fun onCreateView( inflater: LayoutInflater, @@ -123,14 +129,18 @@ abstract class BaseBrowserFragment : Fragment(), BackHandler { return view } - @Suppress("ComplexMethod") @CallSuper override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) + browserInitialized = initializeUI(view) != null + } + @Suppress("ComplexMethod") + @CallSuper + protected open fun initializeUI(view: View): Session? { val sessionManager = requireComponents.core.sessionManager - getSessionById()?.let { session -> + return getSessionById()?.also { session -> val viewModel = activity!!.run { ViewModelProvider(this).get(CreateCollectionViewModel::class.java) } @@ -174,157 +184,179 @@ abstract class BaseBrowserFragment : Fragment(), BackHandler { browserToolbarView.view.setOnSiteSecurityClickedListener { showQuickSettingsDialog() } - } - contextMenuFeature.set( - feature = ContextMenuFeature( - requireFragmentManager(), - sessionManager, - FenixContextMenuCandidate.defaultCandidates( - requireContext(), - requireComponents.useCases.tabsUseCases, - view, - FenixSnackbarDelegate( + contextMenuFeature.set( + feature = ContextMenuFeature( + requireFragmentManager(), + sessionManager, + FenixContextMenuCandidate.defaultCandidates( + requireContext(), + requireComponents.useCases.tabsUseCases, view, - if (getSessionById()?.isCustomTabSession() == true) null else nestedScrollQuickAction - ) + FenixSnackbarDelegate( + view, + if (getSessionById()?.isCustomTabSession() == true) null else nestedScrollQuickAction + ) + ), + view.engineView ), - view.engineView - ), - owner = this, - view = view - ) - - downloadsFeature.set( - feature = DownloadsFeature( - requireContext().applicationContext, - sessionManager = sessionManager, - fragmentManager = childFragmentManager, - sessionId = customTabSessionId, - downloadManager = FetchDownloadManager(requireContext().applicationContext, DownloadService::class), - onNeedToRequestPermissions = { permissions -> - requestPermissions(permissions, REQUEST_CODE_DOWNLOAD_PERMISSIONS) - }), - owner = this, - view = view - ) - - appLinksFeature.set( - feature = AppLinksFeature( - requireContext(), - sessionManager = sessionManager, - sessionId = customTabSessionId, - interceptLinkClicks = true, - fragmentManager = requireFragmentManager() - ), - owner = this, - view = view - ) - - promptsFeature.set( - feature = PromptFeature( - fragment = this, - sessionManager = sessionManager, - sessionId = customTabSessionId, - fragmentManager = requireFragmentManager(), - onNeedToRequestPermissions = { permissions -> - requestPermissions(permissions, REQUEST_CODE_PROMPT_PERMISSIONS) - }), - owner = this, - view = view - ) - - sessionFeature.set( - feature = SessionFeature( - sessionManager, - SessionUseCases(sessionManager), - view.engineView, - customTabSessionId - ), - owner = this, - view = view - ) - - val accentHighContrastColor = ThemeManager.resolveAttribute(R.attr.accentHighContrast, requireContext()) - - sitePermissionsFeature.set( - feature = SitePermissionsFeature( - context = requireContext(), - sessionManager = sessionManager, - fragmentManager = requireFragmentManager(), - promptsStyling = SitePermissionsFeature.PromptsStyling( - gravity = getAppropriateLayoutGravity(), - shouldWidthMatchParent = true, - positiveButtonBackgroundColor = accentHighContrastColor, - positiveButtonTextColor = R.color.photonWhite + owner = this, + view = view + ) + + downloadsFeature.set( + feature = DownloadsFeature( + requireContext().applicationContext, + sessionManager = sessionManager, + fragmentManager = childFragmentManager, + sessionId = customTabSessionId, + downloadManager = FetchDownloadManager(requireContext().applicationContext, DownloadService::class), + onNeedToRequestPermissions = { permissions -> + requestPermissions(permissions, REQUEST_CODE_DOWNLOAD_PERMISSIONS) + }), + owner = this, + view = view + ) + + appLinksFeature.set( + feature = AppLinksFeature( + requireContext(), + sessionManager = sessionManager, + sessionId = customTabSessionId, + interceptLinkClicks = true, + fragmentManager = requireFragmentManager() ), - sessionId = customTabSessionId - ) { permissions -> - requestPermissions(permissions, REQUEST_CODE_APP_PERMISSIONS) - }, - owner = this, - view = view - ) - - fullScreenFeature.set( - feature = FullScreenFeature( - sessionManager, - SessionUseCases(sessionManager), - customTabSessionId - ) { inFullScreen -> - if (inFullScreen) { - FenixSnackbar.make(view.rootView, Snackbar.LENGTH_SHORT) - .setText(getString(R.string.full_screen_notification)) - .show() - activity?.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_USER_LANDSCAPE - activity?.enterToImmersiveMode() - toolbar.visibility = View.GONE - nestedScrollQuickAction.visibility = View.GONE - } else { - activity?.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_USER - activity?.exitImmersiveModeIfNeeded() - (activity as HomeActivity).let { activity: HomeActivity -> - ThemeManager.applyStatusBarTheme( - activity.window, - activity.themeManager, - activity - ) - } - toolbar.visibility = View.VISIBLE - nestedScrollQuickAction.visibility = View.VISIBLE - } - view.swipeRefresh.apply { - val (topMargin, bottomMargin) = if (inFullScreen) 0 to 0 else getEngineMargins() - (layoutParams as CoordinatorLayout.LayoutParams).setMargins(0, topMargin, 0, bottomMargin) - } - }, - owner = this, - view = view - ) - - if (FeatureFlags.pullToRefreshEnabled) { - val primaryTextColor = ThemeManager.resolveAttribute(R.attr.primaryText, requireContext()) - view.swipeRefresh.setColorSchemeColors(primaryTextColor) - swipeRefreshFeature.set( - feature = SwipeRefreshFeature( - requireComponents.core.sessionManager, - requireComponents.useCases.sessionUseCases.reload, - view.swipeRefresh, + owner = this, + view = view + ) + + promptsFeature.set( + feature = PromptFeature( + fragment = this, + sessionManager = sessionManager, + sessionId = customTabSessionId, + fragmentManager = requireFragmentManager(), + onNeedToRequestPermissions = { permissions -> + requestPermissions(permissions, REQUEST_CODE_PROMPT_PERMISSIONS) + }), + owner = this, + view = view + ) + + sessionFeature.set( + feature = SessionFeature( + sessionManager, + SessionUseCases(sessionManager), + view.engineView, customTabSessionId ), owner = this, view = view ) - } else { - // Disable pull to refresh - view.swipeRefresh.setOnChildScrollUpCallback { _, _ -> true } + + val accentHighContrastColor = ThemeManager.resolveAttribute(R.attr.accentHighContrast, requireContext()) + + sitePermissionsFeature.set( + feature = SitePermissionsFeature( + context = requireContext(), + sessionManager = sessionManager, + fragmentManager = requireFragmentManager(), + promptsStyling = SitePermissionsFeature.PromptsStyling( + gravity = getAppropriateLayoutGravity(), + shouldWidthMatchParent = true, + positiveButtonBackgroundColor = accentHighContrastColor, + positiveButtonTextColor = R.color.photonWhite + ), + sessionId = customTabSessionId + ) { permissions -> + requestPermissions(permissions, REQUEST_CODE_APP_PERMISSIONS) + }, + owner = this, + view = view + ) + + fullScreenFeature.set( + feature = FullScreenFeature( + sessionManager, + SessionUseCases(sessionManager), + customTabSessionId + ) { inFullScreen -> + if (inFullScreen) { + FenixSnackbar.make(view.rootView, Snackbar.LENGTH_SHORT) + .setText(getString(R.string.full_screen_notification)) + .show() + activity?.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_USER_LANDSCAPE + activity?.enterToImmersiveMode() + toolbar.visibility = View.GONE + nestedScrollQuickAction.visibility = View.GONE + } else { + activity?.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_USER + activity?.exitImmersiveModeIfNeeded() + (activity as HomeActivity).let { activity: HomeActivity -> + ThemeManager.applyStatusBarTheme( + activity.window, + activity.themeManager, + activity + ) + } + toolbar.visibility = View.VISIBLE + nestedScrollQuickAction.visibility = View.VISIBLE + } + view.swipeRefresh.apply { + val (topMargin, bottomMargin) = if (inFullScreen) 0 to 0 else getEngineMargins() + (layoutParams as CoordinatorLayout.LayoutParams).setMargins(0, topMargin, 0, bottomMargin) + } + }, + owner = this, + view = view + ) + + @Suppress("ConstantConditionIf") + if (FeatureFlags.pullToRefreshEnabled) { + val primaryTextColor = ThemeManager.resolveAttribute(R.attr.primaryText, requireContext()) + view.swipeRefresh.setColorSchemeColors(primaryTextColor) + swipeRefreshFeature.set( + feature = SwipeRefreshFeature( + requireComponents.core.sessionManager, + requireComponents.useCases.sessionUseCases.reload, + view.swipeRefresh, + customTabSessionId + ), + owner = this, + view = view + ) + } else { + // Disable pull to refresh + view.swipeRefresh.setOnChildScrollUpCallback { _, _ -> true } + } + + (activity as HomeActivity).updateThemeForSession(session) } } + @CallSuper + override fun onSessionSelected(session: Session) { + super.onSessionSelected(session) + if (!browserInitialized) { + // Initializing a new coroutineScope to avoid ConcurrentModificationException in ObserverRegistry + // This will be removed when ObserverRegistry is deprecated by browser-state. + initUIJob = MainScope().launch { + view?.let { + browserInitialized = initializeUI(it) != null + } + } + } + } + + @CallSuper + override fun onStart() { + super.onStart() + requireComponents.core.sessionManager.register(this, this, autoPause = true) + } + @CallSuper override fun onResume() { super.onResume() - val session = getSessionById() ?: return val components = requireComponents val preferredColorScheme = components.core.getPreferredColorScheme() @@ -332,20 +364,23 @@ abstract class BaseBrowserFragment : Fragment(), BackHandler { components.core.engine.settings.preferredColorScheme = preferredColorScheme components.useCases.sessionUseCases.reload() } - (activity as HomeActivity).updateThemeForSession(session) (activity as AppCompatActivity).supportActionBar?.hide() assignSitePermissionsRules() } - /** - * Exits full screen mode. - */ + @CallSuper final override fun onPause() { super.onPause() fullScreenFeature.onBackPressed() } + @CallSuper + override fun onStop() { + super.onStop() + initUIJob?.cancel() + } + @CallSuper override fun onBackPressed(): Boolean { return findInPageIntegration.onBackPressed() || @@ -469,8 +504,9 @@ abstract class BaseBrowserFragment : Fragment(), BackHandler { */ protected fun getSessionById(): Session? { val sessionManager = context?.components?.core?.sessionManager ?: return null - return if (customTabSessionId != null) { - sessionManager.findSessionById(customTabSessionId!!) + val localCustomTabId = customTabSessionId + return if (localCustomTabId != null) { + sessionManager.findSessionById(localCustomTabId) } else { sessionManager.selectedSession } 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 8800bcf11637..5a656e6ae45c 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt @@ -14,7 +14,6 @@ import android.widget.RadioButton import androidx.core.content.ContextCompat import androidx.lifecycle.Observer import androidx.lifecycle.lifecycleScope -import androidx.navigation.fragment.NavHostFragment.findNavController import androidx.navigation.fragment.findNavController import androidx.transition.TransitionInflater import com.google.android.material.snackbar.Snackbar @@ -29,7 +28,6 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import mozilla.appservices.places.BookmarkRoot import mozilla.components.browser.session.Session -import mozilla.components.browser.session.SessionManager import mozilla.components.feature.readerview.ReaderViewFeature import mozilla.components.feature.session.ThumbnailsFeature import mozilla.components.lib.state.ext.consumeFrom @@ -58,6 +56,8 @@ import java.net.URL /** * Fragment used for browsing the web within the main app and external apps. */ +@ObsoleteCoroutinesApi +@ExperimentalCoroutinesApi @Suppress("TooManyFunctions") class BrowserFragment : BaseBrowserFragment(), BackHandler { private lateinit var quickActionSheetView: QuickActionSheetView @@ -90,17 +90,12 @@ class BrowserFragment : BaseBrowserFragment(), BackHandler { return view } - @Suppress("LongMethod", "ComplexMethod") - @ObsoleteCoroutinesApi - @ExperimentalCoroutinesApi - override fun onViewCreated(view: View, savedInstanceState: Bundle?) { - super.onViewCreated(view, savedInstanceState) - + override fun initializeUI(view: View): Session? { val sessionManager = requireComponents.core.sessionManager - getSessionById()?.let { - quickActionSheetView = - QuickActionSheetView(view.nestedScrollQuickAction, browserInteractor) + return super.initializeUI(view)?.also { session -> + + quickActionSheetView = QuickActionSheetView(view.nestedScrollQuickAction, browserInteractor) customTabSessionId?.let { customTabSessionId -> customTabsIntegration.set( @@ -118,57 +113,58 @@ class BrowserFragment : BaseBrowserFragment(), BackHandler { view = view) } + thumbnailsFeature.set( + feature = ThumbnailsFeature( + requireContext(), + view.engineView, + requireComponents.core.sessionManager + ), + owner = this, + view = view + ) + + readerViewFeature.set( + feature = ReaderViewFeature( + requireContext(), + requireComponents.core.engine, + requireComponents.core.sessionManager, + view.readerViewControlsBar + ) { available -> + if (available) { + requireComponents.analytics.metrics.track(Event.ReaderModeAvailable) + } + + browserStore.apply { + dispatch(QuickActionSheetAction.ReadableStateChange(available)) + dispatch( + QuickActionSheetAction.ReaderActiveStateChange( + sessionManager.selectedSession?.readerMode ?: false + ) + ) + } + }, + owner = this, + view = view + ) + + if ((activity as HomeActivity).browsingModeManager.isPrivate) { + // We need to update styles for private mode programmatically for now: + // https://github.com/mozilla-mobile/android-components/issues/3400 + themeReaderViewControlsForPrivateMode(view.readerViewControlsBar) + } + + updateBookmarkState(session) + consumeFrom(browserStore) { quickActionSheetView.update(it) browserToolbarView.update(it) } } - - thumbnailsFeature.set( - feature = ThumbnailsFeature( - requireContext(), - view.engineView, - requireComponents.core.sessionManager - ), - owner = this, - view = view - ) - - readerViewFeature.set( - feature = ReaderViewFeature( - requireContext(), - requireComponents.core.engine, - requireComponents.core.sessionManager, - view.readerViewControlsBar - ) { available -> - if (available) { - requireComponents.analytics.metrics.track(Event.ReaderModeAvailable) - } - - browserStore.apply { - dispatch(QuickActionSheetAction.ReadableStateChange(available)) - dispatch( - QuickActionSheetAction.ReaderActiveStateChange( - sessionManager.selectedSession?.readerMode ?: false - ) - ) - } - }, - owner = this, - view = view - ) - - if ((activity as HomeActivity).browsingModeManager.isPrivate) { - // We need to update styles for private mode programmatically for now: - // https://github.com/mozilla-mobile/android-components/issues/3400 - themeReaderViewControlsForPrivateMode(view.readerViewControlsBar) - } } override fun onStart() { super.onStart() subscribeToSession() - subscribeToSessions() subscribeToTabCollections() } @@ -176,11 +172,6 @@ class BrowserFragment : BaseBrowserFragment(), BackHandler { super.onResume() requireComponents.core.tabCollectionStorage.register(collectionStorageObserver, this) - - getSessionById()?.let { updateBookmarkState(it) } - - // See #4387 for why we're popping here - if (getSessionById() == null) findNavController(this).popBackStack(R.id.homeFragment, false) } override fun onBackPressed(): Boolean { @@ -331,14 +322,9 @@ class BrowserFragment : BaseBrowserFragment(), BackHandler { getSessionById()?.register(observer, this, autoPause = true) } - private fun subscribeToSessions() { - val observer = object : SessionManager.Observer { - override fun onSessionSelected(session: Session) { - (activity as HomeActivity).updateThemeForSession(session) - updateBookmarkState(session) - } - } - requireComponents.core.sessionManager.register(observer, this, autoPause = true) + override fun onSessionSelected(session: Session) { + super.onSessionSelected(session) + updateBookmarkState(session) } private suspend fun findBookmarkedURL(session: Session?): Boolean { diff --git a/app/src/main/java/org/mozilla/fenix/settings/DeleteBrowsingDataFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/DeleteBrowsingDataFragment.kt index fe715f349ab9..0637814d0ee0 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/DeleteBrowsingDataFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/DeleteBrowsingDataFragment.kt @@ -14,6 +14,7 @@ import androidx.appcompat.app.AppCompatActivity import androidx.fragment.app.Fragment import androidx.lifecycle.Observer import androidx.lifecycle.lifecycleScope +import androidx.navigation.fragment.findNavController import kotlinx.android.synthetic.main.fragment_delete_browsing_data.* import kotlinx.android.synthetic.main.fragment_delete_browsing_data.view.* import kotlinx.coroutines.Dispatchers @@ -114,14 +115,15 @@ class DeleteBrowsingDataFragment : Fragment() { } } - fun startDeletion() { + private fun startDeletion() { progress_bar.visibility = View.VISIBLE delete_browsing_data_wrapper.isEnabled = false delete_browsing_data_wrapper.isClickable = false delete_browsing_data_wrapper.alpha = DISABLED_ALPHA } - fun finishDeletion() { + private fun finishDeletion() { + val popAfter = open_tabs_item.isChecked progress_bar.visibility = View.GONE delete_browsing_data_wrapper.isEnabled = true delete_browsing_data_wrapper.isClickable = true @@ -138,6 +140,10 @@ class DeleteBrowsingDataFragment : Fragment() { FenixSnackbar.make(view!!, FenixSnackbar.LENGTH_SHORT) .setText(resources.getString(R.string.preferences_delete_browsing_data_snackbar)) .show() + + if (popAfter) viewLifecycleOwner.lifecycleScope.launch(Dispatchers.Main) { + findNavController().popBackStack(R.id.homeFragment, false) + } } private fun updateDeleteButton() {