From ee3384ac526b976cb9641f65e4fefe57c75138e7 Mon Sep 17 00:00:00 2001 From: Sebastian Kaspari Date: Wed, 16 Sep 2020 16:29:01 +0200 Subject: [PATCH 01/14] Issue #14225: Remove task when finishing ExternalAppBrowserActivity. --- .../org/mozilla/fenix/customtabs/CustomTabsIntegration.kt | 2 +- .../mozilla/fenix/customtabs/ExternalAppBrowserActivity.kt | 2 +- .../fenix/customtabs/ExternalAppBrowserActivityTest.kt | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabsIntegration.kt b/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabsIntegration.kt index 452f1b0d6..d34b038dd 100644 --- a/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabsIntegration.kt +++ b/app/src/main/java/org/mozilla/fenix/customtabs/CustomTabsIntegration.kt @@ -89,7 +89,7 @@ class CustomTabsIntegration( menuItemIndex = START_OF_MENU_ITEMS_INDEX, window = activity.window, shareListener = { onItemTapped.invoke(ToolbarMenu.Item.Share) }, - closeListener = { activity.finish() } + closeListener = { activity.finishAndRemoveTask() } ) override fun start() = feature.start() diff --git a/app/src/main/java/org/mozilla/fenix/customtabs/ExternalAppBrowserActivity.kt b/app/src/main/java/org/mozilla/fenix/customtabs/ExternalAppBrowserActivity.kt index 40c64d5bd..f7dc5dbc1 100644 --- a/app/src/main/java/org/mozilla/fenix/customtabs/ExternalAppBrowserActivity.kt +++ b/app/src/main/java/org/mozilla/fenix/customtabs/ExternalAppBrowserActivity.kt @@ -44,7 +44,7 @@ open class ExternalAppBrowserActivity : HomeActivity() { customTabSessionId: String? ): NavDirections? { if (customTabSessionId == null) { - finish() + finishAndRemoveTask() return null } diff --git a/app/src/test/java/org/mozilla/fenix/customtabs/ExternalAppBrowserActivityTest.kt b/app/src/test/java/org/mozilla/fenix/customtabs/ExternalAppBrowserActivityTest.kt index 33393e224..a0b117af8 100644 --- a/app/src/test/java/org/mozilla/fenix/customtabs/ExternalAppBrowserActivityTest.kt +++ b/app/src/test/java/org/mozilla/fenix/customtabs/ExternalAppBrowserActivityTest.kt @@ -58,10 +58,10 @@ class ExternalAppBrowserActivityTest { var directions = activity.getNavDirections(BrowserDirection.FromGlobal, "id") assertNotNull(directions) - verify(exactly = 0) { activity.finish() } + verify(exactly = 0) { activity.finishAndRemoveTask() } directions = activity.getNavDirections(BrowserDirection.FromGlobal, null) assertNull(directions) - verify { activity.finish() } + verify { activity.finishAndRemoveTask() } } } From 1f004aff8c8012f515807f9782900ddfeecb5ae0 Mon Sep 17 00:00:00 2001 From: mcarare Date: Wed, 16 Sep 2020 12:23:45 +0300 Subject: [PATCH 02/14] For #15116: Use safe cast for layout params. --- .../org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ad4ed00bb..6865387c8 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 @@ -210,7 +210,7 @@ class BrowserToolbarView( fun expand() { when (settings.toolbarPosition) { ToolbarPosition.BOTTOM -> { - (view.layoutParams as CoordinatorLayout.LayoutParams).apply { + (view.layoutParams as? CoordinatorLayout.LayoutParams)?.apply { // behavior can be null if the "Scroll to hide toolbar" setting is toggled off. (behavior as? BrowserToolbarBottomBehavior)?.forceExpand(view) } From c5a2e2e5a0be93a91df229e3f3f645255d2c21ff Mon Sep 17 00:00:00 2001 From: mcarare Date: Wed, 16 Sep 2020 12:25:13 +0300 Subject: [PATCH 03/14] For #15116: Do not expand toolbar on PWA tabs. --- .../fenix/components/toolbar/BrowserToolbarView.kt | 8 ++++++++ 1 file changed, 8 insertions(+) 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 6865387c8..ea5d8732d 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 @@ -23,6 +23,7 @@ import kotlinx.android.synthetic.main.component_browser_top_toolbar.* import kotlinx.android.synthetic.main.component_browser_top_toolbar.view.* import mozilla.components.browser.domains.autocomplete.ShippedDomainsProvider import mozilla.components.browser.session.Session +import mozilla.components.browser.state.state.ExternalAppType import mozilla.components.browser.toolbar.BrowserToolbar import mozilla.components.browser.toolbar.behavior.BrowserToolbarBottomBehavior import mozilla.components.browser.toolbar.display.DisplayToolbar @@ -77,6 +78,9 @@ class BrowserToolbarView( val toolbarIntegration: ToolbarIntegration + private val isPwaTab: Boolean + get() = customTabSession?.customTabConfig?.externalAppType == ExternalAppType.PROGRESSIVE_WEB_APP + init { val isCustomTabSession = customTabSession != null @@ -208,6 +212,10 @@ class BrowserToolbarView( } fun expand() { + // expand only for normal tabs and custom tabs not for PWA + if (isPwaTab) { + return + } when (settings.toolbarPosition) { ToolbarPosition.BOTTOM -> { (view.layoutParams as? CoordinatorLayout.LayoutParams)?.apply { From 83478b9db4b3b9b1767530901761cc810ee0e42c Mon Sep 17 00:00:00 2001 From: mcarare Date: Wed, 16 Sep 2020 12:25:50 +0300 Subject: [PATCH 04/14] For #15116: Do not set bottom toolbar behavior on PWA tabs. --- .../org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ea5d8732d..89532e2a0 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 @@ -237,7 +237,7 @@ class BrowserToolbarView( fun setScrollFlags(shouldDisableScroll: Boolean = false) { when (settings.toolbarPosition) { ToolbarPosition.BOTTOM -> { - if (settings.isDynamicToolbarEnabled) { + if (settings.isDynamicToolbarEnabled && !isPwaTab) { (view.layoutParams as? CoordinatorLayout.LayoutParams)?.apply { behavior = BrowserToolbarBottomBehavior(view.context, null) } From e5a93116135ba5fb3589a7c1a9bf68ab2c2d5af9 Mon Sep 17 00:00:00 2001 From: mcarare Date: Thu, 17 Sep 2020 13:54:28 +0300 Subject: [PATCH 05/14] For #15116: Also do not expand or set bottom toolbar behavior on TWA tabs. --- .../fenix/components/toolbar/BrowserToolbarView.kt | 11 ++++++----- 1 file changed, 6 insertions(+), 5 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 89532e2a0..45af4648a 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 @@ -78,8 +78,9 @@ class BrowserToolbarView( val toolbarIntegration: ToolbarIntegration - private val isPwaTab: Boolean - get() = customTabSession?.customTabConfig?.externalAppType == ExternalAppType.PROGRESSIVE_WEB_APP + private val isPwaTabOrTwaTab: Boolean + get() = customTabSession?.customTabConfig?.externalAppType == ExternalAppType.PROGRESSIVE_WEB_APP || + customTabSession?.customTabConfig?.externalAppType == ExternalAppType.TRUSTED_WEB_ACTIVITY init { val isCustomTabSession = customTabSession != null @@ -212,8 +213,8 @@ class BrowserToolbarView( } fun expand() { - // expand only for normal tabs and custom tabs not for PWA - if (isPwaTab) { + // expand only for normal tabs and custom tabs not for PWA or TWA + if (isPwaTabOrTwaTab) { return } when (settings.toolbarPosition) { @@ -237,7 +238,7 @@ class BrowserToolbarView( fun setScrollFlags(shouldDisableScroll: Boolean = false) { when (settings.toolbarPosition) { ToolbarPosition.BOTTOM -> { - if (settings.isDynamicToolbarEnabled && !isPwaTab) { + if (settings.isDynamicToolbarEnabled && !isPwaTabOrTwaTab) { (view.layoutParams as? CoordinatorLayout.LayoutParams)?.apply { behavior = BrowserToolbarBottomBehavior(view.context, null) } From 19f0c543fe2fc6d157fe8ed3181ec29d615c4a16 Mon Sep 17 00:00:00 2001 From: Arturo Mejia Date: Fri, 11 Sep 2020 16:18:17 -0400 Subject: [PATCH 06/14] Fix add-ons permissions breaking change --- .../org/mozilla/fenix/addons/AddonPermissionsDetailsView.kt | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/addons/AddonPermissionsDetailsView.kt b/app/src/main/java/org/mozilla/fenix/addons/AddonPermissionsDetailsView.kt index e3f936df6..78f89a6cc 100644 --- a/app/src/main/java/org/mozilla/fenix/addons/AddonPermissionsDetailsView.kt +++ b/app/src/main/java/org/mozilla/fenix/addons/AddonPermissionsDetailsView.kt @@ -6,7 +6,6 @@ package org.mozilla.fenix.addons import android.net.Uri import android.view.View -import androidx.annotation.StringRes import androidx.core.net.toUri import androidx.recyclerview.widget.LinearLayoutManager import kotlinx.android.extensions.LayoutContainer @@ -40,10 +39,7 @@ class AddonPermissionsDetailsView( private fun bindPermissions(addon: Addon) { add_ons_permissions.apply { layoutManager = LinearLayoutManager(context) - val sortedPermissions = addon.translatePermissions().map { - @StringRes val stringId = it - context.getString(stringId) - }.sorted() + val sortedPermissions = addon.translatePermissions(context).sorted() adapter = AddonPermissionsAdapter( sortedPermissions, style = AddonPermissionsAdapter.Style( From d3d24c0f485a1dd2eb479be10c48883494b59812 Mon Sep 17 00:00:00 2001 From: Arturo Mejia Date: Thu, 17 Sep 2020 11:25:11 -0400 Subject: [PATCH 07/14] Update Android Components version to 60.0.20200917130150. --- buildSrc/src/main/java/AndroidComponents.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/buildSrc/src/main/java/AndroidComponents.kt b/buildSrc/src/main/java/AndroidComponents.kt index 2364f0929..8e8ea9494 100644 --- a/buildSrc/src/main/java/AndroidComponents.kt +++ b/buildSrc/src/main/java/AndroidComponents.kt @@ -3,5 +3,5 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ object AndroidComponents { - const val VERSION = "59.0.20200916130055" + const val VERSION = "60.0.20200917130150" } From 1f17371df6d5f3e2c5c9ffd5dd7c52d7e2ab644a Mon Sep 17 00:00:00 2001 From: Sawyer Blatz Date: Wed, 16 Sep 2020 07:14:06 -0700 Subject: [PATCH 08/14] For #14565: Add telemetry for top sites --- app/metrics.yaml | 60 +++++++++++++++++++ .../mozilla/fenix/components/metrics/Event.kt | 14 +++++ .../components/metrics/GleanMetricsService.kt | 14 +++++ .../SessionControlController.kt | 11 ++-- .../SessionControlInteractor.kt | 8 +-- .../viewholders/TopSitePagerViewHolder.kt | 8 +++ .../topsites/TopSiteItemViewHolder.kt | 6 +- .../DefaultSessionControlControllerTest.kt | 5 +- .../topsites/TopSiteItemViewHolderTest.kt | 2 +- docs/metrics.md | 4 ++ 10 files changed, 119 insertions(+), 13 deletions(-) diff --git a/app/metrics.yaml b/app/metrics.yaml index ab3a3cddb..11bd45857 100644 --- a/app/metrics.yaml +++ b/app/metrics.yaml @@ -3195,6 +3195,66 @@ top_sites: notification_emails: - fenix-core@mozilla.com expires: "2020-11-15" + open_frecency: + type: event + description: | + A user opened a frecency top site + bugs: + - https://github.com/mozilla-mobile/fenix/issues/14565 + data_reviews: + - https://github.com/mozilla-mobile/fenix/pull/15136 + data_sensitivity: + - interaction + notification_emails: + - fenix-core@mozilla.com + expires: "2021-03-15" + open_pinned: + type: event + description: | + A user opened a pinned top site + bugs: + - https://github.com/mozilla-mobile/fenix/issues/14565 + data_reviews: + - https://github.com/mozilla-mobile/fenix/pull/15136 + data_sensitivity: + - interaction + notification_emails: + - fenix-core@mozilla.com + expires: "2021-03-15" + swipe_carousel: + type: event + description: | + A user swiped to change the page of the top sites carousel + extra_keys: + page: + description: | + The page number the carousel is now on + bugs: + - https://github.com/mozilla-mobile/fenix/issues/14565 + data_reviews: + - https://github.com/mozilla-mobile/fenix/pull/15136 + data_sensitivity: + - interaction + notification_emails: + - fenix-core@mozilla.com + expires: "2021-03-15" + long_press: + type: event + description: | + A user long pressed on a top site + extra_keys: + type: + description: | + The type of top site. Options are: "FRECENCY," "DEFAULT," or "PINNED." + bugs: + - https://github.com/mozilla-mobile/fenix/issues/14565 + data_reviews: + - https://github.com/mozilla-mobile/fenix/pull/15136 + data_sensitivity: + - interaction + notification_emails: + - fenix-core@mozilla.com + expires: "2021-03-15" open_in_new_tab: type: event description: | diff --git a/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt b/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt index db78aa466..1a3be9a71 100644 --- a/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt +++ b/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt @@ -7,6 +7,7 @@ package org.mozilla.fenix.components.metrics import android.content.Context import mozilla.components.browser.errorpages.ErrorType import mozilla.components.browser.search.SearchEngine +import mozilla.components.feature.top.sites.TopSite import org.mozilla.fenix.GleanMetrics.Addons import org.mozilla.fenix.GleanMetrics.AppTheme import org.mozilla.fenix.GleanMetrics.Autoplay @@ -21,6 +22,7 @@ import org.mozilla.fenix.GleanMetrics.ProgressiveWebApp import org.mozilla.fenix.GleanMetrics.SearchShortcuts import org.mozilla.fenix.GleanMetrics.Tip import org.mozilla.fenix.GleanMetrics.ToolbarSettings +import org.mozilla.fenix.GleanMetrics.TopSites import org.mozilla.fenix.GleanMetrics.TrackingProtection import org.mozilla.fenix.R import java.util.Locale @@ -121,6 +123,8 @@ sealed class Event { object NotificationMediaPlay : Event() object NotificationMediaPause : Event() object TopSiteOpenDefault : Event() + object TopSiteOpenFrecent : Event() + object TopSiteOpenPinned : Event() object TopSiteOpenInNewTab : Event() object TopSiteOpenInPrivateTab : Event() object TopSiteRemoved : Event() @@ -191,6 +195,16 @@ sealed class Event { // Interaction events with extras + data class TopSiteSwipeCarousel(val page: Int) : Event() { + override val extras: Map? + get() = hashMapOf(TopSites.swipeCarouselKeys.page to page.toString()) + } + + data class TopSiteLongPress(val type: TopSite.Type) : Event() { + override val extras: Map? + get() = hashMapOf(TopSites.longPressKeys.type to type.name) + } + data class ProgressiveWebAppForeground(val timeForegrounded: Long) : Event() { override val extras: Map? get() = mapOf(ProgressiveWebApp.foregroundKeys.timeMs to timeForegrounded.toString()) diff --git a/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt b/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt index 219a7fd19..ff71896e1 100644 --- a/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt +++ b/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt @@ -517,6 +517,12 @@ private val Event.wrapper: EventWrapper<*>? is Event.TopSiteOpenDefault -> EventWrapper( { TopSites.openDefault.record(it) } ) + is Event.TopSiteOpenFrecent -> EventWrapper( + { TopSites.openFrecency.record(it) } + ) + is Event.TopSiteOpenPinned -> EventWrapper( + { TopSites.openPinned.record(it) } + ) is Event.TopSiteOpenInNewTab -> EventWrapper( { TopSites.openInNewTab.record(it) } ) @@ -526,6 +532,14 @@ private val Event.wrapper: EventWrapper<*>? is Event.TopSiteRemoved -> EventWrapper( { TopSites.remove.record(it) } ) + is Event.TopSiteLongPress -> EventWrapper( + { TopSites.longPress.record(it) }, + { TopSites.longPressKeys.valueOf(it) } + ) + is Event.TopSiteSwipeCarousel -> EventWrapper( + { TopSites.swipeCarousel.record(it) }, + { TopSites.swipeCarouselKeys.valueOf(it) } + ) is Event.SupportTapped -> EventWrapper( { AboutPage.supportTapped.record(it) } ) diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt index db4ada0e0..3ad615f26 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt +++ b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt @@ -98,7 +98,7 @@ interface SessionControlController { /** * @see [TopSiteInteractor.onSelectTopSite] */ - fun handleSelectTopSite(url: String, isDefault: Boolean) + fun handleSelectTopSite(url: String, type: TopSite.Type) /** * @see [OnboardingInteractor.onStartBrowsingClicked] @@ -302,11 +302,14 @@ class DefaultSessionControlController( metrics.track(Event.CollectionRenamePressed) } - override fun handleSelectTopSite(url: String, isDefault: Boolean) { + override fun handleSelectTopSite(url: String, type: TopSite.Type) { metrics.track(Event.TopSiteOpenInNewTab) - if (isDefault) { - metrics.track(Event.TopSiteOpenDefault) + when (type) { + TopSite.Type.DEFAULT -> metrics.track(Event.TopSiteOpenDefault) + TopSite.Type.FRECENT -> metrics.track(Event.TopSiteOpenFrecent) + TopSite.Type.PINNED -> metrics.track(Event.TopSiteOpenPinned) } + if (url == SupportUtils.POCKET_TRENDING_URL) { metrics.track(Event.PocketTopSiteClicked) } diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlInteractor.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlInteractor.kt index b33b97699..b600d0983 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlInteractor.kt +++ b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlInteractor.kt @@ -167,9 +167,9 @@ interface TopSiteInteractor { * Selects the given top site. Called when a user clicks on a top site. * * @param url The URL of the top site. - * @param isDefault Whether or not the top site is a default one. + * @param type The type of the top site. */ - fun onSelectTopSite(url: String, isDefault: Boolean) + fun onSelectTopSite(url: String, type: TopSite.Type) } /** @@ -218,8 +218,8 @@ class SessionControlInteractor( controller.handleRenameCollectionTapped(collection) } - override fun onSelectTopSite(url: String, isDefault: Boolean) { - controller.handleSelectTopSite(url, isDefault) + override fun onSelectTopSite(url: String, type: TopSite.Type) { + controller.handleSelectTopSite(url, type) } override fun onStartBrowsingClicked() { diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/TopSitePagerViewHolder.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/TopSitePagerViewHolder.kt index ab0142fa1..06affb026 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/TopSitePagerViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/TopSitePagerViewHolder.kt @@ -11,6 +11,8 @@ import androidx.viewpager2.widget.ViewPager2 import kotlinx.android.synthetic.main.component_top_sites_pager.view.* import mozilla.components.feature.top.sites.TopSite import org.mozilla.fenix.R +import org.mozilla.fenix.components.metrics.Event +import org.mozilla.fenix.ext.components import org.mozilla.fenix.home.sessioncontrol.TopSiteInteractor import org.mozilla.fenix.home.sessioncontrol.viewholders.topsites.TopSitesPagerAdapter @@ -21,10 +23,16 @@ class TopSitePagerViewHolder( private val topSitesPagerAdapter = TopSitesPagerAdapter(interactor) private val pageIndicator = view.page_indicator + private var currentPage = 0 private val topSitesPageChangeCallback = object : ViewPager2.OnPageChangeCallback() { override fun onPageSelected(position: Int) { + if (currentPage != position) { + pageIndicator.context.components.analytics.metrics.track(Event.TopSiteSwipeCarousel(position)) + } + pageIndicator.setSelection(position) + currentPage = position } } diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSiteItemViewHolder.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSiteItemViewHolder.kt index ff7df347f..0b6fe85f7 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSiteItemViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSiteItemViewHolder.kt @@ -14,10 +14,10 @@ import kotlinx.android.synthetic.main.top_site_item.* import mozilla.components.browser.menu.BrowserMenuBuilder import mozilla.components.browser.menu.item.SimpleBrowserMenuItem import mozilla.components.feature.top.sites.TopSite -import mozilla.components.feature.top.sites.TopSite.Type.DEFAULT import mozilla.components.feature.top.sites.TopSite.Type.FRECENT import mozilla.components.feature.top.sites.TopSite.Type.PINNED import org.mozilla.fenix.R +import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.loadIntoView import org.mozilla.fenix.home.sessioncontrol.TopSiteInteractor @@ -32,10 +32,12 @@ class TopSiteItemViewHolder( init { top_site_item.setOnClickListener { - interactor.onSelectTopSite(topSite.url, topSite.type === DEFAULT) + interactor.onSelectTopSite(topSite.url, topSite.type) } top_site_item.setOnLongClickListener { + it.context.components.analytics.metrics.track(Event.TopSiteLongPress(topSite.type)) + val topSiteMenu = TopSiteItemMenu(view.context, topSite.type != FRECENT) { item -> when (item) { is TopSiteItemMenu.Item.OpenInPrivateTab -> interactor.onOpenInPrivateTabClicked( diff --git a/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt b/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt index d114f5002..6ad511775 100644 --- a/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt @@ -18,6 +18,7 @@ import mozilla.components.browser.session.SessionManager import mozilla.components.concept.engine.Engine import mozilla.components.feature.tab.collections.TabCollection import mozilla.components.feature.tabs.TabsUseCases +import mozilla.components.feature.top.sites.TopSite import mozilla.components.support.test.rule.MainCoroutineRule import org.junit.Before import org.junit.Rule @@ -279,7 +280,7 @@ class DefaultSessionControlControllerTest { fun handleSelectDefaultTopSite() { val topSiteUrl = "mozilla.org" - controller.handleSelectTopSite(topSiteUrl, true) + controller.handleSelectTopSite(topSiteUrl, TopSite.Type.DEFAULT) verify { metrics.track(Event.TopSiteOpenInNewTab) } verify { metrics.track(Event.TopSiteOpenDefault) } verify { @@ -296,7 +297,7 @@ class DefaultSessionControlControllerTest { fun handleSelectNonDefaultTopSite() { val topSiteUrl = "mozilla.org" - controller.handleSelectTopSite(topSiteUrl, false) + controller.handleSelectTopSite(topSiteUrl, TopSite.Type.FRECENT) verify { metrics.track(Event.TopSiteOpenInNewTab) } verify { tabsUseCases.addTab.invoke( diff --git a/app/src/test/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSiteItemViewHolderTest.kt b/app/src/test/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSiteItemViewHolderTest.kt index 321bad7a9..54d1b91f0 100644 --- a/app/src/test/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSiteItemViewHolderTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSiteItemViewHolderTest.kt @@ -42,6 +42,6 @@ class TopSiteItemViewHolderTest { TopSiteItemViewHolder(view, interactor).bind(pocket) view.top_site_item.performClick() - verify { interactor.onSelectTopSite("https://getpocket.com", isDefault = true) } + verify { interactor.onSelectTopSite("https://getpocket.com", TopSite.Type.DEFAULT) } } } diff --git a/docs/metrics.md b/docs/metrics.md index db27d96ac..197643258 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -213,10 +213,14 @@ The following metrics are added to the ping: | tip.displayed |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |The tip was displayed |[1](https://github.com/mozilla-mobile/fenix/pull/9836)|
  • identifier: The identifier of the tip displayed
