diff --git a/app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt b/app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt index 7a06c178b6..23653209d3 100644 --- a/app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt +++ b/app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt @@ -60,6 +60,7 @@ sealed class AppAction : Action { data class RemoveRecentBookmark(val recentBookmark: RecentBookmark) : AppAction() data class RecentHistoryChange(val recentHistory: List) : AppAction() data class RemoveRecentHistoryHighlight(val highlightUrl: String) : AppAction() + data class DisbandSearchGroupAction(val searchTerm: String) : AppAction() /** * Indicates the given [categoryName] was selected by the user. */ diff --git a/app/src/main/java/org/mozilla/fenix/components/appstate/AppStoreReducer.kt b/app/src/main/java/org/mozilla/fenix/components/appstate/AppStoreReducer.kt index 284dcecf4f..e838893676 100644 --- a/app/src/main/java/org/mozilla/fenix/components/appstate/AppStoreReducer.kt +++ b/app/src/main/java/org/mozilla/fenix/components/appstate/AppStoreReducer.kt @@ -4,15 +4,18 @@ package org.mozilla.fenix.components.appstate +import androidx.annotation.VisibleForTesting import mozilla.components.service.pocket.PocketStory.PocketRecommendedStory import mozilla.components.service.pocket.PocketStory.PocketSponsoredStory import mozilla.components.service.pocket.ext.recordNewImpression import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.ext.filterOutTab import org.mozilla.fenix.ext.getFilteredStories +import org.mozilla.fenix.ext.recentSearchGroup import org.mozilla.fenix.gleanplumb.state.MessagingReducer import org.mozilla.fenix.home.pocket.PocketRecommendedStoriesSelectedCategory import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup /** * Reducer for [AppStore]. @@ -87,6 +90,14 @@ internal object AppStoreReducer { it is RecentlyVisitedItem.RecentHistoryHighlight && it.url == action.highlightUrl } ) + is AppAction.DisbandSearchGroupAction -> state.copy( + recentHistory = state.recentHistory.filterNot { + it is RecentHistoryGroup && ( + it.title.equals(action.searchTerm, true) || + it.title.equals(state.recentSearchGroup?.searchTerm, true) + ) + } + ) is AppAction.SelectPocketStoriesCategory -> { val updatedCategoriesState = state.copy( pocketStoriesCategoriesSelections = @@ -194,3 +205,16 @@ internal object AppStoreReducer { ) } } + +/** + * Removes a [RecentHistoryGroup] identified by [groupTitle] if it exists in the current list. + * + * @param groupTitle [RecentHistoryGroup.title] of the item that should be removed. + */ +@VisibleForTesting +internal fun List.filterOut(groupTitle: String?): List { + return when (groupTitle != null) { + true -> filterNot { it is RecentHistoryGroup && it.title.equals(groupTitle, true) } + false -> this + } +} diff --git a/app/src/main/java/org/mozilla/fenix/ext/AppState.kt b/app/src/main/java/org/mozilla/fenix/ext/AppState.kt index b2d2824dc7..9b2270731e 100644 --- a/app/src/main/java/org/mozilla/fenix/ext/AppState.kt +++ b/app/src/main/java/org/mozilla/fenix/ext/AppState.kt @@ -16,6 +16,7 @@ import org.mozilla.fenix.home.pocket.POCKET_STORIES_DEFAULT_CATEGORY_NAME import org.mozilla.fenix.home.pocket.PocketRecommendedStoriesCategory import org.mozilla.fenix.home.pocket.PocketStory import org.mozilla.fenix.home.recentsyncedtabs.RecentSyncedTabState +import org.mozilla.fenix.home.recenttabs.RecentTab.SearchGroup import org.mozilla.fenix.utils.Settings /** @@ -161,6 +162,13 @@ internal fun getFilteredSponsoredStories( .toList() } +/** + * Get the [SearchGroup] shown in the "Jump back in" section. + * May be null if no search group is shown. + */ +internal val AppState.recentSearchGroup: SearchGroup? + get() = recentTabs.find { it is SearchGroup } as SearchGroup? + /** * Filter a [AppState] by the blocklist. * diff --git a/app/src/main/java/org/mozilla/fenix/home/recenttabs/RecentTabsListFeature.kt b/app/src/main/java/org/mozilla/fenix/home/recenttabs/RecentTabsListFeature.kt index 956331c44b..2596f84b0b 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recenttabs/RecentTabsListFeature.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recenttabs/RecentTabsListFeature.kt @@ -4,6 +4,7 @@ package org.mozilla.fenix.home.recenttabs +import android.graphics.Bitmap import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.map @@ -44,4 +45,21 @@ sealed class RecentTab { * @param state Recently viewed [TabSessionState] */ data class Tab(val state: TabSessionState) : RecentTab() + + /** + * A search term group that was recently viewed + * + * @param searchTerm The search term that was recently viewed. Forced to start with uppercase. + * @param tabId The id of the tab that was recently viewed + * @param url The url that was recently viewed + * @param thumbnail The thumbnail of the search term that was recently viewed + * @param count The number of tabs in the search term group + */ + data class SearchGroup( + val searchTerm: String, + val tabId: String, + val url: String, + val thumbnail: Bitmap?, + val count: Int + ) : RecentTab() } diff --git a/app/src/main/java/org/mozilla/fenix/home/recenttabs/view/RecentTabs.kt b/app/src/main/java/org/mozilla/fenix/home/recenttabs/view/RecentTabs.kt index 2c56d6b0f2..221075ece7 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recenttabs/view/RecentTabs.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recenttabs/view/RecentTabs.kt @@ -81,6 +81,7 @@ fun RecentTabs( onRecentTabClick = onRecentTabClick ) } + else -> {} } } } diff --git a/app/src/main/java/org/mozilla/fenix/home/recentvisits/RecentVisitsFeature.kt b/app/src/main/java/org/mozilla/fenix/home/recentvisits/RecentVisitsFeature.kt index 61259b9014..586933a52b 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recentvisits/RecentVisitsFeature.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recentvisits/RecentVisitsFeature.kt @@ -17,11 +17,16 @@ import mozilla.components.concept.storage.HistoryHighlightWeights import mozilla.components.concept.storage.HistoryMetadata import mozilla.components.concept.storage.HistoryMetadataStorage import mozilla.components.support.base.feature.LifecycleAwareFeature +import org.mozilla.fenix.FeatureFlags import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.components.appstate.AppAction import org.mozilla.fenix.home.HomeFragment +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItemInternal.HistoryGroupInternal import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItemInternal.HistoryHighlightInternal +import org.mozilla.fenix.utils.Settings.Companion.SEARCH_GROUP_MINIMUM_SITES +import kotlin.math.max @VisibleForTesting internal const val MAX_RESULTS_TOTAL = 9 @VisibleForTesting internal const val MIN_VIEW_TIME_OF_HIGHLIGHT = 10.0 @@ -45,6 +50,7 @@ class RecentVisitsFeature( private val historyHighlightsStorage: Lazy, private val scope: CoroutineScope, private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO, + private val historyImprovementFeatures: Boolean = FeatureFlags.historyImprovementFeatures, ) : LifecycleAwareFeature { private var job: Job? = null @@ -63,48 +69,75 @@ class RecentVisitsFeature( } val historyHighlights = getHistoryHighlights(highlights.await(), allHistoryMetadata.await()) + val historyGroups = getHistorySearchGroups(allHistoryMetadata.await()) - updateState(historyHighlights) + updateState(historyHighlights, historyGroups) } } @VisibleForTesting internal fun updateState( historyHighlights: List, + historyGroups: List ) { appStore.dispatch( AppAction.RecentHistoryChange( - getCombinedHistory(historyHighlights) + getCombinedHistory(historyHighlights, historyGroups) ) ) } /** - * Get up to [MAX_RESULTS_TOTAL] items if available of history highlights. - * Maps the internal highlights and search groups to the final objects to be returned. - * Items will be sorted by their last accessed date so that the most recent will be first. + * Get up to [MAX_RESULTS_TOTAL] items if available as an even split of history highlights and history groups. + * If more items then needed are available then highlights will be more by one. * * @param historyHighlights List of history highlights. Can be empty. + * @param historyGroups List of history groups. Can be empty. * * @return [RecentlyVisitedItem] list representing the data expected by clients of this feature. */ @VisibleForTesting internal fun getCombinedHistory( historyHighlights: List, + historyGroups: List ): List { - return historyHighlights - .sortedByDescending { it.lastAccessedTime } - .take(MAX_RESULTS_TOTAL) - .map { - RecentHistoryHighlight( - title = if (it.historyHighlight.title.isNullOrBlank()) { - it.historyHighlight.url - } else { - it.historyHighlight.title!! - }, - url = it.historyHighlight.url - ) + // Cleanup highlights now to avoid counting them below and then removing the ones found in groups. + val distinctHighlights = historyHighlights + .removeHighlightsAlreadyInGroups(historyGroups) + + val totalItemsCount = distinctHighlights.size + historyGroups.size + + return if (totalItemsCount <= MAX_RESULTS_TOTAL) { + getSortedHistory( + distinctHighlights.sortedByDescending { it.lastAccessedTime }, + historyGroups.sortedByDescending { it.lastAccessedTime } + ) + } else { + var groupsCount = 0 + var highlightCount = 0 + while ((highlightCount + groupsCount) < MAX_RESULTS_TOTAL) { + if ((highlightCount + groupsCount) < MAX_RESULTS_TOTAL && + distinctHighlights.getOrNull(highlightCount) != null + ) { + highlightCount += 1 + } + + if ((highlightCount + groupsCount) < MAX_RESULTS_TOTAL && + historyGroups.getOrNull(groupsCount) != null + ) { + groupsCount += 1 + } } + + getSortedHistory( + distinctHighlights + .sortedByDescending { it.lastAccessedTime } + .take(highlightCount), + historyGroups + .sortedByDescending { it.lastAccessedTime } + .take(groupsCount) + ) + } } /** @@ -142,11 +175,101 @@ class RecentVisitsFeature( } } + /** + * Group all urls accessed following a particular search. + * Automatically dedupes identical urls and adds each url's view time to the group's total. + * + * @param metadata List of history visits. + * + * @return List of user searches and all urls accessed from those. + */ + @VisibleForTesting + internal fun getHistorySearchGroups( + metadata: List + ): List { + return metadata + .filter { it.totalViewTime > 0 && it.key.searchTerm != null } + .groupBy { it.key.searchTerm!! } + .mapValues { group -> + // Within a group, we dedupe entries based on their url so we don't display + // a page multiple times in the same group, and we sum up the total view time + // of deduped entries while making sure to keep the latest updatedAt value. + val metadataInGroup = group.value + val metadataUrlGroups = metadataInGroup.groupBy { metadata -> metadata.key.url } + metadataUrlGroups.map { metadata -> + metadata.value.reduce { acc, elem -> + acc.copy( + totalViewTime = acc.totalViewTime + elem.totalViewTime, + updatedAt = max(acc.updatedAt, elem.updatedAt) + ) + } + } + } + .map { + HistoryGroupInternal( + groupName = it.key, + groupItems = it.value + ) + } + .filter { + if (historyImprovementFeatures) { + it.groupItems.size >= SEARCH_GROUP_MINIMUM_SITES + } else { + true + } + } + } + + /** + * Maps the internal highlights and search groups to the final objects to be returned. + * Items will be sorted by their last accessed date so that the most recent will be first. + */ + @VisibleForTesting + internal fun getSortedHistory( + historyHighlights: List, + historyGroups: List + ): List { + return (historyHighlights + historyGroups) + .sortedByDescending { it.lastAccessedTime } + .map { + when (it) { + is HistoryHighlightInternal -> RecentHistoryHighlight( + title = if (it.historyHighlight.title.isNullOrBlank()) { + it.historyHighlight.url + } else { + it.historyHighlight.title!! + }, + url = it.historyHighlight.url + ) + is HistoryGroupInternal -> RecentHistoryGroup( + title = it.groupName, + historyMetadata = it.groupItems + ) + } + } + } + override fun stop() { job?.cancel() } } +/** + * Filter out highlights that are already part of a history group. + */ +@VisibleForTesting +internal fun List.removeHighlightsAlreadyInGroups( + historyMetadata: List +): List { + return filterNot { highlight -> + historyMetadata.any { + it.groupItems.any { + it.key.url == highlight.historyHighlight.url + } + } + } +} + @VisibleForTesting internal sealed class RecentlyVisitedItemInternal { abstract val lastAccessedTime: Long @@ -158,4 +281,13 @@ internal sealed class RecentlyVisitedItemInternal { val historyHighlight: HistoryHighlight, override val lastAccessedTime: Long ) : RecentlyVisitedItemInternal() + + /** + * Temporary search group allowing for easier data manipulation. + */ + data class HistoryGroupInternal( + val groupName: String, + val groupItems: List, + override val lastAccessedTime: Long = groupItems.maxOf { it.updatedAt } + ) : RecentlyVisitedItemInternal() } diff --git a/app/src/main/java/org/mozilla/fenix/home/recentvisits/RecentlyVisitedItem.kt b/app/src/main/java/org/mozilla/fenix/home/recentvisits/RecentlyVisitedItem.kt index 23eb68adda..a3e39577f0 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recentvisits/RecentlyVisitedItem.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recentvisits/RecentlyVisitedItem.kt @@ -4,8 +4,11 @@ package org.mozilla.fenix.home.recentvisits +import mozilla.components.concept.storage.HistoryMetadata +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup + /** - * History items of previously accessed webpages. + * History items as individual or groups of previously accessed webpages. */ sealed class RecentlyVisitedItem { /** @@ -18,4 +21,18 @@ sealed class RecentlyVisitedItem { val title: String, val url: String ) : RecentlyVisitedItem() + + /** + * A group of previously accessed webpages related by their search terms. + * + * @property title The title of the group. + * @property historyMetadata A list of [HistoryMetadata] records that matches the title. + */ + data class RecentHistoryGroup( + val title: String, + val historyMetadata: List = emptyList() + ) : RecentlyVisitedItem() } + +// The last updated time of the group is based on the most recently updated item in the group +fun RecentHistoryGroup.lastUpdated(): Long = historyMetadata.maxOf { it.updatedAt } diff --git a/app/src/main/java/org/mozilla/fenix/home/recentvisits/controller/RecentVisitsController.kt b/app/src/main/java/org/mozilla/fenix/home/recentvisits/controller/RecentVisitsController.kt index adf138465a..7ea3db3e19 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recentvisits/controller/RecentVisitsController.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recentvisits/controller/RecentVisitsController.kt @@ -9,14 +9,19 @@ import androidx.annotation.VisibleForTesting.PRIVATE import androidx.navigation.NavController import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch +import mozilla.components.browser.state.action.HistoryMetadataAction import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.storage.HistoryMetadataStorage import mozilla.components.feature.tabs.TabsUseCases.SelectOrAddUseCase +import mozilla.components.service.glean.private.NoExtras +import org.mozilla.fenix.GleanMetrics.RecentSearches import org.mozilla.fenix.R import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.components.appstate.AppAction import org.mozilla.fenix.home.HomeFragmentDirections +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight +import org.mozilla.fenix.library.history.toHistoryMetadata /** * All possible updates following user interactions with the "Recent visits" section from the Home screen. @@ -28,6 +33,20 @@ interface RecentVisitsController { */ fun handleHistoryShowAllClicked() + /** + * Callback for when the user clicks on a specific [RecentHistoryGroup]. + * + * @param recentHistoryGroup The just clicked [RecentHistoryGroup]. + */ + fun handleRecentHistoryGroupClicked(recentHistoryGroup: RecentHistoryGroup) + + /** + * Callback for when the user removes a certain [RecentHistoryGroup]. + * + * @param groupTitle Title of the [RecentHistoryGroup] to remove. + */ + fun handleRemoveRecentHistoryGroup(groupTitle: String) + /** * Callback for when the user clicks on a specific [RecentHistoryHighlight]. * @@ -65,6 +84,39 @@ class DefaultRecentVisitsController( ) } + /** + * Navigates to the history metadata group fragment to display the group. + * + * @param recentHistoryGroup The [RecentHistoryGroup] to which to navigate to. + */ + override fun handleRecentHistoryGroupClicked(recentHistoryGroup: RecentHistoryGroup) { + navController.navigate( + HomeFragmentDirections.actionGlobalHistoryMetadataGroup( + title = recentHistoryGroup.title, + historyMetadataItems = recentHistoryGroup.historyMetadata + .mapIndexed { index, item -> item.toHistoryMetadata(index) }.toTypedArray() + ) + ) + } + + /** + * Removes a [RecentHistoryGroup] with the given title from the homescreen. + * + * @param groupTitle The title of the [RecentHistoryGroup] to be removed. + */ + override fun handleRemoveRecentHistoryGroup(groupTitle: String) { + // We want to update the UI right away in response to user action without waiting for the IO. + // First, dispatch actions that will clean up search groups in the two stores that have + // metadata-related state. + store.dispatch(HistoryMetadataAction.DisbandSearchGroupAction(searchTerm = groupTitle)) + appStore.dispatch(AppAction.DisbandSearchGroupAction(searchTerm = groupTitle)) + // Then, perform the expensive IO work of removing search groups from storage. + scope.launch { + storage.deleteHistoryMetadata(groupTitle) + } + RecentSearches.groupDeleted.record(NoExtras()) + } + /** * Switch to an already open tab for [recentHistoryHighlight] if one exists or * create a new tab in which to load this item's URL. diff --git a/app/src/main/java/org/mozilla/fenix/home/recentvisits/interactor/RecentVisitsInteractor.kt b/app/src/main/java/org/mozilla/fenix/home/recentvisits/interactor/RecentVisitsInteractor.kt index c99694312f..f6f1b10df0 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recentvisits/interactor/RecentVisitsInteractor.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recentvisits/interactor/RecentVisitsInteractor.kt @@ -4,6 +4,7 @@ package org.mozilla.fenix.home.recentvisits.interactor +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight /** @@ -16,6 +17,20 @@ interface RecentVisitsInteractor { */ fun onHistoryShowAllClicked() + /** + * Callbacks for when the user clicks on a [RecentHistoryGroup]. + * + * @param recentHistoryGroup The just clicked [RecentHistoryGroup]. + */ + fun onRecentHistoryGroupClicked(recentHistoryGroup: RecentHistoryGroup) + + /** + * Callback for when the user selected an option to remove a [RecentHistoryGroup]. + * + * @param groupTitle [RecentHistoryGroup.title] of the item to remove. + */ + fun onRemoveRecentHistoryGroup(groupTitle: String) + /** * Callback for when the user clicks on a [RecentHistoryHighlight]. * diff --git a/app/src/main/java/org/mozilla/fenix/home/recentvisits/view/RecentlyVisited.kt b/app/src/main/java/org/mozilla/fenix/home/recentvisits/view/RecentlyVisited.kt index 9c9089a1a6..52a80776b4 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recentvisits/view/RecentlyVisited.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recentvisits/view/RecentlyVisited.kt @@ -5,8 +5,10 @@ package org.mozilla.fenix.home.recentvisits.view import androidx.compose.foundation.ExperimentalFoundationApi +import androidx.compose.foundation.Image import androidx.compose.foundation.background import androidx.compose.foundation.combinedClickable +import androidx.compose.foundation.isSystemInDarkTheme import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column @@ -16,6 +18,7 @@ import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.fillMaxHeight import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.width import androidx.compose.foundation.lazy.LazyListState @@ -37,13 +40,17 @@ import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.platform.LocalConfiguration +import androidx.compose.ui.platform.LocalContext +import androidx.compose.ui.res.painterResource import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.sp +import org.mozilla.fenix.R import org.mozilla.fenix.compose.EagerFlingBehavior import org.mozilla.fenix.compose.Favicon import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight import org.mozilla.fenix.theme.FirefoxTheme import org.mozilla.fenix.theme.Theme @@ -96,6 +103,15 @@ fun RecentlyVisited( onRecentVisitClick(it, pageIndex + 1) } ) + is RecentHistoryGroup -> RecentlyVisitedHistoryGroup( + recentVisit = recentVisit, + menuItems = menuItems, + clickableEnabled = listState.atLeastHalfVisibleItems.contains(pageIndex), + showDividerLine = index < items.size - 1, + onRecentVisitClick = { + onRecentVisitClick(it, pageIndex + 1) + } + ) } } } @@ -104,6 +120,73 @@ fun RecentlyVisited( } } +/** + * A recently visited history group. + * + * @param recentVisit The [RecentHistoryGroup] to display. + * @param menuItems List of [RecentVisitMenuItem] to display in a recent visit dropdown menu. + * @param clickableEnabled Whether click actions should be invoked or not. + * @param showDividerLine Whether to show a divider line at the bottom. + * @param onRecentVisitClick Invoked when the user clicks on a recent visit. + */ +@OptIn(ExperimentalFoundationApi::class) +@Composable +private fun RecentlyVisitedHistoryGroup( + recentVisit: RecentHistoryGroup, + menuItems: List, + clickableEnabled: Boolean, + showDividerLine: Boolean, + onRecentVisitClick: (RecentHistoryGroup) -> Unit = { _ -> }, +) { + var isMenuExpanded by remember { mutableStateOf(false) } + + Row( + modifier = Modifier + .combinedClickable( + enabled = clickableEnabled, + onClick = { onRecentVisitClick(recentVisit) }, + onLongClick = { isMenuExpanded = true } + ) + .size(268.dp, 56.dp), + verticalAlignment = Alignment.CenterVertically + ) { + Image( + painter = painterResource(R.drawable.ic_multiple_tabs), + contentDescription = null, + modifier = Modifier.size(24.dp), + ) + + Spacer(modifier = Modifier.width(16.dp)) + + Column( + modifier = Modifier.fillMaxSize() + ) { + RecentlyVisitedTitle( + text = recentVisit.title, + modifier = Modifier + .padding(top = 7.dp, bottom = 2.dp) + .weight(1f) + ) + + RecentlyVisitedCaption( + count = recentVisit.historyMetadata.size, + modifier = Modifier.weight(1f) + ) + + if (showDividerLine) { + RecentlyVisitedDivider() + } + } + + RecentlyVisitedMenu( + showMenu = isMenuExpanded, + menuItems = menuItems, + recentVisit = recentVisit, + onDismissRequest = { isMenuExpanded = false } + ) + } +} + /** * A recently visited history item. * @@ -179,6 +262,36 @@ private fun RecentlyVisitedTitle( ) } +/** + * The caption text for a recent visit. + * + * @param count Number of recently visited items to display in the caption. + * @param modifier [Modifier] allowing to perfectly place this. + */ +@Composable +private fun RecentlyVisitedCaption( + count: Int, + modifier: Modifier +) { + val stringId = if (count == 1) { + R.string.history_search_group_site + } else { + R.string.history_search_group_sites + } + + Text( + text = String.format(LocalContext.current.getString(stringId), count), + modifier = modifier, + color = when (isSystemInDarkTheme()) { + true -> FirefoxTheme.colors.textPrimary + false -> FirefoxTheme.colors.textSecondary + }, + fontSize = 12.sp, + overflow = TextOverflow.Ellipsis, + maxLines = 1 + ) +} + /** * Menu shown for a [RecentlyVisitedItem]. * @@ -260,14 +373,10 @@ private fun RecentlyVisitedPreview() { FirefoxTheme(theme = Theme.getTheme()) { RecentlyVisited( recentVisits = listOf( - RecentHistoryHighlight( - title = "Google", - url = "www.google.com", - ), - RecentHistoryHighlight( - title = "Firefox", - url = "www.firefox.com", - ), + RecentHistoryGroup(title = "running shoes"), + RecentHistoryGroup(title = "mozilla"), + RecentHistoryGroup(title = "firefox"), + RecentHistoryGroup(title = "pocket") ), menuItems = emptyList() ) diff --git a/app/src/main/java/org/mozilla/fenix/home/recentvisits/view/RecentlyVisitedViewHolder.kt b/app/src/main/java/org/mozilla/fenix/home/recentvisits/view/RecentlyVisitedViewHolder.kt index d9a140097c..e371fef801 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recentvisits/view/RecentlyVisitedViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recentvisits/view/RecentlyVisitedViewHolder.kt @@ -11,11 +11,13 @@ import androidx.compose.ui.res.stringResource import androidx.lifecycle.LifecycleOwner import mozilla.components.lib.state.ext.observeAsComposableState import mozilla.components.service.glean.private.NoExtras +import org.mozilla.fenix.GleanMetrics.History import org.mozilla.fenix.GleanMetrics.RecentlyVisitedHomepage import org.mozilla.fenix.R import org.mozilla.fenix.components.components import org.mozilla.fenix.compose.ComposeViewHolder import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight import org.mozilla.fenix.home.recentvisits.interactor.RecentVisitsInteractor @@ -49,6 +51,7 @@ class RecentlyVisitedViewHolder( title = stringResource(R.string.recently_visited_menu_item_remove), onClick = { visit -> when (visit) { + is RecentHistoryGroup -> interactor.onRemoveRecentHistoryGroup(visit.title) is RecentHistoryHighlight -> interactor.onRemoveRecentHistoryHighlight( visit.url ) @@ -56,12 +59,21 @@ class RecentlyVisitedViewHolder( } ) ), - onRecentVisitClick = { recentlyVisitedItem, _ -> + onRecentVisitClick = { recentlyVisitedItem, pageNumber -> when (recentlyVisitedItem) { is RecentHistoryHighlight -> { RecentlyVisitedHomepage.historyHighlightOpened.record(NoExtras()) interactor.onRecentHistoryHighlightClicked(recentlyVisitedItem) } + is RecentHistoryGroup -> { + RecentlyVisitedHomepage.searchGroupOpened.record(NoExtras()) + History.recentSearchesTapped.record( + History.RecentSearchesTappedExtra( + pageNumber.toString() + ) + ) + interactor.onRecentHistoryGroupClicked(recentlyVisitedItem) + } } } ) 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 52dba43698..787bdb2739 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 @@ -23,6 +23,7 @@ import org.mozilla.fenix.home.recentsyncedtabs.interactor.RecentSyncedTabInterac import org.mozilla.fenix.home.recenttabs.RecentTab import org.mozilla.fenix.home.recenttabs.controller.RecentTabController import org.mozilla.fenix.home.recenttabs.interactor.RecentTabInteractor +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight import org.mozilla.fenix.home.recentvisits.controller.RecentVisitsController import org.mozilla.fenix.home.recentvisits.interactor.RecentVisitsInteractor @@ -403,6 +404,16 @@ class SessionControlInteractor( recentVisitsController.handleHistoryShowAllClicked() } + override fun onRecentHistoryGroupClicked(recentHistoryGroup: RecentHistoryGroup) { + recentVisitsController.handleRecentHistoryGroupClicked( + recentHistoryGroup + ) + } + + override fun onRemoveRecentHistoryGroup(groupTitle: String) { + recentVisitsController.handleRemoveRecentHistoryGroup(groupTitle) + } + override fun onRecentHistoryHighlightClicked(recentHistoryHighlight: RecentHistoryHighlight) { recentVisitsController.handleRecentHistoryHighlightClicked(recentHistoryHighlight) } diff --git a/app/src/main/res/drawable/ic_all_tabs.xml b/app/src/main/res/drawable/ic_all_tabs.xml new file mode 100644 index 0000000000..53e3663241 --- /dev/null +++ b/app/src/main/res/drawable/ic_all_tabs.xml @@ -0,0 +1,14 @@ + + + + + diff --git a/app/src/main/res/drawable/ic_search_group_thumbnail.xml b/app/src/main/res/drawable/ic_search_group_thumbnail.xml new file mode 100644 index 0000000000..72166bd9b8 --- /dev/null +++ b/app/src/main/res/drawable/ic_search_group_thumbnail.xml @@ -0,0 +1,49 @@ + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 2ace68b92e..0cc85f4fb0 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -117,10 +117,10 @@ Show all recent tabs button - Your search for \"%1$s\" + Your search for \"%1$s\" - %d sites + %d sites See all synced tabs diff --git a/app/src/test/java/org/mozilla/fenix/components/AppStoreTest.kt b/app/src/test/java/org/mozilla/fenix/components/AppStoreTest.kt index 5dc0c2815f..b3e12d9ae0 100644 --- a/app/src/test/java/org/mozilla/fenix/components/AppStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/AppStoreTest.kt @@ -31,6 +31,7 @@ import org.mozilla.fenix.browser.browsingmode.BrowsingModeManager import org.mozilla.fenix.components.appstate.AppAction import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.UpdateMessageToShow import org.mozilla.fenix.components.appstate.AppState +import org.mozilla.fenix.components.appstate.filterOut import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.getFilteredStories import org.mozilla.fenix.home.CurrentMode @@ -42,6 +43,7 @@ import org.mozilla.fenix.home.recentsyncedtabs.RecentSyncedTab import org.mozilla.fenix.home.recentsyncedtabs.RecentSyncedTabState import org.mozilla.fenix.home.recenttabs.RecentTab import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight import org.mozilla.fenix.onboarding.FenixOnboarding @@ -131,10 +133,13 @@ class AppStoreTest { @Test fun `Test changing the recent tabs in AppStore`() = runTest { - val highlight = RecentHistoryHighlight(title = "title", "") + val group1 = RecentHistoryGroup(title = "title1") + val group2 = RecentHistoryGroup(title = "title2") + val group3 = RecentHistoryGroup(title = "title3") + val highlight = RecentHistoryHighlight(title = group2.title, "") appStore = AppStore( AppState( - recentHistory = listOf(highlight) + recentHistory = listOf(group1, group2, group3, highlight) ) ) assertEquals(0, appStore.state.recentTabs.size) @@ -146,7 +151,7 @@ class AppStoreTest { appStore.dispatch(AppAction.RecentTabsChange(recentTabs)).join() assertEquals(recentTabs, appStore.state.recentTabs) - assertEquals(listOf(highlight), appStore.state.recentHistory) + assertEquals(listOf(group1, group3, highlight), appStore.state.recentHistory) } @Test @@ -172,7 +177,7 @@ class AppStoreTest { fun `Test changing the history metadata in AppStore`() = runTest { assertEquals(0, appStore.state.recentHistory.size) - val historyMetadata: List = listOf(mockk(), mockk()) + val historyMetadata: List = listOf(mockk(), mockk()) appStore.dispatch(AppAction.RecentHistoryChange(historyMetadata)).join() assertEquals(historyMetadata, appStore.state.recentHistory) @@ -180,10 +185,12 @@ class AppStoreTest { @Test fun `Test removing a history highlight from AppStore`() = runTest { + val g1 = RecentHistoryGroup(title = "group One") + val g2 = RecentHistoryGroup(title = "grup two") val h1 = RecentHistoryHighlight(title = "highlight One", url = "url1") val h2 = RecentHistoryHighlight(title = "highlight two", url = "url2") val recentHistoryState = AppState( - recentHistory = listOf(h1, h2) + recentHistory = listOf(g1, g2, h1, h2) ) appStore = AppStore(recentHistoryState) @@ -195,7 +202,7 @@ class AppStoreTest { appStore.dispatch(AppAction.RemoveRecentHistoryHighlight(h1.url)).join() assertEquals( - recentHistoryState.copy(recentHistory = listOf(h2)), + recentHistoryState.copy(recentHistory = listOf(g1, g2, h2)), appStore.state ) } @@ -234,12 +241,16 @@ class AppStoreTest { assertEquals(0, appStore.state.recentHistory.size) assertEquals(Mode.Normal, appStore.state.mode) + val recentGroup = RecentTab.SearchGroup("testSearchTerm", "id", "url", null, 3) val collections: List = listOf(mockk()) val topSites: List = listOf(mockk(), mockk()) val recentTabs: List = listOf(mockk(), mockk()) val recentBookmarks: List = listOf(mockk(), mockk()) - val highlight = RecentHistoryHighlight("title", "") - val recentHistory: List = listOf(highlight) + val group1 = RecentHistoryGroup(title = "test One") + val group2 = RecentHistoryGroup(title = recentGroup.searchTerm.lowercase()) + val group3 = RecentHistoryGroup(title = "test two") + val highlight = RecentHistoryHighlight(group2.title, "") + val recentHistory: List = listOf(group1, group2, group3, highlight) appStore.dispatch( AppAction.Change( @@ -257,7 +268,7 @@ class AppStoreTest { assertEquals(topSites, appStore.state.topSites) assertEquals(recentTabs, appStore.state.recentTabs) assertEquals(recentBookmarks, appStore.state.recentBookmarks) - assertEquals(listOf(highlight), appStore.state.recentHistory) + assertEquals(listOf(group1, group3, highlight), appStore.state.recentHistory) assertEquals(Mode.Private, appStore.state.mode) } @@ -469,4 +480,21 @@ class AppStoreTest { assertSame(firstFilteredStories, appStore.state.pocketStories) } } + + @Test + fun `Test filtering out search groups`() { + val group1 = RecentHistoryGroup("title1") + val group2 = RecentHistoryGroup("title2") + val group3 = RecentHistoryGroup("title3") + val highLight1 = RecentHistoryHighlight("title1", "") + val highLight2 = RecentHistoryHighlight("title2", "") + val highLight3 = RecentHistoryHighlight("title3", "") + val recentHistory = listOf(group1, highLight1, group2, highLight2, group3, highLight3) + + assertEquals(recentHistory, recentHistory.filterOut(null)) + assertEquals(recentHistory, recentHistory.filterOut("")) + assertEquals(recentHistory, recentHistory.filterOut(" ")) + assertEquals(recentHistory - group2, recentHistory.filterOut("Title2")) + assertEquals(recentHistory - group3, recentHistory.filterOut("title3")) + } } diff --git a/app/src/test/java/org/mozilla/fenix/home/recentvisits/RecentVisitsFeatureTest.kt b/app/src/test/java/org/mozilla/fenix/home/recentvisits/RecentVisitsFeatureTest.kt index 648100abec..b73d35b27c 100644 --- a/app/src/test/java/org/mozilla/fenix/home/recentvisits/RecentVisitsFeatureTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/recentvisits/RecentVisitsFeatureTest.kt @@ -24,13 +24,16 @@ import mozilla.components.support.test.middleware.CaptureActionsMiddleware import mozilla.components.support.test.rule.MainCoroutineRule import mozilla.components.support.test.rule.runTestOnMain import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Rule import org.junit.Test import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.components.appstate.AppAction import org.mozilla.fenix.components.appstate.AppState +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItemInternal.HistoryGroupInternal import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItemInternal.HistoryHighlightInternal import kotlin.random.Random @@ -66,6 +69,10 @@ class RecentVisitsFeatureTest { documentType = DocumentType.Regular, previewImageUrl = null ) + val recentHistoryGroup = RecentHistoryGroup( + title = "mozilla", + historyMetadata = listOf(historyEntry) + ) val highlightEntry = HistoryHighlight(1.0, 1, "https://firefox.com", "firefox", null) val recentHistoryHighlight = RecentHistoryHighlight("firefox", "https://firefox.com") coEvery { historyMetadataStorage.getHistoryMetadataSince(any()) }.coAnswers { @@ -80,7 +87,7 @@ class RecentVisitsFeatureTest { startRecentVisitsFeature() middleware.assertLastAction(AppAction.RecentHistoryChange::class) { - assertEquals(listOf(recentHistoryHighlight), it.recentHistory) + assertEquals(listOf(recentHistoryGroup, recentHistoryHighlight), it.recentHistory) } } @@ -106,7 +113,200 @@ class RecentVisitsFeatureTest { } @Test - fun `GIVEN multiple highlights exist WHEN they are added to store THEN only MAX_RESULTS_TOTAL are sent`() = + fun `GIVEN groups containing history metadata items with the same url WHEN they are added to store THEN entries are deduped`() = + runTestOnMain { + val historyEntry1 = HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null), + title = "mozilla", + createdAt = System.currentTimeMillis(), + updatedAt = 1, + totalViewTime = 10, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + + val historyEntry2 = HistoryMetadata( + key = HistoryMetadataKey("http://firefox.com", "mozilla", null), + title = "firefox", + createdAt = System.currentTimeMillis(), + updatedAt = 2, + totalViewTime = 20, + documentType = DocumentType.Regular, + previewImageUrl = "http://firefox.com/image1" + ) + + val historyEntry3 = HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null), + title = "mozilla", + createdAt = System.currentTimeMillis(), + updatedAt = 3, + totalViewTime = 30, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + + val expectedHistoryGroup = RecentHistoryGroup( + title = "mozilla", + historyMetadata = listOf( + // Expected total view time to be summed up for deduped entries + historyEntry1.copy( + totalViewTime = historyEntry1.totalViewTime + historyEntry3.totalViewTime, + updatedAt = historyEntry3.updatedAt + ), + historyEntry2 + ) + ) + + coEvery { historyMetadataStorage.getHistoryMetadataSince(any()) }.coAnswers { + listOf( + historyEntry1, historyEntry2, historyEntry3 + ) + } + + startRecentVisitsFeature() + + middleware.assertLastAction(AppAction.RecentHistoryChange::class) { + assertEquals(listOf(expectedHistoryGroup), it.recentHistory) + } + } + + @Test + fun `GIVEN different groups containing history metadata items with the same url WHEN they are added to store THEN entries are not deduped`() = + runTestOnMain { + val now = System.currentTimeMillis() + val historyEntry1 = HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null), + title = "mozilla", + createdAt = now, + updatedAt = now + 3, + totalViewTime = 10, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + + val historyEntry2 = HistoryMetadata( + key = HistoryMetadataKey("http://firefox.com", "mozilla", null), + title = "firefox", + createdAt = now, + updatedAt = now + 2, + totalViewTime = 20, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + + val historyEntry3 = HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", "firefox", null), + title = "mozilla", + createdAt = now, + updatedAt = now + 1, + totalViewTime = 30, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + + val expectedHistoryGroup1 = RecentHistoryGroup( + title = "mozilla", + historyMetadata = listOf(historyEntry1, historyEntry2) + ) + + val expectedHistoryGroup2 = RecentHistoryGroup( + title = "firefox", + historyMetadata = listOf(historyEntry3) + ) + + coEvery { historyMetadataStorage.getHistoryMetadataSince(any()) }.coAnswers { + listOf( + historyEntry1, historyEntry2, historyEntry3 + ) + } + + startRecentVisitsFeature() + + middleware.assertLastAction(AppAction.RecentHistoryChange::class) { + assertEquals(listOf(expectedHistoryGroup1, expectedHistoryGroup2), it.recentHistory) + } + } + + @Test + fun `GIVEN history groups WHEN they are added to store THEN they are sorted descending by last updated timestamp`() = + runTestOnMain { + val now = System.currentTimeMillis() + val historyEntry1 = HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null), + title = "mozilla", + createdAt = now, + updatedAt = now + 1, + totalViewTime = 10, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + + val historyEntry2 = HistoryMetadata( + key = HistoryMetadataKey("http://firefox.com", "mozilla", null), + title = "firefox", + createdAt = now, + updatedAt = now + 2, + totalViewTime = 20, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + + val historyEntry3 = HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", "firefox", null), + title = "mozilla", + createdAt = now, + updatedAt = now + 3, + totalViewTime = 30, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + + val expectedHistoryGroup1 = RecentHistoryGroup( + title = "mozilla", + historyMetadata = listOf(historyEntry1, historyEntry2) + ) + + val expectedHistoryGroup2 = RecentHistoryGroup( + title = "firefox", + historyMetadata = listOf(historyEntry3) + ) + + coEvery { historyMetadataStorage.getHistoryMetadataSince(any()) }.coAnswers { + listOf( + historyEntry1, historyEntry2, historyEntry3 + ) + } + + startRecentVisitsFeature() + + middleware.assertLastAction(AppAction.RecentHistoryChange::class) { + assertEquals(listOf(expectedHistoryGroup2, expectedHistoryGroup1), it.recentHistory) + } + } + + @Test + fun `GIVEN multiple groups exist but no highlights WHEN they are added to store THEN only MAX_RESULTS_TOTAL are sent`() = + runTestOnMain { + val visitsFromSearch = getSearchFromHistoryMetadataItems(10) + val expectedRecentHistoryGroups = visitsFromSearch + // Expect to only have the last accessed 9 groups. + .subList(1, 10) + .toIndividualRecentHistoryGroups() + coEvery { historyMetadataStorage.getHistoryMetadataSince(any()) }.coAnswers { visitsFromSearch } + + startRecentVisitsFeature() + + middleware.assertLastAction(AppAction.RecentHistoryChange::class) { + assertEquals( + // The 9 most recent groups. + expectedRecentHistoryGroups, + it.recentHistory + ) + } + } + + @Test + fun `GIVEN multiple highlights exist but no history groups WHEN they are added to store THEN only MAX_RESULTS_TOTAL are sent`() = runTestOnMain { val highlights = getHistoryHighlightsItems(10) val expectedRecentHighlights = highlights @@ -126,12 +326,70 @@ class RecentVisitsFeatureTest { } @Test - fun `GIVEN a list of history highlights WHEN updateState is called THEN emit RecentHistoryChange`() { - val feature = spyk(RecentVisitsFeature(appStore, mockk(), mockk(), mockk(), mockk())) + fun `GIVEN multiple history highlights and history groups WHEN they are added to store THEN only last accessed are added`() = + runTestOnMain { + val visitsFromSearch = getSearchFromHistoryMetadataItems(10) + val directVisits = getDirectVisitsHistoryMetadataItems(10) + val expectedRecentHistoryGroups = visitsFromSearch + // Expect only 4 groups. Take 5 here for using in the below zip() and be dropped after. + .subList(5, 10) + .toIndividualRecentHistoryGroups() + val expectedRecentHistoryHighlights = directVisits.reversed().toRecentHistoryHighlights() + val expectedItems = expectedRecentHistoryHighlights.zip(expectedRecentHistoryGroups).flatMap { + listOf(it.first, it.second) + }.take(9) + coEvery { historyMetadataStorage.getHistoryMetadataSince(any()) }.coAnswers { visitsFromSearch + directVisits } + coEvery { historyHightlightsStorage.getHistoryHighlights(any(), any()) }.coAnswers { + directVisits.toHistoryHighlights() + } + + startRecentVisitsFeature() + + middleware.assertLastAction(AppAction.RecentHistoryChange::class) { + assertEquals(expectedItems, it.recentHistory) + } + } + + @Test + fun `GIVEN history highlights exist as history metadata WHEN they are added to store THEN don't add highlight dupes`() { + // To know if a highlight appears in a search group each visit's url should be checked. + val visitsFromSearch = getSearchFromHistoryMetadataItems(10) + val directDistinctVisits = getDirectVisitsHistoryMetadataItems(10).takeLast(2) + val directDupeVisits = visitsFromSearch.takeLast(2).map { + // Erase the search term for this to not be mapped to a search group. + // The url remains the same as the item from a group so it should be skipped. + it.copy(key = it.key.copy(searchTerm = null)) + } + val expectedRecentHistoryGroups = visitsFromSearch + .subList(3, 10) + .toIndividualRecentHistoryGroups() + val expectedRecentHistoryHighlights = directDistinctVisits.reversed().toRecentHistoryHighlights() + val expectedItems = listOf( + expectedRecentHistoryHighlights.first(), + expectedRecentHistoryGroups.first(), + expectedRecentHistoryHighlights[1] + ) + expectedRecentHistoryGroups.subList(1, expectedRecentHistoryGroups.size) + coEvery { historyMetadataStorage.getHistoryMetadataSince(any()) }.coAnswers { + visitsFromSearch + directDistinctVisits + directDupeVisits + } + coEvery { historyHightlightsStorage.getHistoryHighlights(any(), any()) }.coAnswers { + directDistinctVisits.toHistoryHighlights() + directDupeVisits.toHistoryHighlights() + } + + startRecentVisitsFeature() + + middleware.assertLastAction(AppAction.RecentHistoryChange::class) { + assertEquals(expectedItems, it.recentHistory) + } + } + + @Test + fun `GIVEN a list of history highlights and groups WHEN updateState is called THEN emit RecentHistoryChange`() { + val feature = spyk(RecentVisitsFeature(appStore, mockk(), mockk(), mockk(), mockk(), false)) val expected = List(1) { mockk() } - every { feature.getCombinedHistory(any()) } returns expected + every { feature.getCombinedHistory(any(), any()) } returns expected - feature.updateState(emptyList()) + feature.updateState(emptyList(), emptyList()) appStore.waitUntilIdle() middleware.assertLastAction(AppAction.RecentHistoryChange::class) { @@ -139,9 +397,84 @@ class RecentVisitsFeatureTest { } } + @Test + 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(), false) + val visitsFromSearch = getSearchFromHistoryMetadataItems(4) + val directVisits = getDirectVisitsHistoryMetadataItems(4) + val directDupeVisits = getSearchFromHistoryMetadataItems(2).map { + // Erase the search term for this to not be mapped to a search group. + // The url remains the same as the item from a group so it should be skipped. + it.copy(key = it.key.copy(searchTerm = null)) + } + val expected = directVisits.reversed().toRecentHistoryHighlights() + .zip(visitsFromSearch.toIndividualRecentHistoryGroups()) + .flatMap { + listOf(it.first, it.second) + } + + val result = feature.getCombinedHistory( + (directVisits + directDupeVisits).toHistoryHighlightsInternal(), + visitsFromSearch.toHistoryGroupsInternal() + ) + + assertEquals(expected, result) + } + + @Test + 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(), false) + val visitsFromSearch = getSearchFromHistoryMetadataItems(4) + val directVisits = getDirectVisitsHistoryMetadataItems(4) + val expected = directVisits.reversed().toRecentHistoryHighlights() + .zip(visitsFromSearch.toIndividualRecentHistoryGroups()) + .flatMap { + listOf(it.first, it.second) + } + + val result = feature.getCombinedHistory( + directVisits.toHistoryHighlightsInternal(), + visitsFromSearch.toHistoryGroupsInternal() + ) + + assertEquals(expected, result) + } + + @Test + 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(), false) + val visitsFromSearch = getSearchFromHistoryMetadataItems(5) + val directVisits = getDirectVisitsHistoryMetadataItems(14) + val expected = directVisits.takeLast(5).reversed().toRecentHistoryHighlights() + + visitsFromSearch.takeLast(4).toIndividualRecentHistoryGroups() + + val result = feature.getCombinedHistory( + directVisits.toHistoryHighlightsInternal(), + visitsFromSearch.toHistoryGroupsInternal() + ) + + assertEquals(expected, result) + } + + @Test + 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(), false) + val visitsFromSearch = getSearchFromHistoryMetadataItems(14) + val directVisits = getDirectVisitsHistoryMetadataItems(5) + val expected = visitsFromSearch.takeLast(4).toIndividualRecentHistoryGroups() + + directVisits.takeLast(5).reversed().toRecentHistoryHighlights() + + val result = feature.getCombinedHistory( + directVisits.toHistoryHighlightsInternal(), + visitsFromSearch.toHistoryGroupsInternal() + ) + + assertEquals(expected, result) + } + @Test 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 directVisits = getDirectVisitsHistoryMetadataItems(10) @@ -158,7 +491,7 @@ class RecentVisitsFeatureTest { @Test 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 directVisits = getDirectVisitsHistoryMetadataItems(10) val highlightsWithUnknownAccessTime = directVisits.toHistoryHighlightsInternal().take(5).map { @@ -179,7 +512,7 @@ class RecentVisitsFeatureTest { @Test 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 directVisits = getDirectVisitsHistoryMetadataItems(10) val newerDirectVisits = directVisits.mapIndexed { index, item -> @@ -200,8 +533,70 @@ class RecentVisitsFeatureTest { } @Test - fun `GIVEN highlights don't have a valid title WHEN getCombinedHistory is called THEN the url is set as title`() { - val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk()) + 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(), false) + val directVisits = getDirectVisitsHistoryMetadataItems(10) + + val result = feature.getHistorySearchGroups(directVisits) + + assertTrue(result.isEmpty()) + } + + @Test + 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(), false) + val visitsFromSearch = getSearchFromHistoryMetadataItems(10) + val directVisits = getDirectVisitsHistoryMetadataItems(10) + + val result = feature.getHistorySearchGroups(visitsFromSearch + directVisits) + + assertEquals(10, result.size) + assertEquals(visitsFromSearch.map { it.key.searchTerm }, result.map { it.groupName }) + assertEquals(visitsFromSearch.map { listOf(it) }, result.map { it.groupItems }) + } + + @Test + 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(), false) + val visitsFromSearch = getSearchFromHistoryMetadataItems(10) + val newerVisitsFromSearch = visitsFromSearch.map { it.copy(updatedAt = it.updatedAt * 2) } + val directVisits = getDirectVisitsHistoryMetadataItems(10) + + val result = feature.getHistorySearchGroups(visitsFromSearch + directVisits + newerVisitsFromSearch) + + assertEquals(10, result.size) + assertEquals(newerVisitsFromSearch.map { it.key.searchTerm }, result.map { it.groupName }) + assertEquals( + newerVisitsFromSearch.map { + listOf(it.copy(totalViewTime = it.totalViewTime * 2,)) + }, + result.map { it.groupItems } + ) + } + + @Test + 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(), false) + val visitsFromSearch = getSearchFromHistoryMetadataItems(10) + val directVisits = getDirectVisitsHistoryMetadataItems(10) + val expected = directVisits.reversed().toRecentHistoryHighlights() + .zip(visitsFromSearch.toIndividualRecentHistoryGroups()) + .flatMap { + listOf(it.first, it.second) + } + + val result = feature.getSortedHistory( + directVisits.toHistoryHighlightsInternal(), + visitsFromSearch.toHistoryGroupsInternal() + ) + + assertEquals(expected, result) + } + + @Test + 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(), false) + val visitsFromSearch = getSearchFromHistoryMetadataItems(10) val directVisits = getDirectVisitsHistoryMetadataItems(10).mapIndexed { index, item -> when (index % 3) { 0 -> item @@ -211,11 +606,12 @@ class RecentVisitsFeatureTest { } val sortedByDateHighlights = directVisits.reversed() - val result = feature.getCombinedHistory( + val result = feature.getSortedHistory( directVisits.toHistoryHighlightsInternal(), + visitsFromSearch.toHistoryGroupsInternal() ).filterIsInstance() - assertEquals(9, result.size) + assertEquals(10, result.size) result.forEachIndexed { index, item -> when (index % 3) { 0 -> assertEquals(sortedByDateHighlights[index].title, item.title) @@ -225,6 +621,25 @@ class RecentVisitsFeatureTest { } } + @Test + fun `GIVEN highlight visits also exist in search groups WHEN removeHighlightsAlreadyInGroups is called THEN filter out such highlights`() { + val visitsFromSearch = getSearchFromHistoryMetadataItems(10) + // To know if a highlight appears in a search group each visit's url should be checked. + // Ensure we have the identical urls with the ones from a search group and also some random others. + val directDupeVisits = visitsFromSearch.mapIndexed { index, item -> + when (index % 2) { + 0 -> item + else -> item.copy(key = item.key.copy(url = "https://mozilla.org")) + } + } + val highlights = directDupeVisits.toHistoryHighlightsInternal() + + val result = highlights.removeHighlightsAlreadyInGroups(visitsFromSearch.toHistoryGroupsInternal()) + + assertEquals(5, result.size) + result.forEach { assertEquals("https://mozilla.org", it.historyHighlight.url) } + } + private fun startRecentVisitsFeature() { val feature = RecentVisitsFeature( appStore, @@ -232,8 +647,11 @@ class RecentVisitsFeatureTest { lazy { historyHightlightsStorage }, scope, testDispatcher, + false ) + assertEquals(emptyList(), appStore.state.recentHistory) + feature.start() scope.advanceUntilIdle() @@ -328,6 +746,15 @@ private fun HistoryMetadata.toHistoryHighlight(): HistoryHighlight = HistoryHigh previewImageUrl = null ) +private fun HistoryMetadata.toRecentHistoryGroup(): RecentHistoryGroup = RecentHistoryGroup( + title = key.searchTerm!!, + historyMetadata = listOf(this) +) + +private fun List.toIndividualRecentHistoryGroups(): List = + map { it.toRecentHistoryGroup() } + .sortedByDescending { it.lastUpdated() } + private fun HistoryMetadata.toRecentHistoryHighlight(): RecentHistoryHighlight = RecentHistoryHighlight( title = if (title.isNullOrBlank()) key.url else title!!, @@ -357,3 +784,10 @@ private fun HistoryMetadata.toHistoryHighlightInternal(lastAccessTime: Long) = private fun List.toHistoryHighlightsInternal() = mapIndexed { index, item -> item.toHistoryHighlightInternal(index + 1L) } + +private fun HistoryMetadata.toHistoryGroupInternal() = HistoryGroupInternal( + groupName = key.searchTerm!!, + groupItems = listOf(this) +) + +private fun List.toHistoryGroupsInternal() = map { it.toHistoryGroupInternal() } diff --git a/app/src/test/java/org/mozilla/fenix/home/recentvisits/controller/RecentVisitsControllerTest.kt b/app/src/test/java/org/mozilla/fenix/home/recentvisits/controller/RecentVisitsControllerTest.kt index 4e50d06d41..8b20787968 100644 --- a/app/src/test/java/org/mozilla/fenix/home/recentvisits/controller/RecentVisitsControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/recentvisits/controller/RecentVisitsControllerTest.kt @@ -5,6 +5,8 @@ package org.mozilla.fenix.home.recentvisits.controller import androidx.navigation.NavController +import androidx.navigation.NavDirections +import io.mockk.coVerify import io.mockk.every import io.mockk.mockk import io.mockk.spyk @@ -12,22 +14,31 @@ import io.mockk.verify import io.mockk.verifyOrder import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.launch +import kotlinx.coroutines.test.advanceUntilIdle +import mozilla.components.browser.state.action.HistoryMetadataAction import mozilla.components.browser.state.store.BrowserStore +import mozilla.components.concept.storage.DocumentType +import mozilla.components.concept.storage.HistoryMetadata +import mozilla.components.concept.storage.HistoryMetadataKey import mozilla.components.concept.storage.HistoryMetadataStorage import mozilla.components.feature.tabs.TabsUseCases.SelectOrAddUseCase import mozilla.components.service.glean.testing.GleanTestRule import mozilla.components.support.test.robolectric.testContext import mozilla.components.support.test.rule.MainCoroutineRule import mozilla.components.support.test.rule.runTestOnMain +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull import org.junit.Before import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith +import org.mozilla.fenix.GleanMetrics.RecentSearches import org.mozilla.fenix.R import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.components.appstate.AppAction import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.home.HomeFragmentDirections +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight @OptIn(ExperimentalCoroutinesApi::class) @@ -82,6 +93,69 @@ class RecentVisitsControllerTest { } } + @Test + fun handleRecentHistoryGroupClicked() = runTestOnMain { + val historyEntry = HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null), + title = "mozilla", + createdAt = System.currentTimeMillis(), + updatedAt = System.currentTimeMillis(), + totalViewTime = 10, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + val historyGroup = RecentHistoryGroup( + title = "mozilla", + historyMetadata = listOf(historyEntry) + ) + + controller.handleRecentHistoryGroupClicked(historyGroup) + + verify { + navController.navigate( + match { it.actionId == R.id.action_global_history_metadata_group } + ) + } + } + + @Test + fun handleRemoveGroup() = runTestOnMain { + val historyMetadataKey = HistoryMetadataKey( + "http://www.mozilla.com", + "mozilla", + null + ) + + val historyGroup = RecentHistoryGroup( + title = "mozilla", + historyMetadata = listOf( + HistoryMetadata( + key = historyMetadataKey, + title = "mozilla", + createdAt = System.currentTimeMillis(), + updatedAt = System.currentTimeMillis(), + totalViewTime = 10, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + ) + ) + assertNull(RecentSearches.groupDeleted.testGetValue()) + + controller.handleRemoveRecentHistoryGroup(historyGroup.title) + + advanceUntilIdle() + verify { + store.dispatch(HistoryMetadataAction.DisbandSearchGroupAction(searchTerm = historyGroup.title)) + appStore.dispatch(AppAction.DisbandSearchGroupAction(searchTerm = historyGroup.title)) + } + assertNotNull(RecentSearches.groupDeleted.testGetValue()) + + coVerify { + storage.deleteHistoryMetadata(historyGroup.title) + } + } + @Test fun handleRecentHistoryHighlightClicked() = runTestOnMain { val historyHighlight = RecentHistoryHighlight("title", "url") diff --git a/app/src/test/java/org/mozilla/fenix/home/recentvisits/interactor/RecentVisitsInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/home/recentvisits/interactor/RecentVisitsInteractorTest.kt index 667aff61fb..6f6687c5ba 100644 --- a/app/src/test/java/org/mozilla/fenix/home/recentvisits/interactor/RecentVisitsInteractorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/recentvisits/interactor/RecentVisitsInteractorTest.kt @@ -6,12 +6,16 @@ package org.mozilla.fenix.home.recentvisits.interactor import io.mockk.mockk import io.mockk.verify +import mozilla.components.concept.storage.DocumentType +import mozilla.components.concept.storage.HistoryMetadata +import mozilla.components.concept.storage.HistoryMetadataKey import org.junit.Before import org.junit.Test import org.mozilla.fenix.home.pocket.PocketStoriesController import org.mozilla.fenix.home.recentbookmarks.controller.RecentBookmarksController import org.mozilla.fenix.home.recentsyncedtabs.controller.RecentSyncedTabController import org.mozilla.fenix.home.recenttabs.controller.RecentTabController +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight import org.mozilla.fenix.home.recentvisits.controller.RecentVisitsController import org.mozilla.fenix.home.sessioncontrol.DefaultSessionControlController @@ -40,12 +44,67 @@ class RecentVisitsInteractorTest { ) } + @Test + fun handleRecentHistoryGroupClicked() { + val historyGroup = + RecentHistoryGroup( + title = "mozilla", + historyMetadata = listOf( + HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", null, null), + title = "mozilla", + createdAt = System.currentTimeMillis(), + updatedAt = System.currentTimeMillis(), + totalViewTime = 10, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + ) + ) + + interactor.onRecentHistoryGroupClicked(historyGroup) + verify { + recentVisitsController.handleRecentHistoryGroupClicked(historyGroup) + } + } + @Test fun handleHistoryShowAllClicked() { interactor.onHistoryShowAllClicked() verify { recentVisitsController.handleHistoryShowAllClicked() } } + @Test + fun onRemoveRecentHistoryGroup() { + val historyMetadataKey = HistoryMetadataKey( + "http://www.mozilla.com", + "mozilla", + null + ) + + val historyGroup = + RecentHistoryGroup( + title = "mozilla", + historyMetadata = listOf( + HistoryMetadata( + key = historyMetadataKey, + title = "mozilla", + createdAt = System.currentTimeMillis(), + updatedAt = System.currentTimeMillis(), + totalViewTime = 10, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + ) + ) + + interactor.onRemoveRecentHistoryGroup(historyGroup.title) + + verify { + recentVisitsController.handleRemoveRecentHistoryGroup(historyGroup.title) + } + } + @Test fun onRecentHistoryHighlightClicked() { val historyHighlight: RecentHistoryHighlight = mockk() 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 d22b0d7781..8ed4baf402 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 @@ -24,7 +24,7 @@ import org.mozilla.fenix.gleanplumb.Message import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.home.recentbookmarks.RecentBookmark import org.mozilla.fenix.home.recenttabs.RecentTab -import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem +import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGroup import org.mozilla.fenix.utils.Settings @RunWith(FenixRobolectricTestRunner::class) @@ -56,7 +56,7 @@ class SessionControlViewTest { @Test fun `GIVEN historyMetadata WHEN calling shouldShowHomeOnboardingDialog THEN show the dialog `() { - val historyMetadata = listOf(RecentlyVisitedItem.RecentHistoryHighlight("title", "")) + val historyMetadata = listOf(RecentHistoryGroup("title", emptyList())) val settings: Settings = mockk() every { settings.hasShownHomeOnboardingDialog } returns false @@ -138,7 +138,7 @@ class SessionControlViewTest { val collections = emptyList() val expandedCollections = emptySet() val recentBookmarks = listOf(RecentBookmark()) - val historyMetadata = emptyList() + val historyMetadata = emptyList() val pocketStories = emptyList() every { settings.showTopSitesFeature } returns true @@ -173,7 +173,7 @@ class SessionControlViewTest { val collections = emptyList() val expandedCollections = emptySet() val recentBookmarks = listOf(RecentBookmark()) - val historyMetadata = emptyList() + val historyMetadata = emptyList() val pocketStories = emptyList() val nimbusMessageCard: Message = mockk() @@ -206,7 +206,7 @@ class SessionControlViewTest { val collections = emptyList() val expandedCollections = emptySet() val recentBookmarks = listOf() - val historyMetadata = emptyList() + val historyMetadata = emptyList() val pocketStories = emptyList() every { settings.showTopSitesFeature } returns true @@ -241,7 +241,7 @@ class SessionControlViewTest { val collections = emptyList() val expandedCollections = emptySet() val recentBookmarks = listOf() - val historyMetadata = listOf(mockk()) + val historyMetadata = listOf(RecentHistoryGroup("title", emptyList())) val pocketStories = emptyList() every { settings.showTopSitesFeature } returns true @@ -276,7 +276,7 @@ class SessionControlViewTest { val collections = emptyList() val expandedCollections = emptySet() val recentBookmarks = listOf() - val historyMetadata = emptyList() + val historyMetadata = emptyList() val pocketStories = listOf(PocketRecommendedStory("", "", "", "", "", 1, 1)) every { settings.showTopSitesFeature } returns true @@ -312,7 +312,7 @@ class SessionControlViewTest { val collections = emptyList() val expandedCollections = emptySet() val recentBookmarks = listOf() - val historyMetadata = emptyList() + val historyMetadata = emptyList() val pocketStories = emptyList() every { settings.showTopSitesFeature } returns true @@ -347,7 +347,7 @@ class SessionControlViewTest { val collections = listOf(collection) val expandedCollections = emptySet() val recentBookmarks = listOf(mockk()) - val historyMetadata = listOf(mockk()) + val historyMetadata = listOf(mockk()) val pocketStories = listOf(mockk()) every { settings.showTopSitesFeature } returns true