From 46924544b6dadcef3deccbf3274570f13d039b18 Mon Sep 17 00:00:00 2001 From: Sawyer Blatz Date: Thu, 25 Apr 2019 13:00:09 -0700 Subject: [PATCH] For #1975 & #1627: Refactors getSessionById in BrowserFragment --- .../java/org/mozilla/fenix/HomeActivity.kt | 37 ++--- .../mozilla/fenix/browser/BrowserFragment.kt | 144 +++++++++--------- .../org/mozilla/fenix/home/HomeFragment.kt | 11 +- .../library/bookmarks/BookmarkFragment.kt | 6 +- .../SelectBookmarkFolderFragment.kt | 2 +- .../mozilla/fenix/search/SearchFragment.kt | 7 +- .../fenix/settings/SettingsFragment.kt | 2 +- app/src/main/res/navigation/nav_graph.xml | 8 +- 8 files changed, 104 insertions(+), 113 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt index c0ae18885..b2da3c9d9 100644 --- a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt +++ b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt @@ -136,40 +136,43 @@ open class HomeActivity : AppCompatActivity() { private fun handleOpenedFromExternalSource() { intent?.putExtra(OPEN_TO_BROWSER, false) openToBrowser( + BrowserDirection.FromGlobal, SafeIntent(intent).getStringExtra(IntentProcessor.ACTIVE_SESSION_ID) - ?: components.core.sessionManager.selectedSession?.id, BrowserDirection.FromGlobal + ?: components.core.sessionManager.selectedSession?.id ) } fun openToBrowserAndLoad( - text: String, - sessionId: String? = null, + searchTermOrURL: String, + externalSessionId: String? = null, engine: SearchEngine? = null, from: BrowserDirection ) { - openToBrowser(sessionId, from) - load(text, sessionId, engine) + openToBrowser(from, externalSessionId) + load(searchTermOrURL, externalSessionId, engine) } - fun openToBrowser(sessionId: String?, from: BrowserDirection) { + fun openToBrowser(from: BrowserDirection, externalSessionId: String? = null) { val directions = when (from) { - BrowserDirection.FromGlobal -> NavGraphDirections.actionGlobalBrowser(sessionId) - BrowserDirection.FromHome -> HomeFragmentDirections.actionHomeFragmentToBrowserFragment(sessionId) - BrowserDirection.FromSearch -> SearchFragmentDirections.actionSearchFragmentToBrowserFragment(sessionId) + BrowserDirection.FromGlobal -> NavGraphDirections.actionGlobalBrowser(externalSessionId) + BrowserDirection.FromHome -> HomeFragmentDirections.actionHomeFragmentToBrowserFragment(externalSessionId) + BrowserDirection.FromSearch -> + SearchFragmentDirections.actionSearchFragmentToBrowserFragment(externalSessionId) BrowserDirection.FromSettings -> - SettingsFragmentDirections.actionSettingsFragmentToBrowserFragment(sessionId) + SettingsFragmentDirections.actionSettingsFragmentToBrowserFragment(externalSessionId) BrowserDirection.FromBookmarks -> - BookmarkFragmentDirections.actionBookmarkFragmentToBrowserFragment(sessionId) + BookmarkFragmentDirections.actionBookmarkFragmentToBrowserFragment(externalSessionId) BrowserDirection.FromBookmarksFolderSelect -> - SelectBookmarkFolderFragmentDirections.actionBookmarkSelectFolderFragmentToBrowserFragment(sessionId) + SelectBookmarkFolderFragmentDirections + .actionBookmarkSelectFolderFragmentToBrowserFragment(externalSessionId) BrowserDirection.FromHistory -> - HistoryFragmentDirections.actionHistoryFragmentToBrowserFragment(sessionId) + HistoryFragmentDirections.actionHistoryFragmentToBrowserFragment(externalSessionId) } navHost.navController.navigate(directions) } - private fun load(text: String, sessionId: String?, engine: SearchEngine?) { + private fun load(searchTermOrURL: String, sessionId: String?, engine: SearchEngine?) { val isPrivate = this.browsingModeManager.isPrivate val loadUrlUseCase = if (sessionId == null) { @@ -187,10 +190,10 @@ open class HomeActivity : AppCompatActivity() { } else components.useCases.searchUseCases.defaultSearch.invoke(searchTerms, engine) } - if (text.isUrl()) { - loadUrlUseCase.invoke(text.toNormalizedUrl()) + if (searchTermOrURL.isUrl()) { + loadUrlUseCase.invoke(searchTermOrURL.toNormalizedUrl()) } else { - searchUseCase.invoke(text) + searchUseCase.invoke(searchTermOrURL) } } 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 6c3bf06ed..99816b8ca 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BrowserFragment.kt @@ -94,7 +94,8 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { private val customTabsIntegration = ViewBoundFeatureWrapper() private lateinit var job: Job - var sessionId: String? = null + // Session id for custom tab or opened from external intent + var externalSessionId: String? = null override val coroutineContext: CoroutineContext get() = Dispatchers.IO + job @@ -109,15 +110,15 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { savedInstanceState: Bundle? ): View? { require(arguments != null) - sessionId = BrowserFragmentArgs.fromBundle(arguments!!).sessionId + externalSessionId = BrowserFragmentArgs.fromBundle(arguments!!).externalSessionId val view = inflater.inflate(R.layout.fragment_browser, container, false) toolbarComponent = ToolbarComponent( view.browserLayout, - ActionBusFactory.get(this), sessionId, + ActionBusFactory.get(this), externalSessionId, (activity as HomeActivity).browsingModeManager.isPrivate, - SearchState("", getSessionByIdOrUseSelectedSession().searchTerms, isEditing = false), + SearchState("", getSessionById()?.searchTerms ?: "", isEditing = false), search_engine_icon ) @@ -146,11 +147,7 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { } private fun getAppropriateLayoutGravity(): Int { - sessionId?.let { sessionId -> - if (requireComponents.core.sessionManager.findSessionById(sessionId)?.isCustomTabSession() == true) { - return Gravity.TOP - } - } + if (getSessionById()?.isCustomTabSession() == true) { return Gravity.TOP } return Gravity.BOTTOM } @@ -158,8 +155,6 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) - sessionId = BrowserFragmentArgs.fromBundle(arguments!!).sessionId - val sessionManager = requireComponents.core.sessionManager contextMenuFeature.set( @@ -206,7 +201,7 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { sessionManager, SessionUseCases(sessionManager), view.engineView, - sessionId + externalSessionId ), owner = this, view = view @@ -241,7 +236,7 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { feature = FullScreenFeature( sessionManager, SessionUseCases(sessionManager), - sessionId + externalSessionId ) { if (it) { FenixSnackbar.make(view.rootView, Snackbar.LENGTH_LONG) @@ -275,21 +270,18 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { val actionEmitter = ActionBusFactory.get(this).getManagedEmitter(SearchAction::class.java) - sessionId?.let { sessionId -> - if (sessionManager.findSessionById(sessionId)?.isCustomTabSession() == true) { - customTabsIntegration.set( - feature = CustomTabsIntegration( - requireContext(), - requireComponents.core.sessionManager, - toolbar, - sessionId, - activity, - onItemTapped = { actionEmitter.onNext(SearchAction.ToolbarMenuItemTapped(it)) } - ), - owner = this, - view = view - ) - } + externalSessionId?.let { + customTabsIntegration.set( + feature = CustomTabsIntegration( + requireContext(), + requireComponents.core.sessionManager, + toolbar, + it, + activity, + onItemTapped = { actionEmitter.onNext(SearchAction.ToolbarMenuItemTapped(it)) } + ), + owner = this, + view = view) } toolbarComponent.getView().setOnSiteSecurityClickedListener { @@ -322,7 +314,7 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { .findNavController(toolbarComponent.getView()) .navigate( BrowserFragmentDirections.actionBrowserFragmentToSearchFragment( - getSessionByIdOrUseSelectedSession().id + getSessionById()?.id ) ) @@ -335,10 +327,12 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { handleToolbarItemInteraction(it) } is SearchAction.ToolbarLongClicked -> { - getSessionByIdOrUseSelectedSession().copyUrl(requireContext()) - FenixSnackbar.make(view!!, Snackbar.LENGTH_LONG) - .setText(resources.getString(R.string.url_copied)) - .show() + getSessionById()?.let { session -> + session.copyUrl(requireContext()) + FenixSnackbar.make(view!!, Snackbar.LENGTH_LONG) + .setText(resources.getString(R.string.url_copied)) + .show() + } } } } @@ -354,7 +348,9 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { } is QuickActionAction.SharePressed -> { requireComponents.analytics.metrics.track(Event.QuickActionSheetShareTapped) - getSessionByIdOrUseSelectedSession().url.apply { requireContext().share(this) } + getSessionById()?.let { session -> + session.url.apply { requireContext().share(this) } + } } is QuickActionAction.DownloadsPressed -> { requireComponents.analytics.metrics.track(Event.QuickActionSheetDownloadTapped) @@ -362,28 +358,29 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { } is QuickActionAction.BookmarkPressed -> { requireComponents.analytics.metrics.track(Event.QuickActionSheetBookmarkTapped) - val session = getSessionByIdOrUseSelectedSession() - CoroutineScope(IO).launch { - val guid = requireComponents.core.bookmarksStorage - .addItem(BookmarkRoot.Mobile.id, session.url, session.title, null) - launch(Main) { - requireComponents.analytics.metrics.track(Event.AddBookmark) - view?.let { - FenixSnackbar.make( - it, - Snackbar.LENGTH_LONG - ) - .setAction(getString(R.string.edit_bookmark_snackbar_action)) { - Navigation.findNavController(requireActivity(), R.id.container) - .navigate( - BrowserFragmentDirections - .actionBrowserFragmentToBookmarkEditFragment( - guid - ) - ) - } - .setText(getString(R.string.bookmark_saved_snackbar)) - .show() + getSessionById()?.let { session -> + CoroutineScope(IO).launch { + val guid = requireComponents.core.bookmarksStorage + .addItem(BookmarkRoot.Mobile.id, session.url, session.title, null) + launch(Main) { + requireComponents.analytics.metrics.track(Event.AddBookmark) + view?.let { + FenixSnackbar.make( + it, + Snackbar.LENGTH_LONG + ) + .setAction(getString(R.string.edit_bookmark_snackbar_action)) { + Navigation.findNavController(requireActivity(), R.id.container) + .navigate( + BrowserFragmentDirections + .actionBrowserFragmentToBookmarkEditFragment( + guid + ) + ) + } + .setText(getString(R.string.bookmark_saved_snackbar)) + .show() + } } } } @@ -474,7 +471,10 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { ToolbarMenu.Item.Library -> Navigation.findNavController(toolbarComponent.getView()) .navigate(BrowserFragmentDirections.actionBrowserFragmentToLibraryFragment()) is ToolbarMenu.Item.RequestDesktop -> sessionUseCases.requestDesktopSite.invoke(action.item.isChecked) - ToolbarMenu.Item.Share -> getSessionByIdOrUseSelectedSession().url.apply { requireContext().share(this) } + ToolbarMenu.Item.Share -> getSessionById()?.let { session -> + session.url.apply { requireContext().share(this) + } + } ToolbarMenu.Item.NewPrivateTab -> { val directions = BrowserFragmentDirections .actionBrowserFragmentToSearchFragment(null) @@ -485,9 +485,11 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { FindInPageIntegration.launch?.invoke() requireComponents.analytics.metrics.track(Event.FindInPageOpened) } - ToolbarMenu.Item.ReportIssue -> getSessionByIdOrUseSelectedSession().url.apply { - val reportUrl = String.format(REPORT_SITE_ISSUE_URL, this) - sessionUseCases.loadUrl.invoke(reportUrl) + ToolbarMenu.Item.ReportIssue -> getSessionById()?.let { session -> + session.url.apply { + val reportUrl = String.format(REPORT_SITE_ISSUE_URL, this) + sessionUseCases.loadUrl.invoke(reportUrl) + } } ToolbarMenu.Item.Help -> { // TODO Help #1016 @@ -501,9 +503,8 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { } ToolbarMenu.Item.OpenInFenix -> { val intent = Intent(context, IntentReceiverActivity::class.java) - val session = context!!.components.core.sessionManager.findSessionById(sessionId!!) intent.action = Intent.ACTION_VIEW - intent.data = Uri.parse(session?.url) + intent.data = Uri.parse(getSessionById()?.url) startActivity(intent) } } @@ -520,8 +521,8 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { } private fun showQuickSettingsDialog() { - val session = getSessionByIdOrUseSelectedSession() - val host = requireNotNull(session.url.toUri().host) + val session = requireNotNull(getSessionById()) + val host = requireNotNull(session.url.toUri()?.host) launch { val storage = requireContext().components.storage @@ -543,11 +544,11 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { } } - private fun getSessionByIdOrUseSelectedSession(): Session { - return if (sessionId != null) { - requireNotNull(requireContext().components.core.sessionManager.findSessionById(requireNotNull(sessionId))) + private fun getSessionById(): Session? { + return if (externalSessionId != null) { + requireNotNull(requireContext().components.core.sessionManager.findSessionById(externalSessionId!!)) } else { - requireContext().components.core.sessionManager.selectedSessionOrThrow + requireComponents.core.sessionManager.selectedSession } } @@ -568,9 +569,8 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { } private fun setToolbarBehavior(loading: Boolean) { - if (sessionId?.let { sessionId -> - context?.components?.core?.sessionManager?.findSessionById(sessionId)?.isCustomTabSession() == true - } == true) return + if (getSessionById()?.isCustomTabSession() == true) { return } + val toolbarView = toolbarComponent.uiView.view (toolbarView.layoutParams as CoordinatorLayout.LayoutParams).apply { // Stop toolbar from collapsing if TalkBack is enabled or page is loading @@ -581,7 +581,7 @@ class BrowserFragment : Fragment(), BackHandler, CoroutineScope { behavior = BrowserToolbarBottomBehavior(context, null) } else { (behavior as? BrowserToolbarBottomBehavior)?.forceExpand(toolbarView) - behavior = null + null } } } 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 860eb97ba..1f2c1c5f7 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -208,8 +208,7 @@ class HomeFragment : Fragment(), CoroutineScope { is TabAction.Select -> { val session = requireComponents.core.sessionManager.findSessionById(action.sessionId) requireComponents.core.sessionManager.select(session!!) - val directions = HomeFragmentDirections.actionHomeFragmentToBrowserFragment(action.sessionId) - Navigation.findNavController(view!!).navigate(directions) + (activity as HomeActivity).openToBrowser(BrowserDirection.FromHome) } is TabAction.Close -> { requireComponents.core.sessionManager.findSessionById(action.sessionId)?.let { session -> @@ -225,12 +224,8 @@ class HomeFragment : Fragment(), CoroutineScope { requireComponents.useCases.tabsUseCases.removeAllTabsOfType.invoke(action.private) } is TabAction.PrivateBrowsingLearnMore -> { - requireComponents.useCases.tabsUseCases.addPrivateTab - .invoke(SupportUtils.getGenericSumoURLForTopic(SupportUtils.SumoTopic.PRIVATE_BROWSING_MYTHS)) - (activity as HomeActivity).openToBrowser( - requireComponents.core.sessionManager.selectedSession?.id, - BrowserDirection.FromHome - ) + (activity as HomeActivity).openToBrowserAndLoad(SupportUtils.getGenericSumoURLForTopic + (SupportUtils.SumoTopic.PRIVATE_BROWSING_MYTHS), from = BrowserDirection.FromHome) } is TabAction.Add -> { val directions = HomeFragmentDirections.actionHomeFragmentToSearchFragment(null) diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt index ce484f94d..eaaae461d 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt @@ -187,7 +187,7 @@ class BookmarkFragment : Fragment(), CoroutineScope, BackHandler, AccountObserve (activity as HomeActivity).browsingModeManager.mode = BrowsingModeManager.Mode.Normal (activity as HomeActivity).openToBrowserAndLoad( - text = url, + searchTermOrURL = url, from = BrowserDirection.FromBookmarks ) requireComponents.analytics.metrics.track(Event.OpenedBookmarkInNewTab) @@ -198,7 +198,7 @@ class BookmarkFragment : Fragment(), CoroutineScope, BackHandler, AccountObserve (activity as HomeActivity).browsingModeManager.mode = BrowsingModeManager.Mode.Private (activity as HomeActivity).openToBrowserAndLoad( - text = url, + searchTermOrURL = url, from = BrowserDirection.FromBookmarks ) requireComponents.analytics.metrics.track(Event.OpenedBookmarkInPrivateTab) @@ -223,7 +223,7 @@ class BookmarkFragment : Fragment(), CoroutineScope, BackHandler, AccountObserve when (it) { is SignInAction.ClickedSignIn -> { requireComponents.services.accountsAuthFeature.beginAuthentication() - (activity as HomeActivity).openToBrowser(null, from = BrowserDirection.FromBookmarks) + (activity as HomeActivity).openToBrowser(BrowserDirection.FromBookmarks) } } } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/selectfolder/SelectBookmarkFolderFragment.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/selectfolder/SelectBookmarkFolderFragment.kt index 167cc3c18..69465fbbc 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/selectfolder/SelectBookmarkFolderFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/selectfolder/SelectBookmarkFolderFragment.kt @@ -80,7 +80,7 @@ class SelectBookmarkFolderFragment : Fragment(), CoroutineScope, AccountObserver is SignInAction.ClickedSignIn -> { requireComponents.services.accountsAuthFeature.beginAuthentication() view?.let { - (activity as HomeActivity).openToBrowser(null, BrowserDirection.FromBookmarksFolderSelect) + (activity as HomeActivity).openToBrowser(BrowserDirection.FromBookmarksFolderSelect) } } } 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 7918d437c..53beb46ef 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt @@ -152,8 +152,7 @@ class SearchFragment : Fragment(), BackHandler { is SearchAction.UrlCommitted -> { if (it.url.isNotBlank()) { (activity as HomeActivity).openToBrowserAndLoad( - it.url, it.session, it.engine, - BrowserDirection.FromSearch + it.url, engine = it.engine, from = BrowserDirection.FromSearch ) val event = if (it.url.isUrl()) { @@ -184,13 +183,13 @@ class SearchFragment : Fragment(), BackHandler { when (it) { is AwesomeBarAction.URLTapped -> { getSessionUseCase(requireContext(), sessionId == null).invoke(it.url) - (activity as HomeActivity).openToBrowser(sessionId, BrowserDirection.FromSearch) + (activity as HomeActivity).openToBrowser(BrowserDirection.FromSearch) requireComponents.analytics.metrics.track(Event.EnteredUrl(false)) } is AwesomeBarAction.SearchTermsTapped -> { getSearchUseCase(requireContext(), sessionId == null) .invoke(it.searchTerms, it.engine) - (activity as HomeActivity).openToBrowser(sessionId, BrowserDirection.FromSearch) + (activity as HomeActivity).openToBrowser(BrowserDirection.FromSearch) val engine = it.engine ?: requireComponents .search.searchEngineManager.getDefaultSearchEngine(requireContext()) diff --git a/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt index c2f5fac91..150764f3f 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt @@ -180,7 +180,7 @@ class SettingsFragment : PreferenceFragmentCompat(), CoroutineScope, AccountObse // We could auto-close this tab once we get to the end of the authentication process? // Via an interceptor, perhaps. view?.let { - (activity as HomeActivity).openToBrowser(null, BrowserDirection.FromSettings) + (activity as HomeActivity).openToBrowser(BrowserDirection.FromSettings) } true } diff --git a/app/src/main/res/navigation/nav_graph.xml b/app/src/main/res/navigation/nav_graph.xml index 2c867b8a3..d6253c7db 100644 --- a/app/src/main/res/navigation/nav_graph.xml +++ b/app/src/main/res/navigation/nav_graph.xml @@ -104,7 +104,7 @@ android:id="@+id/action_browserFragment_to_searchFragment" app:destination="@id/searchFragment" /> - - -