Set a minimum number of sites a search group should contain.

upstream-sync
mcarare 2 years ago committed by mergify[bot]
parent 242e7a1eb5
commit a6abaf7cf9

@ -17,6 +17,7 @@ import org.mozilla.fenix.library.history.History
import org.mozilla.fenix.library.history.HistoryDataSource import org.mozilla.fenix.library.history.HistoryDataSource
import org.mozilla.fenix.library.history.HistoryItemTimeGroup import org.mozilla.fenix.library.history.HistoryItemTimeGroup
import org.mozilla.fenix.perf.runBlockingIncrement import org.mozilla.fenix.perf.runBlockingIncrement
import org.mozilla.fenix.utils.Settings.Companion.SEARCH_GROUP_MINIMUM_SITES
import kotlin.math.abs import kotlin.math.abs
private const val BUFFER_TIME = 15000 /* 15 seconds in ms */ private const val BUFFER_TIME = 15000 /* 15 seconds in ms */
@ -89,6 +90,7 @@ interface PagedHistoryProvider {
class DefaultPagedHistoryProvider( class DefaultPagedHistoryProvider(
private val historyStorage: PlacesHistoryStorage, private val historyStorage: PlacesHistoryStorage,
private val showHistorySearchGroups: Boolean = FeatureFlags.showHistorySearchGroups, private val showHistorySearchGroups: Boolean = FeatureFlags.showHistorySearchGroups,
private val historyImprovementFeatures: Boolean = FeatureFlags.historyImprovementFeatures,
) : PagedHistoryProvider { ) : PagedHistoryProvider {
val urlSet = Array<MutableSet<String>>(HistoryItemTimeGroup.values().size) { mutableSetOf() } val urlSet = Array<MutableSet<String>>(HistoryItemTimeGroup.values().size) { mutableSetOf() }
@ -136,6 +138,7 @@ class DefaultPagedHistoryProvider(
// in the case of a pull to refresh. // in the case of a pull to refresh.
if (historyGroups == null || offset == 0) { if (historyGroups == null || offset == 0) {
historyGroups = historyStorage.getHistoryMetadataSince(Long.MIN_VALUE) historyGroups = historyStorage.getHistoryMetadataSince(Long.MIN_VALUE)
.asSequence()
.sortedByDescending { it.createdAt } .sortedByDescending { it.createdAt }
.filter { it.key.searchTerm != null } .filter { it.key.searchTerm != null }
.groupBy { it.key.searchTerm!! } .groupBy { it.key.searchTerm!! }
@ -146,6 +149,14 @@ class DefaultPagedHistoryProvider(
items = items.map { it.toHistoryDBMetadata() } items = items.map { it.toHistoryDBMetadata() }
) )
} }
.filter {
if (historyImprovementFeatures) {
it.items.size >= SEARCH_GROUP_MINIMUM_SITES
} else {
true
}
}
.toList()
} }
history = getHistoryAndSearchGroups(offset, numberOfItems) history = getHistoryAndSearchGroups(offset, numberOfItems)

@ -17,6 +17,8 @@ import mozilla.components.concept.storage.HistoryHighlightWeights
import mozilla.components.concept.storage.HistoryMetadata import mozilla.components.concept.storage.HistoryMetadata
import mozilla.components.concept.storage.HistoryMetadataStorage import mozilla.components.concept.storage.HistoryMetadataStorage
import mozilla.components.support.base.feature.LifecycleAwareFeature import mozilla.components.support.base.feature.LifecycleAwareFeature
import org.mozilla.fenix.FeatureFlags
import org.mozilla.fenix.FeatureFlags.historyImprovementFeatures
import org.mozilla.fenix.home.HomeFragment import org.mozilla.fenix.home.HomeFragment
import org.mozilla.fenix.home.HomeFragmentAction import org.mozilla.fenix.home.HomeFragmentAction
import org.mozilla.fenix.home.HomeFragmentStore import org.mozilla.fenix.home.HomeFragmentStore
@ -24,6 +26,7 @@ import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGrou
import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight
import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItemInternal.HistoryGroupInternal import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItemInternal.HistoryGroupInternal
import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItemInternal.HistoryHighlightInternal import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItemInternal.HistoryHighlightInternal
import org.mozilla.fenix.utils.Settings.Companion.SEARCH_GROUP_MINIMUM_SITES
import kotlin.math.max import kotlin.math.max
@VisibleForTesting internal const val MAX_RESULTS_TOTAL = 9 @VisibleForTesting internal const val MAX_RESULTS_TOTAL = 9
@ -48,6 +51,7 @@ class RecentVisitsFeature(
private val historyHighlightsStorage: Lazy<PlacesHistoryStorage>, private val historyHighlightsStorage: Lazy<PlacesHistoryStorage>,
private val scope: CoroutineScope, private val scope: CoroutineScope,
private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO, private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO,
private val historyImprovementFeatures: Boolean = FeatureFlags.historyImprovementFeatures,
) : LifecycleAwareFeature { ) : LifecycleAwareFeature {
private var job: Job? = null private var job: Job? = null
@ -208,6 +212,13 @@ class RecentVisitsFeature(
groupItems = it.value groupItems = it.value
) )
} }
.filter {
if (historyImprovementFeatures) {
it.groupItems.size >= SEARCH_GROUP_MINIMUM_SITES
} else {
true
}
}
} }
/** /**

@ -27,6 +27,7 @@ import mozilla.components.support.ktx.android.content.stringPreference
import org.mozilla.fenix.BuildConfig import org.mozilla.fenix.BuildConfig
import org.mozilla.fenix.Config import org.mozilla.fenix.Config
import org.mozilla.fenix.FeatureFlags import org.mozilla.fenix.FeatureFlags
import org.mozilla.fenix.FeatureFlags.historyImprovementFeatures
import org.mozilla.fenix.R import org.mozilla.fenix.R
import org.mozilla.fenix.browser.browsingmode.BrowsingMode import org.mozilla.fenix.browser.browsingmode.BrowsingMode
import org.mozilla.fenix.components.metrics.MozillaProductDetector import org.mozilla.fenix.components.metrics.MozillaProductDetector
@ -76,6 +77,12 @@ class Settings(private val appContext: Context) : PreferencesHolder {
const val ONE_WEEK_MS = 60 * 60 * 24 * 7 * 1000L const val ONE_WEEK_MS = 60 * 60 * 24 * 7 * 1000L
const val ONE_MONTH_MS = (60 * 60 * 24 * 365 * 1000L) / 12 const val ONE_MONTH_MS = (60 * 60 * 24 * 365 * 1000L) / 12
/**
* The minimum number a search groups should contain.
* Filtering is applied depending on the [historyImprovementFeatures] flag value.
*/
const val SEARCH_GROUP_MINIMUM_SITES: Int = 2
private fun Action.toInt() = when (this) { private fun Action.toInt() = when (this) {
Action.BLOCKED -> BLOCKED_INT Action.BLOCKED -> BLOCKED_INT
Action.ASK_TO_ALLOW -> ASK_TO_ALLOW_INT Action.ASK_TO_ALLOW -> ASK_TO_ALLOW_INT

@ -30,7 +30,8 @@ class PagedHistoryProviderTest {
fun `getHistory uses getVisitsPaginated`() { fun `getHistory uses getVisitsPaginated`() {
val provider = DefaultPagedHistoryProvider( val provider = DefaultPagedHistoryProvider(
historyStorage = storage, historyStorage = storage,
showHistorySearchGroups = true showHistorySearchGroups = true,
false
) )
val visitInfo1 = VisitInfo( val visitInfo1 = VisitInfo(
@ -147,7 +148,8 @@ class PagedHistoryProviderTest {
fun `history metadata matching lower bound`() { fun `history metadata matching lower bound`() {
val provider = DefaultPagedHistoryProvider( val provider = DefaultPagedHistoryProvider(
historyStorage = storage, historyStorage = storage,
showHistorySearchGroups = true showHistorySearchGroups = true,
false
) )
// Oldest history visit on the page is 15 seconds (buffer time) newer than matching // Oldest history visit on the page is 15 seconds (buffer time) newer than matching
// metadata record. // metadata record.
@ -219,7 +221,8 @@ class PagedHistoryProviderTest {
fun `history metadata matching upper bound`() { fun `history metadata matching upper bound`() {
val provider = DefaultPagedHistoryProvider( val provider = DefaultPagedHistoryProvider(
historyStorage = storage, historyStorage = storage,
showHistorySearchGroups = true showHistorySearchGroups = true,
false
) )
// Newest history visit on the page is 15 seconds (buffer time) older than matching // Newest history visit on the page is 15 seconds (buffer time) older than matching
// metadata record. // metadata record.
@ -291,7 +294,8 @@ class PagedHistoryProviderTest {
fun `redirects are filtered out from history metadata groups`() { fun `redirects are filtered out from history metadata groups`() {
val provider = DefaultPagedHistoryProvider( val provider = DefaultPagedHistoryProvider(
historyStorage = storage, historyStorage = storage,
showHistorySearchGroups = true showHistorySearchGroups = true,
false
) )
val visitInfo1 = VisitInfo( val visitInfo1 = VisitInfo(

@ -384,7 +384,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN a list of history highlights and groups WHEN updateState is called THEN emit RecentHistoryChange`() { fun `GIVEN a list of history highlights and groups WHEN updateState is called THEN emit RecentHistoryChange`() {
val feature = spyk(RecentVisitsFeature(homeStore, mockk(), mockk(), mockk(), mockk())) val feature = spyk(RecentVisitsFeature(homeStore, mockk(), mockk(), mockk(), mockk(), false))
val expected = List<RecentHistoryHighlight>(1) { mockk() } val expected = List<RecentHistoryHighlight>(1) { mockk() }
every { feature.getCombinedHistory(any(), any()) } returns expected every { feature.getCombinedHistory(any(), any()) } returns expected
@ -398,7 +398,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN highlights visits exist in search groups WHEN getCombined is called THEN remove the highlights already in groups`() { fun `GIVEN highlights visits exist in search groups WHEN getCombined is called THEN remove the highlights already in groups`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val visitsFromSearch = getSearchFromHistoryMetadataItems(4) val visitsFromSearch = getSearchFromHistoryMetadataItems(4)
val directVisits = getDirectVisitsHistoryMetadataItems(4) val directVisits = getDirectVisitsHistoryMetadataItems(4)
val directDupeVisits = getSearchFromHistoryMetadataItems(2).map { val directDupeVisits = getSearchFromHistoryMetadataItems(2).map {
@ -422,7 +422,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN fewer than needed highlights and search groups WHEN getCombined is called THEN the result is sorted by date`() { fun `GIVEN fewer than needed highlights and search groups WHEN getCombined is called THEN the result is sorted by date`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val visitsFromSearch = getSearchFromHistoryMetadataItems(4) val visitsFromSearch = getSearchFromHistoryMetadataItems(4)
val directVisits = getDirectVisitsHistoryMetadataItems(4) val directVisits = getDirectVisitsHistoryMetadataItems(4)
val expected = directVisits.reversed().toRecentHistoryHighlights() val expected = directVisits.reversed().toRecentHistoryHighlights()
@ -441,7 +441,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN more highlights are newer than search groups WHEN getCombined is called THEN then return an even split then sorted by date`() { fun `GIVEN more highlights are newer than search groups WHEN getCombined is called THEN then return an even split then sorted by date`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val visitsFromSearch = getSearchFromHistoryMetadataItems(5) val visitsFromSearch = getSearchFromHistoryMetadataItems(5)
val directVisits = getDirectVisitsHistoryMetadataItems(14) val directVisits = getDirectVisitsHistoryMetadataItems(14)
val expected = directVisits.takeLast(5).reversed().toRecentHistoryHighlights() + val expected = directVisits.takeLast(5).reversed().toRecentHistoryHighlights() +
@ -457,7 +457,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN more search groups are newer than highlights WHEN getCombined is called THEN then return an even split then sorted by date`() { fun `GIVEN more search groups are newer than highlights WHEN getCombined is called THEN then return an even split then sorted by date`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val visitsFromSearch = getSearchFromHistoryMetadataItems(14) val visitsFromSearch = getSearchFromHistoryMetadataItems(14)
val directVisits = getDirectVisitsHistoryMetadataItems(5) val directVisits = getDirectVisitsHistoryMetadataItems(5)
val expected = visitsFromSearch.takeLast(4).toIndividualRecentHistoryGroups() + val expected = visitsFromSearch.takeLast(4).toIndividualRecentHistoryGroups() +
@ -473,7 +473,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN all highlights have metadata WHEN getHistoryHighlights is called THEN return a list of highlights with an inferred last access time`() { fun `GIVEN all highlights have metadata WHEN getHistoryHighlights is called THEN return a list of highlights with an inferred last access time`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val visitsFromSearch = getSearchFromHistoryMetadataItems(10) val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val directVisits = getDirectVisitsHistoryMetadataItems(10) val directVisits = getDirectVisitsHistoryMetadataItems(10)
@ -490,7 +490,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN not all highlights have metadata WHEN getHistoryHighlights is called THEN set 0 for the highlights with not found last access time`() { fun `GIVEN not all highlights have metadata WHEN getHistoryHighlights is called THEN set 0 for the highlights with not found last access time`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val visitsFromSearch = getSearchFromHistoryMetadataItems(10) val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val directVisits = getDirectVisitsHistoryMetadataItems(10) val directVisits = getDirectVisitsHistoryMetadataItems(10)
val highlightsWithUnknownAccessTime = directVisits.toHistoryHighlightsInternal().take(5).map { val highlightsWithUnknownAccessTime = directVisits.toHistoryHighlightsInternal().take(5).map {
@ -511,7 +511,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN multiple metadata records for the same highlight WHEN getHistoryHighlights is called THEN set the latest access time from multiple available`() { fun `GIVEN multiple metadata records for the same highlight WHEN getHistoryHighlights is called THEN set the latest access time from multiple available`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val visitsFromSearch = getSearchFromHistoryMetadataItems(10) val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val directVisits = getDirectVisitsHistoryMetadataItems(10) val directVisits = getDirectVisitsHistoryMetadataItems(10)
val newerDirectVisits = directVisits.mapIndexed { index, item -> val newerDirectVisits = directVisits.mapIndexed { index, item ->
@ -533,7 +533,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN multiple metadata entries only for direct accessed pages WHEN getHistorySearchGroups is called THEN return an empty list`() { fun `GIVEN multiple metadata entries only for direct accessed pages WHEN getHistorySearchGroups is called THEN return an empty list`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val directVisits = getDirectVisitsHistoryMetadataItems(10) val directVisits = getDirectVisitsHistoryMetadataItems(10)
val result = feature.getHistorySearchGroups(directVisits) val result = feature.getHistorySearchGroups(directVisits)
@ -543,7 +543,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN multiple metadata entries WHEN getHistorySearchGroups is called THEN group all entries by their search term`() { fun `GIVEN multiple metadata entries WHEN getHistorySearchGroups is called THEN group all entries by their search term`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val visitsFromSearch = getSearchFromHistoryMetadataItems(10) val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val directVisits = getDirectVisitsHistoryMetadataItems(10) val directVisits = getDirectVisitsHistoryMetadataItems(10)
@ -556,7 +556,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN multiple metadata entries for the same url WHEN getHistorySearchGroups is called THEN entries are deduped`() { fun `GIVEN multiple metadata entries for the same url WHEN getHistorySearchGroups is called THEN entries are deduped`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val visitsFromSearch = getSearchFromHistoryMetadataItems(10) val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val newerVisitsFromSearch = visitsFromSearch.map { it.copy(updatedAt = it.updatedAt * 2) } val newerVisitsFromSearch = visitsFromSearch.map { it.copy(updatedAt = it.updatedAt * 2) }
val directVisits = getDirectVisitsHistoryMetadataItems(10) val directVisits = getDirectVisitsHistoryMetadataItems(10)
@ -575,7 +575,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN highlights and search groups WHEN getSortedHistory is called THEN sort descending all items based on the last access time`() { fun `GIVEN highlights and search groups WHEN getSortedHistory is called THEN sort descending all items based on the last access time`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val visitsFromSearch = getSearchFromHistoryMetadataItems(10) val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val directVisits = getDirectVisitsHistoryMetadataItems(10) val directVisits = getDirectVisitsHistoryMetadataItems(10)
val expected = directVisits.reversed().toRecentHistoryHighlights() val expected = directVisits.reversed().toRecentHistoryHighlights()
@ -594,7 +594,7 @@ class RecentVisitsFeatureTest {
@Test @Test
fun `GIVEN highlights don't have a valid title WHEN getSortedHistory is called THEN the url is set as title`() { fun `GIVEN highlights don't have a valid title WHEN getSortedHistory is called THEN the url is set as title`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val visitsFromSearch = getSearchFromHistoryMetadataItems(10) val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val directVisits = getDirectVisitsHistoryMetadataItems(10).mapIndexed { index, item -> val directVisits = getDirectVisitsHistoryMetadataItems(10).mapIndexed { index, item ->
when (index % 3) { when (index % 3) {
@ -646,6 +646,7 @@ class RecentVisitsFeatureTest {
lazy { historyHightlightsStorage }, lazy { historyHightlightsStorage },
CoroutineScope(testDispatcher), CoroutineScope(testDispatcher),
testDispatcher, testDispatcher,
false
) )
assertEquals(emptyList<RecentHistoryGroup>(), homeStore.state.recentHistory) assertEquals(emptyList<RecentHistoryGroup>(), homeStore.state.recentHistory)

Loading…
Cancel
Save