|2020-11-15 |2 | | tip.pressed |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |The tip's button was pressed |[1](https://github.com/mozilla-mobile/fenix/pull/9836)|
  • identifier: The identifier of the tip the action was taken on
|2020-11-15 |2 | | toolbar_settings.changed_position |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |The user selected a new position for the toolbar |[1](https://github.com/mozilla-mobile/fenix/pull/6608)|
  • position: A string that indicates the new position of the toolbar TOP or BOTTOM
|2020-11-15 |2 | +| top_sites.long_press |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |A user long pressed on a top site |[1](https://github.com/mozilla-mobile/fenix/pull/15136)|
  • type: The type of top site. Options are: "FRECENCY," "DEFAULT," or "PINNED."
|2021-03-15 |2 | | top_sites.open_default |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |A user opened a default top site |[1](https://github.com/mozilla-mobile/fenix/pull/10752)||2020-11-15 |2 | +| top_sites.open_frecency |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |A user opened a frecency top site |[1](https://github.com/mozilla-mobile/fenix/pull/15136)||2021-03-15 |2 | | top_sites.open_in_new_tab |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |A user opens a new tab based on a top site item |[1](https://github.com/mozilla-mobile/fenix/pull/7523)||2020-11-15 |2 | | top_sites.open_in_private_tab |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |A user opens a new private tab based on a top site item |[1](https://github.com/mozilla-mobile/fenix/pull/7523)||2020-11-15 |2 | +| top_sites.open_pinned |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |A user opened a pinned top site |[1](https://github.com/mozilla-mobile/fenix/pull/15136)||2021-03-15 |2 | | top_sites.remove |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |A user removes a top site item |[1](https://github.com/mozilla-mobile/fenix/pull/7523)||2020-11-15 |2 | +| top_sites.swipe_carousel |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |A user swiped to change the page of the top sites carousel |[1](https://github.com/mozilla-mobile/fenix/pull/15136)|
  • page: The page number the carousel is now on
|2021-03-15 |2 | | tracking_protection.etp_setting_changed |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |A user changed their tracking protection level setting to either strict, standard, or custom. |[1](https://github.com/mozilla-mobile/fenix/pull/5414#issuecomment-532847188), [2](https://github.com/mozilla-mobile/fenix/pull/11383)|
  • etp_setting: The new setting for ETP: strict, standard, custom
|2020-11-15 |2 | | tracking_protection.etp_settings |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |A user opened tracking protection settings through settings. |[1](https://github.com/mozilla-mobile/fenix/pull/5414#issuecomment-532847188)||2020-11-15 |2 | | tracking_protection.etp_shield |[event](https://mozilla.github.io/glean/book/user/metrics/event.html) |A user pressed the tracking protection shield icon in toolbar. |[1](https://github.com/mozilla-mobile/fenix/pull/5414#issuecomment-532847188)||2020-11-15 |2 | From 8a81c1ee1d4c65b5f15e872b0ca7072f7f2839dc Mon Sep 17 00:00:00 2001 From: Sawyer Blatz Date: Thu, 17 Sep 2020 11:02:18 -0700 Subject: [PATCH 09/14] For #15123: Fix top sites text color --- app/src/main/res/values/colors.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/main/res/values/colors.xml b/app/src/main/res/values/colors.xml index 58c98a8db..fb96fd10e 100644 --- a/app/src/main/res/values/colors.xml +++ b/app/src/main/res/values/colors.xml @@ -35,7 +35,7 @@ #FF15141A #0015141A @color/photonLightGrey30 - @color/photonLightGrey80 + @color/photonDarkGrey10 #7542E5 #0250BB #E31587 @@ -103,7 +103,7 @@ #F520123A #F515141A @color/photonDarkGrey10 - @color/photonLightGrey90 + @color/photonLightGrey60 #AB71FF #00B3F4 #FF6BBA From 9afe9679d84ae3db3bcac693f117642ec9d4cee2 Mon Sep 17 00:00:00 2001 From: Elise Richards Date: Thu, 17 Sep 2020 16:41:28 -0500 Subject: [PATCH 10/14] For #15079: handle QR permissions when changed in Android settings (#15097) * Define intent data for activity * Search dialog shows permissions for allow and deny camera * Check camera permissions for fxa pairing * Check camera permissions for old search * Tests for pairing sync interactor and controller. * Cleanup * Use bool pref for setting. Use interfaces and default implementations for the sync interactor and controller. * Lint --- .../mozilla/fenix/search/SearchFragment.kt | 127 ++++++++---------- .../searchdialog/SearchDialogFragment.kt | 61 ++++----- .../mozilla/fenix/settings/PairFragment.kt | 69 +--------- .../settings/account/DefaultSyncController.kt | 74 ++++++++++ .../settings/account/DefaultSyncInteractor.kt | 20 +++ .../settings/account/TurnOnSyncFragment.kt | 29 ++++ .../java/org/mozilla/fenix/utils/Settings.kt | 19 ++- app/src/main/res/values/preference_keys.xml | 2 +- .../fenix/search/SearchInteractorTest.kt | 9 ++ .../account/DefaultSyncControllerTest.kt | 40 ++++++ .../account/DefaultSyncInteractorTest.kt | 31 +++++ 11 files changed, 304 insertions(+), 177 deletions(-) create mode 100644 app/src/main/java/org/mozilla/fenix/settings/account/DefaultSyncController.kt create mode 100644 app/src/main/java/org/mozilla/fenix/settings/account/DefaultSyncInteractor.kt create mode 100644 app/src/test/java/org/mozilla/fenix/settings/account/DefaultSyncControllerTest.kt create mode 100644 app/src/test/java/org/mozilla/fenix/settings/account/DefaultSyncInteractorTest.kt 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 0da68c01d..7076dbf70 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt @@ -26,7 +26,6 @@ import androidx.core.widget.NestedScrollView import androidx.fragment.app.Fragment import androidx.navigation.fragment.findNavController import androidx.navigation.fragment.navArgs -import androidx.preference.PreferenceManager import kotlinx.android.synthetic.main.fragment_search.* import kotlinx.android.synthetic.main.fragment_search.view.* import kotlinx.android.synthetic.main.search_suggestions_hint.view.* @@ -51,7 +50,6 @@ import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.searchengine.CustomSearchEngineStore import org.mozilla.fenix.components.searchengine.FenixSearchEngineProvider import org.mozilla.fenix.ext.components -import org.mozilla.fenix.ext.getPreferenceKey import org.mozilla.fenix.ext.hideToolbar import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.ext.settings @@ -67,7 +65,6 @@ class SearchFragment : Fragment(), UserInteractionHandler { private lateinit var toolbarView: ToolbarView private lateinit var awesomeBarView: AwesomeBarView private val qrFeature = ViewBoundFeatureWrapper() - private var permissionDidUpdate = false private lateinit var searchStore: SearchFragmentStore private lateinit var searchInteractor: SearchInteractor @@ -202,62 +199,26 @@ class SearchFragment : Fragment(), UserInteractionHandler { search_scan_button.visibility = if (context?.hasCamera() == true) View.VISIBLE else View.GONE qrFeature.set( - QrFeature( - requireContext(), - fragmentManager = parentFragmentManager, - onNeedToRequestPermissions = { permissions -> - requestPermissions(permissions, REQUEST_CODE_CAMERA_PERMISSIONS) - }, - onScanResult = { result -> - search_scan_button.isChecked = false - activity?.let { - AlertDialog.Builder(it).apply { - val spannable = resources.getSpanned( - R.string.qr_scanner_confirmation_dialog_message, - getString(R.string.app_name) to StyleSpan(BOLD), - result to StyleSpan(ITALIC) - ) - setMessage(spannable) - setNegativeButton(R.string.qr_scanner_dialog_negative) { dialog: DialogInterface, _ -> - requireComponents.analytics.metrics.track(Event.QRScannerNavigationDenied) - dialog.cancel() - resetFocus() - } - setPositiveButton(R.string.qr_scanner_dialog_positive) { dialog: DialogInterface, _ -> - requireComponents.analytics.metrics.track(Event.QRScannerNavigationAllowed) - (activity as HomeActivity) - .openToBrowserAndLoad( - searchTermOrURL = result, - newTab = searchStore.state.tabId == null, - from = BrowserDirection.FromSearch - ) - dialog.dismiss() - resetFocus() - } - create() - }.show() - requireComponents.analytics.metrics.track(Event.QRScannerPromptDisplayed) - } - }), + createQrFeature(), owner = this, view = view ) view.search_scan_button.setOnClickListener { - toolbarView.view.clearFocus() - - val cameraPermissionsDenied = PreferenceManager.getDefaultSharedPreferences(context) - .getBoolean( - getPreferenceKey(R.string.pref_key_camera_permissions), - false - ) - - if (cameraPermissionsDenied) { - searchInteractor.onCameraPermissionsNeeded() - } else { + if (requireContext().settings().shouldShowCameraPermissionPrompt) { requireComponents.analytics.metrics.track(Event.QRScannerOpened) qrFeature.get()?.scan(R.id.container) + } else { + if (requireContext().isPermissionGranted(Manifest.permission.CAMERA)) { + requireComponents.analytics.metrics.track(Event.QRScannerOpened) + qrFeature.get()?.scan(R.id.container) + } else { + searchInteractor.onCameraPermissionsNeeded() + } } + view.hideKeyboard() + search_scan_button.isChecked = false + requireContext().settings().setCameraPermissionNeededState = false } view.search_engines_shortcut_button.setOnClickListener { @@ -322,6 +283,47 @@ class SearchFragment : Fragment(), UserInteractionHandler { startPostponedEnterTransition() } + private fun createQrFeature(): QrFeature { + return QrFeature( + requireContext(), + fragmentManager = parentFragmentManager, + onNeedToRequestPermissions = { permissions -> + requestPermissions(permissions, REQUEST_CODE_CAMERA_PERMISSIONS) + }, + onScanResult = { result -> + search_scan_button.isChecked = false + activity?.let { + AlertDialog.Builder(it).apply { + val spannable = resources.getSpanned( + R.string.qr_scanner_confirmation_dialog_message, + getString(R.string.app_name) to StyleSpan(BOLD), + result to StyleSpan(ITALIC) + ) + setMessage(spannable) + setNegativeButton(R.string.qr_scanner_dialog_negative) { dialog: DialogInterface, _ -> + requireComponents.analytics.metrics.track(Event.QRScannerNavigationDenied) + dialog.cancel() + resetFocus() + } + setPositiveButton(R.string.qr_scanner_dialog_positive) { dialog: DialogInterface, _ -> + requireComponents.analytics.metrics.track(Event.QRScannerNavigationAllowed) + (activity as HomeActivity) + .openToBrowserAndLoad( + searchTermOrURL = result, + newTab = searchStore.state.tabId == null, + from = BrowserDirection.FromSearch + ) + dialog.dismiss() + resetFocus() + } + create() + }.show() + requireComponents.analytics.metrics.track(Event.QRScannerPromptDisplayed) + } + } + ) + } + private fun updateToolbarContentDescription(searchState: SearchFragmentState) { val urlView = toolbarView.view .findViewById(R.id.mozac_browser_toolbar_edit_url_view) @@ -352,16 +354,11 @@ class SearchFragment : Fragment(), UserInteractionHandler { searchStore.dispatch(SearchFragmentAction.UpdateShortcutsAvailability(areShortcutsAvailable)) } - if (!permissionDidUpdate) { - toolbarView.view.edit.focus() - } - updateClipboardSuggestion( searchStore.state, requireComponents.clipboardHandler.url ) - permissionDidUpdate = false hideToolbar() } @@ -423,22 +420,8 @@ class SearchFragment : Fragment(), UserInteractionHandler { when (requestCode) { REQUEST_CODE_CAMERA_PERMISSIONS -> qrFeature.withFeature { it.onPermissionsResult(permissions, grantResults) - - context?.let { context: Context -> - if (context.isPermissionGranted(Manifest.permission.CAMERA)) { - permissionDidUpdate = true - PreferenceManager.getDefaultSharedPreferences(context) - .edit().putBoolean( - getPreferenceKey(R.string.pref_key_camera_permissions), false - ).apply() - } else { - PreferenceManager.getDefaultSharedPreferences(context) - .edit().putBoolean( - getPreferenceKey(R.string.pref_key_camera_permissions), true - ).apply() - resetFocus() - } - } + resetFocus() + requireContext().settings().setCameraPermissionNeededState = false } else -> super.onRequestPermissionsResult(requestCode, permissions, grantResults) } diff --git a/app/src/main/java/org/mozilla/fenix/searchdialog/SearchDialogFragment.kt b/app/src/main/java/org/mozilla/fenix/searchdialog/SearchDialogFragment.kt index ff56f62f2..5080db6d3 100644 --- a/app/src/main/java/org/mozilla/fenix/searchdialog/SearchDialogFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/searchdialog/SearchDialogFragment.kt @@ -29,7 +29,6 @@ import androidx.core.content.ContextCompat import androidx.core.view.isVisible import androidx.navigation.fragment.findNavController import androidx.navigation.fragment.navArgs -import androidx.preference.PreferenceManager import kotlinx.android.synthetic.main.fragment_search_dialog.* import kotlinx.android.synthetic.main.fragment_search_dialog.view.* import kotlinx.android.synthetic.main.search_suggestions_hint.view.* @@ -54,7 +53,6 @@ import org.mozilla.fenix.components.searchengine.CustomSearchEngineStore import org.mozilla.fenix.components.searchengine.FenixSearchEngineProvider import org.mozilla.fenix.components.toolbar.ToolbarPosition import org.mozilla.fenix.ext.components -import org.mozilla.fenix.ext.getPreferenceKey import org.mozilla.fenix.ext.isKeyboardVisible import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.ext.settings @@ -205,28 +203,34 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler { interactor.onSearchShortcutsButtonClicked() } + qrFeature.set( + createQrFeature(), + owner = this, + view = view + ) + qr_scan_button.visibility = if (context?.hasCamera() == true) View.VISIBLE else View.GONE qr_scan_button.setOnClickListener { if (!requireContext().hasCamera()) { return@setOnClickListener } - + view.hideKeyboard() toolbarView.view.clearFocus() - val cameraPermissionsDenied = - PreferenceManager.getDefaultSharedPreferences(context).getBoolean( - getPreferenceKey(R.string.pref_key_camera_permissions), - false - ) - - if (cameraPermissionsDenied) { - interactor.onCameraPermissionsNeeded() - resetFocus() - view.hideKeyboard() - toolbarView.view.requestFocus() - } else { + if (requireContext().settings().shouldShowCameraPermissionPrompt) { requireComponents.analytics.metrics.track(Event.QRScannerOpened) qrFeature.get()?.scan(R.id.search_wrapper) + } else { + if (requireContext().isPermissionGranted(Manifest.permission.CAMERA)) { + requireComponents.analytics.metrics.track(Event.QRScannerOpened) + qrFeature.get()?.scan(R.id.search_wrapper) + } else { + interactor.onCameraPermissionsNeeded() + resetFocus() + view.hideKeyboard() + toolbarView.view.requestFocus() + } } + requireContext().settings().setCameraPermissionNeededState = false } fill_link_from_clipboard.setOnClickListener { @@ -238,12 +242,6 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler { ) } - qrFeature.set( - createQrFeature(), - owner = this, - view = view - ) - val stubListener = ViewStub.OnInflateListener { _, inflated -> inflated.learn_more.setOnClickListener { (activity as HomeActivity) @@ -379,7 +377,8 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler { }.show() requireComponents.analytics.metrics.track(Event.QRScannerPromptDisplayed) } - }) + } + ) } override fun onRequestPermissionsResult( @@ -389,21 +388,9 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler { ) { when (requestCode) { REQUEST_CODE_CAMERA_PERMISSIONS -> qrFeature.withFeature { - context?.let { context: Context -> - it.onPermissionsResult(permissions, grantResults) - if (!context.isPermissionGranted(Manifest.permission.CAMERA)) { - PreferenceManager.getDefaultSharedPreferences(context) - .edit().putBoolean( - getPreferenceKey(R.string.pref_key_camera_permissions), true - ).apply() - resetFocus() - } else { - PreferenceManager.getDefaultSharedPreferences(context) - .edit().putBoolean( - getPreferenceKey(R.string.pref_key_camera_permissions), false - ).apply() - } - } + it.onPermissionsResult(permissions, grantResults) + resetFocus() + requireContext().settings().setCameraPermissionNeededState = false } else -> super.onRequestPermissionsResult(requestCode, permissions, grantResults) } diff --git a/app/src/main/java/org/mozilla/fenix/settings/PairFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/PairFragment.kt index 224efa237..401a4e3b9 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/PairFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/PairFragment.kt @@ -4,28 +4,21 @@ package org.mozilla.fenix.settings -import android.content.DialogInterface -import android.content.Intent import android.content.pm.PackageManager import android.os.Build import android.os.Bundle import android.os.VibrationEffect import android.os.Vibrator -import android.provider.Settings -import android.text.SpannableString import android.view.View -import androidx.appcompat.app.AlertDialog import androidx.core.content.ContextCompat import androidx.core.content.getSystemService import androidx.fragment.app.Fragment import androidx.navigation.fragment.NavHostFragment.findNavController import androidx.navigation.fragment.findNavController -import androidx.preference.PreferenceManager import mozilla.components.feature.qr.QrFeature import mozilla.components.support.base.feature.UserInteractionHandler import mozilla.components.support.base.feature.ViewBoundFeatureWrapper import org.mozilla.fenix.R -import org.mozilla.fenix.ext.getPreferenceKey import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.ext.showToolbar @@ -65,23 +58,14 @@ class PairFragment : Fragment(R.layout.fragment_pair), UserInteractionHandler { false ) }, - scanMessage = R.string.pair_instructions_2), + scanMessage = R.string.pair_instructions_2 + ), owner = this, view = view ) - val cameraPermissionsDenied = PreferenceManager.getDefaultSharedPreferences(context) - .getBoolean( - getPreferenceKey(R.string.pref_key_camera_permissions), - false - ) - qrFeature.withFeature { - if (cameraPermissionsDenied) { - showPermissionsNeededDialog() - } else { - it.scan(R.id.pair_layout) - } + it.scan(R.id.pair_layout) } } @@ -116,57 +100,10 @@ class PairFragment : Fragment(R.layout.fragment_pair), UserInteractionHandler { qrFeature.withFeature { it.onPermissionsResult(permissions, grantResults) } - PreferenceManager.getDefaultSharedPreferences(context) - .edit().putBoolean( - getPreferenceKey(R.string.pref_key_camera_permissions), false - ).apply() } else { - PreferenceManager.getDefaultSharedPreferences(context) - .edit().putBoolean( - getPreferenceKey(R.string.pref_key_camera_permissions), true - ).apply() findNavController().popBackStack(R.id.turnOnSyncFragment, false) } } } } - - /** - * Shows an [AlertDialog] when camera permissions are needed. - * - * In versions above M, [AlertDialog.BUTTON_POSITIVE] takes the user to the app settings. This - * intent only exists in M and above. Below M, [AlertDialog.BUTTON_POSITIVE] routes to a SUMO - * help page to find the app settings. - * - * [AlertDialog.BUTTON_NEGATIVE] dismisses the dialog. - */ - private fun showPermissionsNeededDialog() { - AlertDialog.Builder(requireContext()).apply { - val spannableText = SpannableString( - resources.getString(R.string.camera_permissions_needed_message) - ) - setMessage(spannableText) - setNegativeButton(R.string.camera_permissions_needed_negative_button_text) { - dialog: DialogInterface, _ -> - dialog.cancel() - } - setPositiveButton(R.string.camera_permissions_needed_positive_button_text) { - dialog: DialogInterface, _ -> - val intent: Intent = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { - Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS) - } else { - SupportUtils.createCustomTabIntent( - requireContext(), - SupportUtils.getSumoURLForTopic( - requireContext(), - SupportUtils.SumoTopic.QR_CAMERA_ACCESS - ) - ) - } - dialog.cancel() - startActivity(intent) - } - create() - }.show() - } } diff --git a/app/src/main/java/org/mozilla/fenix/settings/account/DefaultSyncController.kt b/app/src/main/java/org/mozilla/fenix/settings/account/DefaultSyncController.kt new file mode 100644 index 000000000..a44de10ea --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/settings/account/DefaultSyncController.kt @@ -0,0 +1,74 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.settings.account + +import android.content.DialogInterface +import android.content.Intent +import android.net.Uri +import android.os.Build +import android.provider.Settings +import android.text.SpannableString +import androidx.annotation.VisibleForTesting +import androidx.appcompat.app.AlertDialog +import org.mozilla.fenix.HomeActivity +import org.mozilla.fenix.R +import org.mozilla.fenix.settings.SupportUtils + +interface SyncController { + fun handleCameraPermissionsNeeded() +} + +/** + * Controller for handling [DefaultSyncInteractor] requests. + */ +class DefaultSyncController( + private val activity: HomeActivity +) : SyncController { + + /** + * Creates and shows an [AlertDialog] when camera permissions are needed. + * + * In versions above M, [AlertDialog.BUTTON_POSITIVE] takes the user to the app settings. This + * intent only exists in M and above. Below M, [AlertDialog.BUTTON_POSITIVE] routes to a SUMO + * help page to find the app settings. + * + * [AlertDialog.BUTTON_NEGATIVE] dismisses the dialog. + */ + override fun handleCameraPermissionsNeeded() { + val dialog = buildDialog() + dialog.show() + } + + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + fun buildDialog(): AlertDialog.Builder { + return AlertDialog.Builder(activity).apply { + val spannableText = SpannableString( + activity.resources.getString(R.string.camera_permissions_needed_message) + ) + setMessage(spannableText) + setNegativeButton(R.string.camera_permissions_needed_negative_button_text) { dialog: DialogInterface, _ -> + dialog.cancel() + } + setPositiveButton(R.string.camera_permissions_needed_positive_button_text) { dialog: DialogInterface, _ -> + val intent: Intent = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { + Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS) + } else { + SupportUtils.createCustomTabIntent( + activity, + SupportUtils.getSumoURLForTopic( + activity, + SupportUtils.SumoTopic.QR_CAMERA_ACCESS + ) + ) + } + val uri = Uri.fromParts("package", activity.packageName, null) + intent.data = uri + dialog.cancel() + activity.startActivity(intent) + } + create() + } + } +} diff --git a/app/src/main/java/org/mozilla/fenix/settings/account/DefaultSyncInteractor.kt b/app/src/main/java/org/mozilla/fenix/settings/account/DefaultSyncInteractor.kt new file mode 100644 index 000000000..1b92682ed --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/settings/account/DefaultSyncInteractor.kt @@ -0,0 +1,20 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.settings.account + +interface SyncInteractor { + fun onCameraPermissionsNeeded() +} + +/** + * Interactor for [TurnOnSyncFragment]. + * + * @param syncController Handles the interactions + */ +class DefaultSyncInteractor(private val syncController: DefaultSyncController) : SyncInteractor { + override fun onCameraPermissionsNeeded() { + syncController.handleCameraPermissionsNeeded() + } +} diff --git a/app/src/main/java/org/mozilla/fenix/settings/account/TurnOnSyncFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/account/TurnOnSyncFragment.kt index acfa74c2a..7f42aaf1d 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/account/TurnOnSyncFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/account/TurnOnSyncFragment.kt @@ -4,6 +4,7 @@ package org.mozilla.fenix.settings.account +import android.Manifest import android.os.Bundle import android.view.LayoutInflater import android.view.View @@ -18,15 +19,21 @@ import mozilla.components.concept.sync.AccountObserver import mozilla.components.concept.sync.AuthType import mozilla.components.concept.sync.OAuthAccount import mozilla.components.support.ktx.android.content.hasCamera +import mozilla.components.support.ktx.android.content.isPermissionGranted +import mozilla.components.support.ktx.android.view.hideKeyboard +import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.components.FenixSnackbar import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.ext.requireComponents +import org.mozilla.fenix.ext.settings import org.mozilla.fenix.ext.showToolbar class TurnOnSyncFragment : Fragment(), AccountObserver { private val args by navArgs() + private lateinit var interactor: DefaultSyncInteractor + private var shouldLoginJustWithEmail = false private var pairWithEmailStarted = false @@ -35,6 +42,23 @@ class TurnOnSyncFragment : Fragment(), AccountObserver { } private val paringClickListener = View.OnClickListener { + if (requireContext().settings().shouldShowCameraPermissionPrompt) { + requireComponents.analytics.metrics.track(Event.QRScannerOpened) + navigateToPairFragment() + } else { + if (requireContext().isPermissionGranted(Manifest.permission.CAMERA)) { + requireComponents.analytics.metrics.track(Event.QRScannerOpened) + navigateToPairFragment() + } else { + interactor.onCameraPermissionsNeeded() + view?.hideKeyboard() + } + } + view?.hideKeyboard() + requireContext().settings().setCameraPermissionNeededState = false + } + + private fun navigateToPairFragment() { val directions = TurnOnSyncFragmentDirections.actionTurnOnSyncFragmentToPairFragment() requireView().findNavController().navigate(directions) requireComponents.analytics.metrics.track(Event.SyncAuthScanPairing) @@ -89,6 +113,11 @@ class TurnOnSyncFragment : Fragment(), AccountObserver { getString(R.string.sign_in_instructions), HtmlCompat.FROM_HTML_MODE_LEGACY ) + + interactor = DefaultSyncInteractor( + DefaultSyncController(activity = activity as HomeActivity) + ) + return view } diff --git a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt index a5abe4cbc..713976beb 100644 --- a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt +++ b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt @@ -60,7 +60,6 @@ class Settings(private val appContext: Context) : PreferencesHolder { private const val ALLOWED_INT = 2 private const val CFR_COUNT_CONDITION_FOCUS_INSTALLED = 1 private const val CFR_COUNT_CONDITION_FOCUS_NOT_INSTALLED = 3 - private const val MIN_DAYS_SINCE_FEEDBACK_PROMPT = 120 const val ONE_DAY_MS = 60 * 60 * 24 * 1000L const val ONE_WEEK_MS = 60 * 60 * 24 * 7 * 1000L @@ -766,6 +765,24 @@ class Settings(private val appContext: Context) : PreferencesHolder { default = true ) + /** + * Used in [SearchDialogFragment.kt], [SearchFragment.kt] (deprecated), and [PairFragment.kt] + * to see if we need to check for camera permissions before using the QR code scanner. + */ + var shouldShowCameraPermissionPrompt by booleanPreference( + appContext.getPreferenceKey(R.string.pref_key_camera_permissions_needed), + default = true + ) + + /** + * Sets the state of permissions that have been checked, where [false] denotes already checked + * and [true] denotes needing to check. See [shouldShowCameraPermissionPrompt]. + */ + var setCameraPermissionNeededState by booleanPreference( + appContext.getPreferenceKey(R.string.pref_key_camera_permissions_needed), + default = true + ) + var shouldPromptToSaveLogins by booleanPreference( appContext.getPreferenceKey(R.string.pref_key_save_logins), default = true diff --git a/app/src/main/res/values/preference_keys.xml b/app/src/main/res/values/preference_keys.xml index 8139626ed..fd75d44f1 100644 --- a/app/src/main/res/values/preference_keys.xml +++ b/app/src/main/res/values/preference_keys.xml @@ -225,5 +225,5 @@ pref_key_close_tabs_after_one_week pref_key_close_tabs_after_one_month - pref_key_camera_permissions + pref_key_camera_permissions_needed diff --git a/app/src/test/java/org/mozilla/fenix/search/SearchInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/search/SearchInteractorTest.kt index 82b69a425..7a3af7ef0 100644 --- a/app/src/test/java/org/mozilla/fenix/search/SearchInteractorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/search/SearchInteractorTest.kt @@ -106,4 +106,13 @@ class SearchInteractorTest { searchController.handleExistingSessionSelected(session) } } + + @Test + fun onCameraPermissionsNeeded() { + interactor.onCameraPermissionsNeeded() + + verify { + searchController.handleCameraPermissionsNeeded() + } + } } diff --git a/app/src/test/java/org/mozilla/fenix/settings/account/DefaultSyncControllerTest.kt b/app/src/test/java/org/mozilla/fenix/settings/account/DefaultSyncControllerTest.kt new file mode 100644 index 000000000..1d1424d01 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/settings/account/DefaultSyncControllerTest.kt @@ -0,0 +1,40 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + License, v. 2.0. If a copy of the MPL was not distributed with this + file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.settings.account + +import io.mockk.MockKAnnotations +import io.mockk.every +import io.mockk.impl.annotations.MockK +import io.mockk.mockk +import io.mockk.spyk +import io.mockk.verify +import org.junit.Before +import org.junit.Test +import org.mozilla.fenix.HomeActivity +import org.mozilla.fenix.search.AlertDialogBuilder + +class DefaultSyncControllerTest { + + private lateinit var syncController: DefaultSyncController + @MockK(relaxed = true) private lateinit var activity: HomeActivity + + @Before + fun setUp() { + MockKAnnotations.init(this) + syncController = DefaultSyncController(activity) + } + + @Test + fun `show camera permissions needed dialog`() { + val dialogBuilder: AlertDialogBuilder = mockk(relaxed = true) + + val spyController = spyk(syncController) + every { spyController.buildDialog() } returns dialogBuilder + + spyController.handleCameraPermissionsNeeded() + + verify { dialogBuilder.show() } + } +} diff --git a/app/src/test/java/org/mozilla/fenix/settings/account/DefaultSyncInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/settings/account/DefaultSyncInteractorTest.kt new file mode 100644 index 000000000..2469a655a --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/settings/account/DefaultSyncInteractorTest.kt @@ -0,0 +1,31 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + License, v. 2.0. If a copy of the MPL was not distributed with this + file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.settings.account + +import io.mockk.mockk +import io.mockk.verify +import org.junit.Before +import org.junit.Test + +class DefaultSyncInteractorTest { + + private lateinit var syncInteractor: DefaultSyncInteractor + private lateinit var syncController: DefaultSyncController + + @Before + fun setUp() { + syncController = mockk(relaxed = true) + syncInteractor = DefaultSyncInteractor(syncController) + } + + @Test + fun onCameraPermissionsNeeded() { + syncInteractor.onCameraPermissionsNeeded() + + verify { + syncController.handleCameraPermissionsNeeded() + } + } +} From 3059a57747aa081f3ade3992e5a1da7b3f1f6929 Mon Sep 17 00:00:00 2001 From: Tiger Oakes Date: Mon, 17 Aug 2020 09:44:52 -0700 Subject: [PATCH 11/14] Change separator to use its own view holder --- .../fenix/library/LibrarySiteItemView.kt | 8 +---- .../library/bookmarks/BookmarkAdapter.kt | 23 ++++++------- .../BookmarkSeparatorViewHolder.kt | 33 ++++--------------- app/src/main/res/layout/library_separator.xml | 18 ++++++++++ app/src/main/res/layout/library_site_item.xml | 14 -------- 5 files changed, 37 insertions(+), 59 deletions(-) create mode 100644 app/src/main/res/layout/library_separator.xml diff --git a/app/src/main/java/org/mozilla/fenix/library/LibrarySiteItemView.kt b/app/src/main/java/org/mozilla/fenix/library/LibrarySiteItemView.kt index a90fb4231..caf269920 100644 --- a/app/src/main/java/org/mozilla/fenix/library/LibrarySiteItemView.kt +++ b/app/src/main/java/org/mozilla/fenix/library/LibrarySiteItemView.kt @@ -75,13 +75,7 @@ class LibrarySiteItemView @JvmOverloads constructor( * Change visibility of parts of this view based on what type of item is being represented. */ fun displayAs(mode: ItemType) { - favicon.isVisible = mode != ItemType.SEPARATOR - title.isVisible = mode != ItemType.SEPARATOR url.isVisible = mode == ItemType.SITE - overflow_menu.isVisible = mode != ItemType.SEPARATOR - separator.isVisible = mode == ItemType.SEPARATOR - isClickable = mode != ItemType.SEPARATOR - isFocusable = mode != ItemType.SEPARATOR } /** @@ -136,7 +130,7 @@ class LibrarySiteItemView @JvmOverloads constructor( } enum class ItemType { - SITE, FOLDER, SEPARATOR; + SITE, FOLDER; } companion object { diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt index a91bc6277..da3d60526 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt @@ -22,7 +22,7 @@ import org.mozilla.fenix.library.bookmarks.viewholders.BookmarkNodeViewHolder import org.mozilla.fenix.library.bookmarks.viewholders.BookmarkSeparatorViewHolder class BookmarkAdapter(private val emptyView: View, private val interactor: BookmarkViewInteractor) : - RecyclerView.Adapter() { + RecyclerView.Adapter() { private var tree: List = listOf() private var mode: BookmarkFragmentState.Mode = BookmarkFragmentState.Mode.Normal() @@ -78,43 +78,44 @@ class BookmarkAdapter(private val emptyView: View, private val interactor: Bookm override fun getNewListSize(): Int = new.size } - override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): BookmarkNodeViewHolder { + override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): RecyclerView.ViewHolder { val view = LayoutInflater.from(parent.context) .inflate(R.layout.bookmark_list_item, parent, false) as LibrarySiteItemView return when (viewType) { LibrarySiteItemView.ItemType.SITE.ordinal -> BookmarkItemViewHolder(view, interactor) LibrarySiteItemView.ItemType.FOLDER.ordinal -> BookmarkFolderViewHolder(view, interactor) - LibrarySiteItemView.ItemType.SEPARATOR.ordinal -> BookmarkSeparatorViewHolder(view, interactor) + BookmarkSeparatorViewHolder.LAYOUT_ID -> BookmarkSeparatorViewHolder(view) else -> throw IllegalStateException("ViewType $viewType does not match to a ViewHolder") } } override fun getItemViewType(position: Int): Int { return when (tree[position].type) { - BookmarkNodeType.ITEM -> LibrarySiteItemView.ItemType.SITE - BookmarkNodeType.FOLDER -> LibrarySiteItemView.ItemType.FOLDER - BookmarkNodeType.SEPARATOR -> LibrarySiteItemView.ItemType.SEPARATOR + BookmarkNodeType.ITEM -> LibrarySiteItemView.ItemType.SITE.ordinal + BookmarkNodeType.FOLDER -> LibrarySiteItemView.ItemType.FOLDER.ordinal + BookmarkNodeType.SEPARATOR -> BookmarkSeparatorViewHolder.LAYOUT_ID else -> throw IllegalStateException("Item $tree[position] does not match to a ViewType") - }.ordinal + } } override fun getItemCount(): Int = tree.size override fun onBindViewHolder( - holder: BookmarkNodeViewHolder, + holder: RecyclerView.ViewHolder, position: Int, payloads: MutableList ) { if (payloads.isNotEmpty() && payloads[0] is BookmarkPayload) { - holder.bind(tree[position], mode, payloads[0] as BookmarkPayload) + (holder as? BookmarkNodeViewHolder) + ?.bind(tree[position], mode, payloads[0] as BookmarkPayload) } else { super.onBindViewHolder(holder, position, payloads) } } - override fun onBindViewHolder(holder: BookmarkNodeViewHolder, position: Int) { - holder.bind(tree[position], mode) + override fun onBindViewHolder(holder: RecyclerView.ViewHolder, position: Int) { + (holder as? BookmarkNodeViewHolder)?.bind(tree[position], mode) } } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkSeparatorViewHolder.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkSeparatorViewHolder.kt index 2000f7581..3c33f02b8 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkSeparatorViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkSeparatorViewHolder.kt @@ -4,36 +4,15 @@ package org.mozilla.fenix.library.bookmarks.viewholders -import mozilla.components.concept.storage.BookmarkNode -import org.mozilla.fenix.library.LibrarySiteItemView -import org.mozilla.fenix.library.bookmarks.BookmarkFragmentState -import org.mozilla.fenix.library.bookmarks.BookmarkPayload -import org.mozilla.fenix.library.bookmarks.BookmarkViewInteractor +import android.view.View +import androidx.recyclerview.widget.RecyclerView +import org.mozilla.fenix.R /** * Simple view holder for dividers in the bookmarks list. */ -class BookmarkSeparatorViewHolder( - view: LibrarySiteItemView, - interactor: BookmarkViewInteractor -) : BookmarkNodeViewHolder(view, interactor) { - - override var item: BookmarkNode? = null - - override fun bind( - item: BookmarkNode, - mode: BookmarkFragmentState.Mode - ) { - this.item = item - containerView.displayAs(LibrarySiteItemView.ItemType.SEPARATOR) - updateMenu(item.type) - } - - override fun bind( - item: BookmarkNode, - mode: BookmarkFragmentState.Mode, - payload: BookmarkPayload - ) { - bind(item, mode) +class BookmarkSeparatorViewHolder(view: View) : RecyclerView.ViewHolder(view) { + companion object { + const val LAYOUT_ID = R.layout.library_separator } } diff --git a/app/src/main/res/layout/library_separator.xml b/app/src/main/res/layout/library_separator.xml new file mode 100644 index 000000000..245b9ef68 --- /dev/null +++ b/app/src/main/res/layout/library_separator.xml @@ -0,0 +1,18 @@ + + + + + diff --git a/app/src/main/res/layout/library_site_item.xml b/app/src/main/res/layout/library_site_item.xml index 4d8ea73fd..ca050dafe 100644 --- a/app/src/main/res/layout/library_site_item.xml +++ b/app/src/main/res/layout/library_site_item.xml @@ -84,18 +84,4 @@ app:layout_constraintEnd_toEndOf="parent" app:layout_constraintTop_toTopOf="parent"/> - - From b927b688c9688572ffd62f1fd5e26f6216d07887 Mon Sep 17 00:00:00 2001 From: Tiger Oakes Date: Mon, 17 Aug 2020 10:32:04 -0700 Subject: [PATCH 12/14] Merge bookmark item and folder view holders --- .../fenix/library/LibrarySiteItemView.kt | 5 - .../library/bookmarks/BookmarkAdapter.kt | 54 ++++---- .../viewholders/BookmarkFolderViewHolder.kt | 81 ------------ .../viewholders/BookmarkItemViewHolder.kt | 76 ----------- .../viewholders/BookmarkNodeViewHolder.kt | 103 +++++++++++---- .../BookmarkFolderViewHolderTest.kt | 87 ------------- ...rTest.kt => BookmarkNodeViewHolderTest.kt} | 121 ++++++++++++++---- 7 files changed, 207 insertions(+), 320 deletions(-) delete mode 100644 app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkFolderViewHolder.kt delete mode 100644 app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkItemViewHolder.kt delete mode 100644 app/src/test/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkFolderViewHolderTest.kt rename app/src/test/java/org/mozilla/fenix/library/bookmarks/viewholders/{BookmarkItemViewHolderTest.kt => BookmarkNodeViewHolderTest.kt} (51%) diff --git a/app/src/main/java/org/mozilla/fenix/library/LibrarySiteItemView.kt b/app/src/main/java/org/mozilla/fenix/library/LibrarySiteItemView.kt index caf269920..c444c0489 100644 --- a/app/src/main/java/org/mozilla/fenix/library/LibrarySiteItemView.kt +++ b/app/src/main/java/org/mozilla/fenix/library/LibrarySiteItemView.kt @@ -63,8 +63,6 @@ class LibrarySiteItemView @JvmOverloads constructor( val overflowView: ImageButton get() = overflow_menu - private var iconUrl: String? = null - init { LayoutInflater.from(context).inflate(R.layout.library_site_item, this, true) @@ -86,9 +84,6 @@ class LibrarySiteItemView @JvmOverloads constructor( } fun loadFavicon(url: String) { - if (iconUrl == url) return - - iconUrl = url context.components.core.icons.loadIntoView(favicon, url) } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt index da3d60526..2843bc443 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt @@ -14,10 +14,7 @@ import androidx.recyclerview.widget.RecyclerView import mozilla.appservices.places.BookmarkRoot import mozilla.components.concept.storage.BookmarkNode import mozilla.components.concept.storage.BookmarkNodeType -import org.mozilla.fenix.R import org.mozilla.fenix.library.LibrarySiteItemView -import org.mozilla.fenix.library.bookmarks.viewholders.BookmarkFolderViewHolder -import org.mozilla.fenix.library.bookmarks.viewholders.BookmarkItemViewHolder import org.mozilla.fenix.library.bookmarks.viewholders.BookmarkNodeViewHolder import org.mozilla.fenix.library.bookmarks.viewholders.BookmarkSeparatorViewHolder @@ -70,7 +67,8 @@ class BookmarkAdapter(private val emptyView: View, private val interactor: Bookm titleChanged = oldItem.title != newItem.title, urlChanged = oldItem.url != newItem.url, selectedChanged = oldItem in oldMode.selectedItems != newItem in newMode.selectedItems, - modeChanged = oldMode::class != newMode::class + modeChanged = oldMode::class != newMode::class, + iconChanged = oldItem.type != newItem.type || oldItem.url != newItem.url ) } @@ -79,24 +77,20 @@ class BookmarkAdapter(private val emptyView: View, private val interactor: Bookm } override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): RecyclerView.ViewHolder { - val view = LayoutInflater.from(parent.context) - .inflate(R.layout.bookmark_list_item, parent, false) as LibrarySiteItemView + val view = LayoutInflater.from(parent.context).inflate(viewType, parent, false) return when (viewType) { - LibrarySiteItemView.ItemType.SITE.ordinal -> BookmarkItemViewHolder(view, interactor) - LibrarySiteItemView.ItemType.FOLDER.ordinal -> BookmarkFolderViewHolder(view, interactor) - BookmarkSeparatorViewHolder.LAYOUT_ID -> BookmarkSeparatorViewHolder(view) + BookmarkNodeViewHolder.LAYOUT_ID -> + BookmarkNodeViewHolder(view as LibrarySiteItemView, interactor) + BookmarkSeparatorViewHolder.LAYOUT_ID -> + BookmarkSeparatorViewHolder(view) else -> throw IllegalStateException("ViewType $viewType does not match to a ViewHolder") } } - override fun getItemViewType(position: Int): Int { - return when (tree[position].type) { - BookmarkNodeType.ITEM -> LibrarySiteItemView.ItemType.SITE.ordinal - BookmarkNodeType.FOLDER -> LibrarySiteItemView.ItemType.FOLDER.ordinal - BookmarkNodeType.SEPARATOR -> BookmarkSeparatorViewHolder.LAYOUT_ID - else -> throw IllegalStateException("Item $tree[position] does not match to a ViewType") - } + override fun getItemViewType(position: Int) = when (tree[position].type) { + BookmarkNodeType.ITEM, BookmarkNodeType.FOLDER -> BookmarkNodeViewHolder.LAYOUT_ID + BookmarkNodeType.SEPARATOR -> BookmarkSeparatorViewHolder.LAYOUT_ID } override fun getItemCount(): Int = tree.size @@ -106,16 +100,18 @@ class BookmarkAdapter(private val emptyView: View, private val interactor: Bookm position: Int, payloads: MutableList ) { - if (payloads.isNotEmpty() && payloads[0] is BookmarkPayload) { - (holder as? BookmarkNodeViewHolder) - ?.bind(tree[position], mode, payloads[0] as BookmarkPayload) - } else { - super.onBindViewHolder(holder, position, payloads) + (holder as? BookmarkNodeViewHolder)?.apply { + val diffPayload = if (payloads.isNotEmpty() && payloads[0] is BookmarkPayload) { + payloads[0] as BookmarkPayload + } else { + BookmarkPayload() + } + bind(tree[position], mode, diffPayload) } } override fun onBindViewHolder(holder: RecyclerView.ViewHolder, position: Int) { - (holder as? BookmarkNodeViewHolder)?.bind(tree[position], mode) + (holder as? BookmarkNodeViewHolder)?.bind(tree[position], mode, BookmarkPayload()) } } @@ -126,12 +122,22 @@ class BookmarkAdapter(private val emptyView: View, private val interactor: Bookm * @property urlChanged true if there has been a change to [BookmarkNode.url]. * @property selectedChanged true if there has been a change in the BookmarkNode's selected state. * @property modeChanged true if there has been a change in the state's mode type. + * @property iconChanged true if the icon displayed for the node should be changed. */ data class BookmarkPayload( val titleChanged: Boolean, val urlChanged: Boolean, val selectedChanged: Boolean, - val modeChanged: Boolean -) + val modeChanged: Boolean, + val iconChanged: Boolean +) { + constructor() : this( + titleChanged = true, + urlChanged = true, + selectedChanged = true, + modeChanged = true, + iconChanged = true + ) +} fun BookmarkNode.inRoots() = enumValues().any { it.id == guid } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkFolderViewHolder.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkFolderViewHolder.kt deleted file mode 100644 index 9d62e3bcd..000000000 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkFolderViewHolder.kt +++ /dev/null @@ -1,81 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -package org.mozilla.fenix.library.bookmarks.viewholders - -import android.view.View -import androidx.appcompat.content.res.AppCompatResources -import androidx.core.content.ContextCompat -import mozilla.components.concept.storage.BookmarkNode -import org.mozilla.fenix.R -import org.mozilla.fenix.ext.hideAndDisable -import org.mozilla.fenix.ext.showAndEnable -import org.mozilla.fenix.library.LibrarySiteItemView -import org.mozilla.fenix.library.bookmarks.BookmarkFragmentState -import org.mozilla.fenix.library.bookmarks.BookmarkPayload -import org.mozilla.fenix.library.bookmarks.BookmarkViewInteractor -import org.mozilla.fenix.library.bookmarks.inRoots - -/** - * Represents a folder with other bookmarks inside. - */ -class BookmarkFolderViewHolder( - view: LibrarySiteItemView, - interactor: BookmarkViewInteractor -) : BookmarkNodeViewHolder(view, interactor) { - - override var item: BookmarkNode? = null - - init { - containerView.displayAs(LibrarySiteItemView.ItemType.FOLDER) - } - - override fun bind( - item: BookmarkNode, - mode: BookmarkFragmentState.Mode - ) { - bind(item, mode, BookmarkPayload(true, true, true, true)) - } - - override fun bind(item: BookmarkNode, mode: BookmarkFragmentState.Mode, payload: BookmarkPayload) { - this.item = item - - setSelectionListeners(item, mode) - - if (!item.inRoots()) { - updateMenu(item.type) - if (payload.modeChanged) { - if (mode is BookmarkFragmentState.Mode.Selecting) { - containerView.overflowView.hideAndDisable() - } else { - containerView.overflowView.showAndEnable() - } - } - } else { - containerView.overflowView.visibility = View.GONE - } - - if (payload.selectedChanged) { - containerView.changeSelected(item in mode.selectedItems) - } - - containerView.iconView.setImageDrawable( - AppCompatResources.getDrawable( - containerView.context, - R.drawable.ic_folder_icon - )?.apply { - setTint( - ContextCompat.getColor( - containerView.context, - R.color.primary_text_light_theme - ) - ) - } - ) - - if (payload.titleChanged) { - containerView.titleView.text = item.title - } - } -} diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkItemViewHolder.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkItemViewHolder.kt deleted file mode 100644 index 1af60ec26..000000000 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkItemViewHolder.kt +++ /dev/null @@ -1,76 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -package org.mozilla.fenix.library.bookmarks.viewholders - -import androidx.annotation.VisibleForTesting -import mozilla.components.concept.storage.BookmarkNode -import org.mozilla.fenix.ext.hideAndDisable -import org.mozilla.fenix.ext.showAndEnable -import org.mozilla.fenix.library.LibrarySiteItemView -import org.mozilla.fenix.library.bookmarks.BookmarkFragmentState -import org.mozilla.fenix.library.bookmarks.BookmarkPayload -import org.mozilla.fenix.library.bookmarks.BookmarkViewInteractor - -/** - * Represents a bookmarked website in the bookmarks page. - */ -class BookmarkItemViewHolder( - view: LibrarySiteItemView, - interactor: BookmarkViewInteractor -) : BookmarkNodeViewHolder(view, interactor) { - - override var item: BookmarkNode? = null - - init { - containerView.displayAs(LibrarySiteItemView.ItemType.SITE) - } - - override fun bind( - item: BookmarkNode, - mode: BookmarkFragmentState.Mode - ) { - bind(item, mode, BookmarkPayload(true, true, true, true)) - } - - override fun bind(item: BookmarkNode, mode: BookmarkFragmentState.Mode, payload: BookmarkPayload) { - this.item = item - - updateMenu(item.type) - - if (payload.modeChanged) { - if (mode is BookmarkFragmentState.Mode.Selecting) { - containerView.overflowView.hideAndDisable() - } else { - containerView.overflowView.showAndEnable() - } - } - - if (payload.selectedChanged) { - containerView.changeSelected(item in mode.selectedItems) - } - - if (payload.titleChanged) { - containerView.titleView.text = if (item.title.isNullOrBlank()) item.url else item.title - } else if (payload.urlChanged && item.title.isNullOrBlank()) { - containerView.titleView.text = item.url - } - - if (payload.urlChanged) { - containerView.urlView.text = item.url - setColorsAndIcons(item.url) - } - - setSelectionListeners(item, mode) - } - - @VisibleForTesting - internal fun setColorsAndIcons(url: String?) { - if (url != null && url.startsWith("http")) { - containerView.loadFavicon(url) - } else { - containerView.iconView.setImageDrawable(null) - } - } -} diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt index e04f713fa..e7ab7ca5b 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt @@ -4,45 +4,38 @@ package org.mozilla.fenix.library.bookmarks.viewholders +import android.view.View +import androidx.core.content.ContextCompat +import androidx.core.view.isVisible import androidx.recyclerview.widget.RecyclerView import mozilla.components.concept.storage.BookmarkNode import mozilla.components.concept.storage.BookmarkNodeType +import mozilla.components.support.ktx.android.content.getDrawableWithTint +import org.mozilla.fenix.R +import org.mozilla.fenix.ext.components +import org.mozilla.fenix.ext.hideAndDisable +import org.mozilla.fenix.ext.loadIntoView +import org.mozilla.fenix.ext.showAndEnable import org.mozilla.fenix.library.LibrarySiteItemView -import org.mozilla.fenix.library.SelectionHolder import org.mozilla.fenix.library.bookmarks.BookmarkFragmentState import org.mozilla.fenix.library.bookmarks.BookmarkItemMenu import org.mozilla.fenix.library.bookmarks.BookmarkPayload import org.mozilla.fenix.library.bookmarks.BookmarkViewInteractor +import org.mozilla.fenix.library.bookmarks.inRoots import org.mozilla.fenix.utils.Do /** * Base class for bookmark node view holders. */ -abstract class BookmarkNodeViewHolder( - protected val containerView: LibrarySiteItemView, +class BookmarkNodeViewHolder( + private val containerView: LibrarySiteItemView, private val interactor: BookmarkViewInteractor ) : RecyclerView.ViewHolder(containerView) { - abstract var item: BookmarkNode? - private lateinit var menu: BookmarkItemMenu + var item: BookmarkNode? = null + private val menu: BookmarkItemMenu init { - setupMenu() - } - - abstract fun bind(item: BookmarkNode, mode: BookmarkFragmentState.Mode) - - abstract fun bind( - item: BookmarkNode, - mode: BookmarkFragmentState.Mode, - payload: BookmarkPayload - ) - - protected fun setSelectionListeners(item: BookmarkNode, selectionHolder: SelectionHolder) { - containerView.setSelectionInteractor(item, selectionHolder, interactor) - } - - private fun setupMenu() { menu = BookmarkItemMenu(containerView.context) { menuItem -> val item = this.item ?: return@BookmarkItemMenu Do exhaustive when (menuItem) { @@ -58,5 +51,71 @@ abstract class BookmarkNodeViewHolder( containerView.attachMenu(menu.menuController) } - protected fun updateMenu(itemType: BookmarkNodeType) = menu.updateMenu(itemType) + fun bind( + item: BookmarkNode, + mode: BookmarkFragmentState.Mode, + payload: BookmarkPayload + ) { + this.item = item + + containerView.urlView.isVisible = item.type == BookmarkNodeType.ITEM + containerView.setSelectionInteractor(item, mode, interactor) + menu.updateMenu(item.type) + + // Hide menu button if this item is a root folder or is selected + if (item.type == BookmarkNodeType.FOLDER && item.inRoots()) { + containerView.overflowView.visibility = View.GONE + } else if (payload.modeChanged) { + if (mode is BookmarkFragmentState.Mode.Selecting) { + containerView.overflowView.hideAndDisable() + } else { + containerView.overflowView.showAndEnable() + } + } + + if (payload.selectedChanged) { + containerView.changeSelected(item in mode.selectedItems) + } + + val useTitleFallback = item.type == BookmarkNodeType.ITEM && item.title.isNullOrBlank() + if (payload.titleChanged) { + containerView.titleView.text = if (useTitleFallback) item.url else item.title + } else if (payload.urlChanged && useTitleFallback) { + containerView.titleView.text = item.url + } + + if (payload.urlChanged) { + containerView.urlView.text = item.url + } + + if (payload.iconChanged) { + updateIcon(item) + } + } + + private fun updateIcon(item: BookmarkNode) { + val context = containerView.context + val iconView = containerView.iconView + val url = item.url + + when { + // Item is a folder + item.type == BookmarkNodeType.FOLDER -> + iconView.setImageDrawable( + context.getDrawableWithTint( + R.drawable.ic_folder_icon, + ContextCompat.getColor(context, R.color.primary_text_light_theme) + ) + ) + // Item has a http/https URL + url != null && url.startsWith("http") -> + context.components.core.icons.loadIntoView(iconView, url) + else -> + iconView.setImageDrawable(null) + } + } + + companion object { + const val LAYOUT_ID = R.layout.bookmark_list_item + } } diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkFolderViewHolderTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkFolderViewHolderTest.kt deleted file mode 100644 index 64d1c914e..000000000 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkFolderViewHolderTest.kt +++ /dev/null @@ -1,87 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -package org.mozilla.fenix.library.bookmarks.viewholders - -import androidx.appcompat.content.res.AppCompatResources -import io.mockk.MockKAnnotations -import io.mockk.every -import io.mockk.impl.annotations.MockK -import io.mockk.mockk -import io.mockk.mockkStatic -import io.mockk.verify -import mozilla.components.concept.storage.BookmarkNode -import mozilla.components.concept.storage.BookmarkNodeType -import org.junit.Before -import org.junit.Test -import org.mozilla.fenix.ext.hideAndDisable -import org.mozilla.fenix.ext.showAndEnable -import org.mozilla.fenix.library.LibrarySiteItemView -import org.mozilla.fenix.library.bookmarks.BookmarkFragmentInteractor -import org.mozilla.fenix.library.bookmarks.BookmarkFragmentState -import org.mozilla.fenix.library.bookmarks.BookmarkPayload - -class BookmarkFolderViewHolderTest { - - @MockK - private lateinit var interactor: BookmarkFragmentInteractor - @MockK(relaxed = true) - private lateinit var siteItemView: LibrarySiteItemView - private lateinit var holder: BookmarkFolderViewHolder - - private val folder = BookmarkNode( - type = BookmarkNodeType.FOLDER, - guid = "456", - parentGuid = "123", - position = 0, - title = "Folder", - url = null, - children = listOf() - ) - - @Before - fun setup() { - MockKAnnotations.init(this) - - mockkStatic(AppCompatResources::class) - every { AppCompatResources.getDrawable(any(), any()) } returns mockk(relaxed = true) - - holder = BookmarkFolderViewHolder(siteItemView, interactor) - } - - @Test - fun `binds title and selected state`() { - holder.bind(folder, BookmarkFragmentState.Mode.Normal()) - - verify { - siteItemView.titleView.text = folder.title - siteItemView.overflowView.showAndEnable() - siteItemView.changeSelected(false) - } - - holder.bind(folder, BookmarkFragmentState.Mode.Selecting(setOf(folder))) - - verify { - siteItemView.titleView.text = folder.title - siteItemView.overflowView.hideAndDisable() - siteItemView.changeSelected(true) - } - } - - @Test - fun `bind with payload of no changes does not rebind views`() { - holder.bind( - folder, - BookmarkFragmentState.Mode.Normal(), - BookmarkPayload(false, false, false, false) - ) - - verify(inverse = true) { - siteItemView.titleView.text = folder.title - siteItemView.overflowView.showAndEnable() - siteItemView.overflowView.hideAndDisable() - siteItemView.changeSelected(any()) - } - } -} diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkItemViewHolderTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolderTest.kt similarity index 51% rename from app/src/test/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkItemViewHolderTest.kt rename to app/src/test/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolderTest.kt index 0a1cd2f2d..7f0fbb21a 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkItemViewHolderTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolderTest.kt @@ -4,13 +4,23 @@ package org.mozilla.fenix.library.bookmarks.viewholders +import androidx.appcompat.content.res.AppCompatResources +import io.mockk.Called import io.mockk.MockKAnnotations +import io.mockk.every import io.mockk.impl.annotations.MockK +import io.mockk.mockk +import io.mockk.mockkStatic +import io.mockk.unmockkStatic import io.mockk.verify +import mozilla.components.browser.icons.BrowserIcons +import mozilla.components.browser.icons.IconRequest import mozilla.components.concept.storage.BookmarkNode import mozilla.components.concept.storage.BookmarkNodeType +import org.junit.After import org.junit.Before import org.junit.Test +import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.hideAndDisable import org.mozilla.fenix.ext.showAndEnable import org.mozilla.fenix.library.LibrarySiteItemView @@ -18,15 +28,12 @@ import org.mozilla.fenix.library.bookmarks.BookmarkFragmentInteractor import org.mozilla.fenix.library.bookmarks.BookmarkFragmentState import org.mozilla.fenix.library.bookmarks.BookmarkPayload -class BookmarkItemViewHolderTest { +class BookmarkNodeViewHolderTest { - @MockK - private lateinit var interactor: BookmarkFragmentInteractor - - @MockK(relaxed = true) - private lateinit var siteItemView: LibrarySiteItemView - - private lateinit var holder: BookmarkItemViewHolder + @MockK private lateinit var interactor: BookmarkFragmentInteractor + @MockK(relaxed = true) private lateinit var siteItemView: LibrarySiteItemView + @MockK private lateinit var icons: BrowserIcons + private lateinit var holder: BookmarkNodeViewHolder private val item = BookmarkNode( type = BookmarkNodeType.ITEM, @@ -37,17 +44,45 @@ class BookmarkItemViewHolderTest { url = "https://www.mozilla.org", children = listOf() ) + private val folder = BookmarkNode( + type = BookmarkNodeType.FOLDER, + guid = "456", + parentGuid = "123", + position = 0, + title = "Folder", + url = null, + children = listOf() + ) + + private val falsePayload = BookmarkPayload( + titleChanged = false, + urlChanged = false, + selectedChanged = false, + modeChanged = false, + iconChanged = false + ) @Before fun setup() { MockKAnnotations.init(this) - holder = BookmarkItemViewHolder(siteItemView, interactor) + + mockkStatic(AppCompatResources::class) + every { AppCompatResources.getDrawable(any(), any()) } returns mockk(relaxed = true) + every { siteItemView.context.components.core.icons } returns icons + every { icons.loadIntoView(siteItemView.iconView, any()) } returns mockk() + + holder = BookmarkNodeViewHolder(siteItemView, interactor) + } + + @After + fun teardown() { + unmockkStatic(AppCompatResources::class) } @Test fun `binds views for unselected item`() { val mode = BookmarkFragmentState.Mode.Normal() - holder.bind(item, mode) + holder.bind(item, mode, BookmarkPayload()) verify { siteItemView.setSelectionInteractor(item, mode, interactor) @@ -55,14 +90,14 @@ class BookmarkItemViewHolderTest { siteItemView.urlView.text = item.url siteItemView.overflowView.showAndEnable() siteItemView.changeSelected(false) - holder.setColorsAndIcons(item.url) + icons.loadIntoView(siteItemView.iconView, IconRequest(item.url!!)) } } @Test - fun `binds views for selected item`() { + fun `binds views for selected item for item`() { val mode = BookmarkFragmentState.Mode.Selecting(setOf(item)) - holder.bind(item, mode) + holder.bind(item, mode, BookmarkPayload()) verify { siteItemView.setSelectionInteractor(item, mode, interactor) @@ -70,16 +105,15 @@ class BookmarkItemViewHolderTest { siteItemView.urlView.text = item.url siteItemView.overflowView.hideAndDisable() siteItemView.changeSelected(true) - holder.setColorsAndIcons(item.url) } } @Test - fun `bind with payload of no changes does not rebind views`() { + fun `bind with payload of no changes does not rebind views for item`() { holder.bind( item, BookmarkFragmentState.Mode.Normal(), - BookmarkPayload(false, false, false, false) + falsePayload ) verify(inverse = true) { @@ -88,28 +122,28 @@ class BookmarkItemViewHolderTest { siteItemView.overflowView.showAndEnable() siteItemView.overflowView.hideAndDisable() siteItemView.changeSelected(any()) - holder.setColorsAndIcons(item.url) } + verify { siteItemView.iconView wasNot Called } } @Test - fun `binding an item with a null title uses the url as the title`() { + fun `binding an item with a null title uses the url as the title for item`() { val item = item.copy(title = null) - holder.bind(item, BookmarkFragmentState.Mode.Normal()) + holder.bind(item, BookmarkFragmentState.Mode.Normal(), BookmarkPayload()) verify { siteItemView.titleView.text = item.url } } @Test - fun `binding an item with a blank title uses the url as the title`() { + fun `binding an item with a blank title uses the url as the title for item`() { val item = item.copy(title = " ") - holder.bind(item, BookmarkFragmentState.Mode.Normal()) + holder.bind(item, BookmarkFragmentState.Mode.Normal(), BookmarkPayload()) verify { siteItemView.titleView.text = item.url } } @Test - fun `rebinds title if item title is null and the item url has changed`() { + fun `rebinds title if item title is null and the item url has changed for item`() { val item = item.copy(title = null) holder.bind( item, @@ -118,7 +152,8 @@ class BookmarkItemViewHolderTest { titleChanged = false, urlChanged = true, selectedChanged = false, - modeChanged = false + modeChanged = false, + iconChanged = false ) ) @@ -126,7 +161,7 @@ class BookmarkItemViewHolderTest { } @Test - fun `rebinds title if item title is blank and the item url has changed`() { + fun `rebinds title if item title is blank and the item url has changed for item`() { val item = item.copy(title = " ") holder.bind( item, @@ -135,10 +170,46 @@ class BookmarkItemViewHolderTest { titleChanged = false, urlChanged = true, selectedChanged = false, - modeChanged = false + modeChanged = false, + iconChanged = false ) ) verify { siteItemView.titleView.text = item.url } } + + @Test + fun `binds title and selected state for folder`() { + holder.bind(folder, BookmarkFragmentState.Mode.Normal(), BookmarkPayload()) + + verify { + siteItemView.titleView.text = folder.title + siteItemView.overflowView.showAndEnable() + siteItemView.changeSelected(false) + } + + holder.bind(folder, BookmarkFragmentState.Mode.Selecting(setOf(folder)), BookmarkPayload()) + + verify { + siteItemView.titleView.text = folder.title + siteItemView.overflowView.hideAndDisable() + siteItemView.changeSelected(true) + } + } + + @Test + fun `bind with payload of no changes does not rebind views for folder`() { + holder.bind( + folder, + BookmarkFragmentState.Mode.Normal(), + falsePayload + ) + + verify(inverse = true) { + siteItemView.titleView.text = folder.title + siteItemView.overflowView.showAndEnable() + siteItemView.overflowView.hideAndDisable() + siteItemView.changeSelected(any()) + } + } } From 55b5a452d1cf88b1d46e01b432a5a436ae969559 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?So=CC=88ren=20Hentzschel?= Date: Thu, 10 Sep 2020 22:28:34 +0200 Subject: [PATCH 13/14] For #14933 - Fixed private browsing icon color in preferences fragment --- .../java/org/mozilla/fenix/settings/SettingsFragment.kt | 7 +++++++ 1 file changed, 7 insertions(+) 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 33741ff92..b00a82695 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt @@ -27,6 +27,7 @@ import mozilla.components.concept.sync.AccountObserver import mozilla.components.concept.sync.AuthType import mozilla.components.concept.sync.OAuthAccount import mozilla.components.concept.sync.Profile +import mozilla.components.support.ktx.android.content.getColorFromAttr import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.Config import org.mozilla.fenix.FeatureFlags @@ -312,6 +313,8 @@ class SettingsFragment : PreferenceFragmentCompat() { private fun setupPreferences() { val leakKey = getPreferenceKey(R.string.pref_key_leakcanary) val debuggingKey = getPreferenceKey(R.string.pref_key_remote_debugging) + val preferencePrivateBrowsing = + requirePreference(R.string.pref_key_private_browsing) val preferenceExternalDownloadManager = requirePreference(R.string.pref_key_external_download_manager) val preferenceLeakCanary = findPreference(leakKey) @@ -319,6 +322,10 @@ class SettingsFragment : PreferenceFragmentCompat() { val preferenceMakeDefaultBrowser = requirePreference(R.string.pref_key_make_default_browser) + preferencePrivateBrowsing.icon.mutate().apply { + setTint(requireContext().getColorFromAttr(R.attr.primaryText)) + } + if (!Config.channel.isReleased) { preferenceLeakCanary?.setOnPreferenceChangeListener { _, newValue -> val isEnabled = newValue == true From f793c3a5f9d501f919d8cb429a7acac4c64e55e8 Mon Sep 17 00:00:00 2001 From: mcarare Date: Thu, 17 Sep 2020 18:14:25 +0300 Subject: [PATCH 14/14] For #14727: Add content description to resync tabs button. --- app/src/main/res/layout/view_synced_tabs_title.xml | 2 ++ app/src/main/res/values/strings.xml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/app/src/main/res/layout/view_synced_tabs_title.xml b/app/src/main/res/layout/view_synced_tabs_title.xml index 871512830..2c43d678c 100644 --- a/app/src/main/res/layout/view_synced_tabs_title.xml +++ b/app/src/main/res/layout/view_synced_tabs_title.xml @@ -6,6 +6,7 @@ xmlns:app="http://schemas.android.com/apk/res-auto" android:layout_width="match_parent" android:layout_height="wrap_content" + android:importantForAccessibility="no" android:orientation="horizontal"> diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index ee514d5e0..3bc4005cc 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -143,6 +143,8 @@ Install Synced tabs + + Resync Find in page