From 383a70482c4afb54528f1762368e931b1318d227 Mon Sep 17 00:00:00 2001 From: Tiger Oakes Date: Tue, 4 Feb 2020 22:17:23 -0800 Subject: [PATCH] Clean up toolbar menu class --- .../components/toolbar/BrowserToolbarView.kt | 6 - .../components/toolbar/DefaultToolbarMenu.kt | 111 ++++++++---------- .../fenix/components/toolbar/ToolbarMenu.kt | 4 - .../fenix/customtabs/CustomTabToolbarMenu.kt | 31 +++-- 4 files changed, 72 insertions(+), 80 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt index 0eccb3271..69e6a6186 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt @@ -187,12 +187,6 @@ class BrowserToolbarView( DefaultToolbarMenu( context = this, hasAccountProblem = components.backgroundServices.accountManager.accountNeedsReauth(), - requestDesktopStateProvider = { - sessionManager.selectedSession?.desktopMode ?: false - }, - readerModeStateProvider = { - sessionManager.selectedSession?.readerMode ?: false - }, shouldReverseItems = !shouldUseBottomToolbar, onItemTapped = { interactor.onBrowserToolbarMenuItemTapped(it) }, lifecycleOwner = container.context as AppCompatActivity, diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt index 3a016c41f..3d477be19 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt @@ -5,6 +5,7 @@ package org.mozilla.fenix.components.toolbar import android.content.Context +import androidx.annotation.ColorRes import androidx.core.content.ContextCompat.getColor import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.lifecycleScope @@ -29,28 +30,38 @@ import org.mozilla.fenix.ext.components import org.mozilla.fenix.theme.ThemeManager import org.mozilla.fenix.utils.Settings -@Suppress("LargeClass") // While large, most of the class is very simple +/** + * Builds the toolbar object used with the 3-dot menu in the browser fragment. + * @param sessionManager Reference to the session manager that contains all tabs. + * @param hasAccountProblem If true, there was a problem signing into the Firefox account. + * @param shouldReverseItems If true, reverse the menu items. + * @param onItemTapped Called when a menu item is tapped. + * @param lifecycleOwner View lifecycle owner used to determine when to cancel UI jobs. + * @param bookmarksStorage Used to check if a page is bookmarked. + */ +@Suppress("LargeClass") class DefaultToolbarMenu( private val context: Context, - private val hasAccountProblem: Boolean = false, - private val requestDesktopStateProvider: () -> Boolean = { false }, - private val shouldReverseItems: Boolean, + private val sessionManager: SessionManager, + hasAccountProblem: Boolean = false, + shouldReverseItems: Boolean, private val onItemTapped: (ToolbarMenu.Item) -> Unit = {}, private val lifecycleOwner: LifecycleOwner, - private val bookmarksStorage: BookmarksStorage, - readerModeStateProvider: () -> Boolean = { false }, - sessionManager: SessionManager + private val bookmarksStorage: BookmarksStorage ) : ToolbarMenu { private var currentUrlIsBookmarked = false private var isBookmarkedJob: Job? = null + /** Gets the current browser session */ + private val session: Session? get() = sessionManager.selectedSession + override val menuBuilder by lazy { WebExtensionBrowserMenuBuilder( menuItems, endOfMenuAlwaysVisible = true, store = context.components.core.store, - appendExtensionActionAtStart = true + appendExtensionActionAtStart = !shouldReverseItems ) } @@ -58,17 +69,11 @@ class DefaultToolbarMenu( val forward = BrowserMenuItemToolbar.TwoStateButton( primaryImageResource = mozilla.components.ui.icons.R.drawable.mozac_ic_forward, primaryContentDescription = context.getString(R.string.browser_menu_forward), - primaryImageTintResource = ThemeManager.resolveAttribute( - R.attr.primaryText, - context - ), + primaryImageTintResource = primaryTextColor(), isInPrimaryState = { - context.components.core.sessionManager.selectedSession?.canGoForward ?: true + session?.canGoForward ?: true }, - secondaryImageTintResource = ThemeManager.resolveAttribute( - R.attr.disabled, - context - ), + secondaryImageTintResource = ThemeManager.resolveAttribute(R.attr.disabled, context), disableInSecondaryState = true ) { onItemTapped.invoke(ToolbarMenu.Item.Forward) @@ -77,23 +82,16 @@ class DefaultToolbarMenu( val refresh = BrowserMenuItemToolbar.TwoStateButton( primaryImageResource = mozilla.components.ui.icons.R.drawable.mozac_ic_refresh, primaryContentDescription = context.getString(R.string.browser_menu_refresh), - primaryImageTintResource = ThemeManager.resolveAttribute( - R.attr.primaryText, - context - ), + primaryImageTintResource = primaryTextColor(), isInPrimaryState = { - val loading = context.components.core.sessionManager.selectedSession?.loading - loading == false + session?.loading == false }, secondaryImageResource = mozilla.components.ui.icons.R.drawable.mozac_ic_stop, secondaryContentDescription = context.getString(R.string.browser_menu_stop), - secondaryImageTintResource = ThemeManager.resolveAttribute( - R.attr.primaryText, - context - ), + secondaryImageTintResource = primaryTextColor(), disableInSecondaryState = false ) { - if (context.components.core.sessionManager.selectedSession?.loading == true) { + if (session?.loading == true) { onItemTapped.invoke(ToolbarMenu.Item.Stop) } else { onItemTapped.invoke(ToolbarMenu.Item.Reload) @@ -109,24 +107,18 @@ class DefaultToolbarMenu( } ) - registerForIsBookmarkedUpdates(sessionManager) + registerForIsBookmarkedUpdates() val bookmark = BrowserMenuItemToolbar.TwoStateButton( primaryImageResource = R.drawable.ic_bookmark_filled, primaryContentDescription = context.getString(R.string.browser_menu_edit_bookmark), - primaryImageTintResource = ThemeManager.resolveAttribute( - R.attr.primaryText, - context - ), + primaryImageTintResource = primaryTextColor(), // TwoStateButton.isInPrimaryState must be synchronous, and checking bookmark state is // relatively slow. The best we can do here is periodically compute and cache a new "is // bookmarked" state, and use that whenever the menu has been opened. isInPrimaryState = { currentUrlIsBookmarked }, secondaryImageResource = R.drawable.ic_bookmark_outline, secondaryContentDescription = context.getString(R.string.browser_menu_bookmark), - secondaryImageTintResource = ThemeManager.resolveAttribute( - R.attr.primaryText, - context - ), + secondaryImageTintResource = primaryTextColor(), disableInSecondaryState = false ) { if (!currentUrlIsBookmarked) currentUrlIsBookmarked = true @@ -144,18 +136,14 @@ class DefaultToolbarMenu( .shouldDeleteBrowsingDataOnQuit // Predicates that need to be repeatedly called as the session changes - fun shouldShowAddToHomescreen(): Boolean { - return context.components.useCases.webAppUseCases.isPinningSupported() && - context.components.core.sessionManager.selectedSession != null - } - fun shouldShowReaderMode(): Boolean = sessionManager.selectedSession?.readerable ?: false - fun shouldShowOpenInApp(): Boolean = sessionManager.selectedSession?.let { session -> - val appLink = - context.components.useCases.appLinksUseCases.appLinkRedirect + fun shouldShowAddToHomescreen(): Boolean = + session != null && context.components.useCases.webAppUseCases.isPinningSupported() + fun shouldShowReaderMode(): Boolean = session?.readerable ?: false + fun shouldShowOpenInApp(): Boolean = session?.let { session -> + val appLink = context.components.useCases.appLinksUseCases.appLinkRedirect appLink(session.url).hasExternalApp() } ?: false - fun shouldShowReaderAppearance(): Boolean = - sessionManager.selectedSession?.readerMode ?: false + fun shouldShowReaderAppearance(): Boolean = session?.readerMode ?: false val menuItems = listOfNotNull( help, @@ -182,17 +170,17 @@ class DefaultToolbarMenu( } private val addons = BrowserMenuImageText( - context.getString(R.string.browser_menu_addon_manager), - R.drawable.mozac_ic_extensions, - primaryTextColor() + label = context.getString(R.string.browser_menu_addon_manager), + imageResource = R.drawable.mozac_ic_extensions, + iconTintColorResource = primaryTextColor() ) { onItemTapped.invoke(ToolbarMenu.Item.AddonsManager) } private val help = BrowserMenuImageText( - context.getString(R.string.browser_menu_help), - R.drawable.ic_help, - primaryTextColor() + label = context.getString(R.string.browser_menu_help), + imageResource = R.drawable.ic_help, + iconTintColorResource = primaryTextColor() ) { onItemTapped.invoke(ToolbarMenu.Item.Help) } @@ -226,7 +214,9 @@ class DefaultToolbarMenu( private val desktopMode = BrowserMenuImageSwitch( imageResource = R.drawable.ic_desktop, label = context.getString(R.string.browser_menu_desktop_site), - initialState = requestDesktopStateProvider + initialState = { + session?.desktopMode ?: false + } ) { checked -> onItemTapped.invoke(ToolbarMenu.Item.RequestDesktop(checked)) } @@ -306,7 +296,9 @@ class DefaultToolbarMenu( private val readerMode = BrowserMenuHighlightableSwitch( label = context.getString(R.string.browser_menu_read), startImageResource = R.drawable.ic_readermode, - initialState = readerModeStateProvider, + initialState = { + session?.readerMode ?: false + }, highlight = BrowserMenuHighlight.LowPriority( label = context.getString(R.string.browser_menu_read), notificationTint = getColor(context, R.color.whats_new_notification_color) @@ -333,13 +325,14 @@ class DefaultToolbarMenu( notificationTint = getColor(context, R.color.whats_new_notification_color) ), isHighlighted = { true } - ) { + ) { onItemTapped.invoke(ToolbarMenu.Item.OpenInApp) } + @ColorRes private fun primaryTextColor() = ThemeManager.resolveAttribute(R.attr.primaryText, context) - private fun registerForIsBookmarkedUpdates(sessionManager: SessionManager) { + private fun registerForIsBookmarkedUpdates() { val observer = object : Session.Observer { override fun onUrlChanged(session: Session, url: String) { currentUrlIsBookmarked = false @@ -347,8 +340,8 @@ class DefaultToolbarMenu( } } - sessionManager.selectedSession?.url?.let { updateCurrentUrlIsBookmarked(it) } - sessionManager.selectedSession?.register(observer, lifecycleOwner) + session?.url?.let { updateCurrentUrlIsBookmarked(it) } + session?.register(observer, lifecycleOwner) } private fun updateCurrentUrlIsBookmarked(newUrl: String) { diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/ToolbarMenu.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/ToolbarMenu.kt index 66325eebc..362b41d8a 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/ToolbarMenu.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/ToolbarMenu.kt @@ -36,8 +36,4 @@ interface ToolbarMenu { val menuBuilder: BrowserMenuBuilder val menuToolbar: BrowserMenuItemToolbar - - companion object { - const val CAPTION_TEXT_SIZE = 12f - } } diff --git a/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabToolbarMenu.kt b/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabToolbarMenu.kt index 2af3ae449..04cb75641 100644 --- a/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabToolbarMenu.kt +++ b/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabToolbarMenu.kt @@ -6,6 +6,7 @@ package org.mozilla.fenix.customtabs import android.content.Context import android.graphics.Typeface +import androidx.annotation.ColorRes import mozilla.components.browser.menu.BrowserMenuBuilder import mozilla.components.browser.menu.item.BrowserMenuCategory import mozilla.components.browser.menu.item.BrowserMenuDivider @@ -19,6 +20,13 @@ import org.mozilla.fenix.R import org.mozilla.fenix.components.toolbar.ToolbarMenu import org.mozilla.fenix.theme.ThemeManager +/** + * Builds the toolbar object used with the 3-dot menu in the custom tab browser fragment. + * @param sessionManager Reference to the session manager that contains all tabs. + * @param sessionId ID of the open custom tab session. + * @param shouldReverseItems If true, reverse the menu items. + * @param onItemTapped Called when a menu item is tapped. + */ class CustomTabToolbarMenu( private val context: Context, private val sessionManager: SessionManager, @@ -26,10 +34,12 @@ class CustomTabToolbarMenu( private val shouldReverseItems: Boolean, private val onItemTapped: (ToolbarMenu.Item) -> Unit = {} ) : ToolbarMenu { + override val menuBuilder by lazy { BrowserMenuBuilder(menuItems) } - private val session: Session? - get() = sessionId?.let { sessionManager.findSessionById(it) } + /** Gets the current custom tab session */ + private val session: Session? get() = sessionId?.let { sessionManager.findSessionById(it) } + private val appName = context.getString(R.string.app_name) override val menuToolbar by lazy { val back = BrowserMenuItemToolbar.TwoStateButton( @@ -116,24 +126,23 @@ class CustomTabToolbarMenu( } private val openInFenix = SimpleBrowserMenuItem( - label = { - val appName = context.getString(R.string.app_name) - context.getString(R.string.browser_menu_open_in_fenix, appName) - }(), + label = context.getString(R.string.browser_menu_open_in_fenix, appName), textColorResource = primaryTextColor() ) { onItemTapped.invoke(ToolbarMenu.Item.OpenInFenix) } private val poweredBy = BrowserMenuCategory( - label = { - val appName = context.getString(R.string.app_name) - context.getString(R.string.browser_menu_powered_by, appName).toUpperCase() - }(), - textSize = ToolbarMenu.CAPTION_TEXT_SIZE, + label = context.getString(R.string.browser_menu_powered_by, appName).toUpperCase(), + textSize = CAPTION_TEXT_SIZE, textColorResource = primaryTextColor(), textStyle = Typeface.NORMAL ) + @ColorRes private fun primaryTextColor() = ThemeManager.resolveAttribute(R.attr.primaryText, context) + + companion object { + private const val CAPTION_TEXT_SIZE = 12f + } }