From 24e4452cb52aed9b63bff54b82252a7bece3b0e6 Mon Sep 17 00:00:00 2001 From: Roger Yang Date: Wed, 10 Nov 2021 11:53:22 -0500 Subject: [PATCH] Close #22402: Add top placeholder for home --- .../perf/StartupExcessiveResourceUseTest.kt | 2 +- .../org/mozilla/fenix/home/HomeFragment.kt | 5 +- .../mozilla/fenix/home/HomeScreenViewModel.kt | 5 -- .../fenix/home/TopPlaceholderViewHolder.kt | 24 +++++++ .../sessioncontrol/SessionControlAdapter.kt | 6 ++ .../home/sessioncontrol/SessionControlView.kt | 23 ++----- .../main/res/layout/top_placeholder_item.xml | 7 +++ .../sessioncontrol/SessionControlViewTest.kt | 63 ++++++++++++++----- 8 files changed, 92 insertions(+), 43 deletions(-) create mode 100644 app/src/main/java/org/mozilla/fenix/home/TopPlaceholderViewHolder.kt create mode 100644 app/src/main/res/layout/top_placeholder_item.xml diff --git a/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt b/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt index c39dfd9971..966ad1dd7a 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt @@ -25,7 +25,7 @@ private const val EXPECTED_SUPPRESSION_COUNT = 19 @Suppress("TopLevelPropertyNaming") // it's silly this would have a different naming convention b/c no const private val EXPECTED_RUNBLOCKING_RANGE = 0..1 // CI has +1 counts compared to local runs: increment these together private const val EXPECTED_RECYCLER_VIEW_CONSTRAINT_LAYOUT_CHILDREN = 4 -private const val EXPECTED_NUMBER_OF_INFLATION = 12 +private const val EXPECTED_NUMBER_OF_INFLATION = 13 private val failureMsgStrictMode = getErrorMessage( shortName = "StrictMode suppression", diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt index cbfc7da9e1..fe35fc54fd 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -377,8 +377,7 @@ class HomeFragment : Fragment() { homeFragmentStore, binding.sessionControlRecyclerView, viewLifecycleOwner, - sessionControlInteractor, - homeViewModel + sessionControlInteractor ) updateSessionControlView() @@ -546,8 +545,6 @@ class HomeFragment : Fragment() { if (bundleArgs.getBoolean(FOCUS_ON_ADDRESS_BAR)) { navigateToSearch() } else if (bundleArgs.getLong(FOCUS_ON_COLLECTION, -1) >= 0) { - // No need to scroll to async'd loaded TopSites if we want to scroll to collections. - homeViewModel.shouldScrollToTopSites = false /* Triggered when the user has added a tab to a collection and has tapped * the View action on the [TabsTrayDialogFragment] snackbar.*/ scrollAndAnimateCollection(bundleArgs.getLong(FOCUS_ON_COLLECTION, -1)) diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeScreenViewModel.kt b/app/src/main/java/org/mozilla/fenix/home/HomeScreenViewModel.kt index 8fa03b7d7a..479b54e3fa 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeScreenViewModel.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeScreenViewModel.kt @@ -11,9 +11,4 @@ class HomeScreenViewModel : ViewModel() { * Used to delete a specific session once the home screen is resumed */ var sessionToDelete: String? = null - - /** - * Used to remember if we need to scroll to top of the homeFragment's recycleView (top sites) see #8561 - * */ - var shouldScrollToTopSites: Boolean = true } diff --git a/app/src/main/java/org/mozilla/fenix/home/TopPlaceholderViewHolder.kt b/app/src/main/java/org/mozilla/fenix/home/TopPlaceholderViewHolder.kt new file mode 100644 index 0000000000..1fd76ff277 --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/home/TopPlaceholderViewHolder.kt @@ -0,0 +1,24 @@ +/* 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.home + +import android.view.View +import org.mozilla.fenix.R +import org.mozilla.fenix.utils.view.ViewHolder + +/** + * View holder for a synchronous, unconditional and invisible placeholder. This is to anchor home to + * the top when home is created. + */ +class TopPlaceholderViewHolder( + view: View +) : ViewHolder(view) { + + fun bind() = Unit + + companion object { + const val LAYOUT_ID = R.layout.top_placeholder_item + } +} diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt index 282195a03e..52df6b596f 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt @@ -23,6 +23,7 @@ import org.mozilla.fenix.components.tips.Tip import org.mozilla.fenix.historymetadata.view.HistoryMetadataGroupViewHolder import org.mozilla.fenix.historymetadata.view.HistoryMetadataHeaderViewHolder import org.mozilla.fenix.home.HomeFragmentStore +import org.mozilla.fenix.home.TopPlaceholderViewHolder import org.mozilla.fenix.home.OnboardingState import org.mozilla.fenix.home.recentbookmarks.view.RecentBookmarksViewHolder import org.mozilla.fenix.home.recenttabs.view.RecentTabViewHolder @@ -49,6 +50,7 @@ import org.mozilla.fenix.home.topsites.TopSitePagerViewHolder import mozilla.components.feature.tab.collections.Tab as ComponentTab sealed class AdapterItem(@LayoutRes val viewType: Int) { + object TopPlaceholderItem : AdapterItem(TopPlaceholderViewHolder.LAYOUT_ID) data class TipItem(val tip: Tip) : AdapterItem( ButtonTipViewHolder.LAYOUT_ID ) @@ -255,6 +257,7 @@ class SessionControlAdapter( val view = LayoutInflater.from(parent.context).inflate(viewType, parent, false) return when (viewType) { + TopPlaceholderViewHolder.LAYOUT_ID -> TopPlaceholderViewHolder(view) ButtonTipViewHolder.LAYOUT_ID -> ButtonTipViewHolder(view, interactor) TopSitePagerViewHolder.LAYOUT_ID -> TopSitePagerViewHolder(view, interactor) PrivateBrowsingDescriptionViewHolder.LAYOUT_ID -> PrivateBrowsingDescriptionViewHolder( @@ -350,6 +353,9 @@ class SessionControlAdapter( val tipItem = item as AdapterItem.TipItem holder.bind(tipItem.tip) } + is TopPlaceholderViewHolder -> { + holder.bind() + } is TopSitePagerViewHolder -> { holder.bind((item as AdapterItem.TopSitePager).topSites) } diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlView.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlView.kt index 95a0d007d1..74b57baa34 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlView.kt +++ b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlView.kt @@ -20,7 +20,6 @@ import org.mozilla.fenix.ext.settings import org.mozilla.fenix.historymetadata.HistoryMetadataGroup import org.mozilla.fenix.home.HomeFragmentState import org.mozilla.fenix.home.HomeFragmentStore -import org.mozilla.fenix.home.HomeScreenViewModel import org.mozilla.fenix.home.Mode import org.mozilla.fenix.home.OnboardingState import org.mozilla.fenix.home.recenttabs.RecentTab @@ -46,6 +45,9 @@ internal fun normalModeAdapterItems( val items = mutableListOf() var shouldShowCustomizeHome = false + // Add a synchronous, unconditional and invisible placeholder so home is anchored to the top when created. + items.add(AdapterItem.TopPlaceholderItem) + tip?.let { items.add(AdapterItem.TipItem(it)) } if (showSetAsDefaultBrowserCard) { @@ -182,8 +184,7 @@ class SessionControlView( store: HomeFragmentStore, val containerView: View, viewLifecycleOwner: LifecycleOwner, - internal val interactor: SessionControlInteractor, - private var homeScreenViewModel: HomeScreenViewModel + internal val interactor: SessionControlInteractor ) { val view: RecyclerView = containerView as RecyclerView @@ -222,20 +223,6 @@ class SessionControlView( if (shouldReportMetrics) interactor.reportSessionMetrics(state) - val stateAdapterList = state.toAdapterList() - if (homeScreenViewModel.shouldScrollToTopSites) { - sessionControlAdapter.submitList(stateAdapterList) { - - val loadedTopSites = stateAdapterList.find { adapterItem -> - adapterItem is AdapterItem.TopSitePager && adapterItem.topSites.isNotEmpty() - } - loadedTopSites?.run { - homeScreenViewModel.shouldScrollToTopSites = false - view.scrollToPosition(0) - } - } - } else { - sessionControlAdapter.submitList(stateAdapterList) - } + sessionControlAdapter.submitList(state.toAdapterList()) } } diff --git a/app/src/main/res/layout/top_placeholder_item.xml b/app/src/main/res/layout/top_placeholder_item.xml new file mode 100644 index 0000000000..fc1596dbcb --- /dev/null +++ b/app/src/main/res/layout/top_placeholder_item.xml @@ -0,0 +1,7 @@ + + + diff --git a/app/src/test/java/org/mozilla/fenix/home/sessioncontrol/SessionControlViewTest.kt b/app/src/test/java/org/mozilla/fenix/home/sessioncontrol/SessionControlViewTest.kt index 11e9afd02a..cd68e5dd32 100644 --- a/app/src/test/java/org/mozilla/fenix/home/sessioncontrol/SessionControlViewTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/sessioncontrol/SessionControlViewTest.kt @@ -14,6 +14,7 @@ import mozilla.components.feature.tab.collections.TabCollection import mozilla.components.feature.top.sites.TopSite import mozilla.components.service.pocket.PocketRecommendedStory import mozilla.components.support.test.robolectric.testContext +import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Assert.assertFalse import org.junit.Test @@ -96,8 +97,7 @@ class SessionControlViewTest { mockk(relaxed = true), view, mockk(relaxed = true), - interactor, - mockk(relaxed = true) + interactor ) val recentTabs = listOf(mockk(relaxed = true)) @@ -118,8 +118,7 @@ class SessionControlViewTest { mockk(relaxed = true), view, mockk(relaxed = true), - interactor, - mockk(relaxed = true) + interactor ) val state = HomeFragmentState() @@ -155,8 +154,9 @@ class SessionControlViewTest { pocketArticles ) - assertTrue(results[0] is AdapterItem.RecentBookmarks) - assertTrue(results[1] is AdapterItem.CustomizeHomeButton) + assertTrue(results[0] is AdapterItem.TopPlaceholderItem) + assertTrue(results[1] is AdapterItem.RecentBookmarks) + assertTrue(results[2] is AdapterItem.CustomizeHomeButton) } @Test @@ -182,9 +182,10 @@ class SessionControlViewTest { pocketArticles ) - assertTrue(results[0] is AdapterItem.RecentTabsHeader) - assertTrue(results[1] is AdapterItem.RecentTabItem) - assertTrue(results[2] is AdapterItem.CustomizeHomeButton) + assertTrue(results[0] is AdapterItem.TopPlaceholderItem) + assertTrue(results[1] is AdapterItem.RecentTabsHeader) + assertTrue(results[2] is AdapterItem.RecentTabItem) + assertTrue(results[3] is AdapterItem.CustomizeHomeButton) } @Test @@ -210,9 +211,10 @@ class SessionControlViewTest { pocketArticles ) - assertTrue(results[0] is AdapterItem.HistoryMetadataHeader) - assertTrue(results[1] is AdapterItem.HistoryMetadataGroup) - assertTrue(results[2] is AdapterItem.CustomizeHomeButton) + assertTrue(results[0] is AdapterItem.TopPlaceholderItem) + assertTrue(results[1] is AdapterItem.HistoryMetadataHeader) + assertTrue(results[2] is AdapterItem.HistoryMetadataGroup) + assertTrue(results[3] is AdapterItem.CustomizeHomeButton) } @Test @@ -238,8 +240,9 @@ class SessionControlViewTest { pocketArticles ) - assertTrue(results[0] is AdapterItem.PocketStoriesItem) - assertTrue(results[1] is AdapterItem.CustomizeHomeButton) + assertTrue(results[0] is AdapterItem.TopPlaceholderItem) + assertTrue(results[1] is AdapterItem.PocketStoriesItem) + assertTrue(results[2] is AdapterItem.CustomizeHomeButton) } @Test @@ -264,6 +267,36 @@ class SessionControlViewTest { historyMetadata, pocketArticles ) - assertTrue(results.isEmpty()) + assertEquals(results.size, 1) + assertTrue(results[0] is AdapterItem.TopPlaceholderItem) + } + + @Test + fun `GIVEN all items THEN top placeholder item is always the first item`() { + val collection = mockk { + every { id } returns 123L + } + val topSites = listOf(mockk()) + val collections = listOf(collection) + val expandedCollections = emptySet() + val recentBookmarks = listOf(mockk()) + val recentTabs = listOf(mockk()) + val historyMetadata = listOf(mockk()) + val pocketArticles = listOf(mockk()) + + val results = normalModeAdapterItems( + topSites, + collections, + expandedCollections, + null, + recentBookmarks, + false, + false, + recentTabs, + historyMetadata, + pocketArticles + ) + + assertTrue(results[0] is AdapterItem.TopPlaceholderItem) } }