diff --git a/app/src/main/java/org/mozilla/fenix/ext/HomeFragmentState.kt b/app/src/main/java/org/mozilla/fenix/ext/HomeFragmentState.kt index ff82c2d241..5f3734f832 100644 --- a/app/src/main/java/org/mozilla/fenix/ext/HomeFragmentState.kt +++ b/app/src/main/java/org/mozilla/fenix/ext/HomeFragmentState.kt @@ -11,15 +11,12 @@ import org.mozilla.fenix.home.sessioncontrol.viewholders.pocket.POCKET_STORIES_D import org.mozilla.fenix.home.sessioncontrol.viewholders.pocket.PocketRecommendedStoryCategory /** - * Get the list of stories to be displayed. - * Either the stories from the [POCKET_STORIES_DEFAULT_CATEGORY_NAME] either - * filtered stories based on the user selected categories. + * Get the list of stories to be displayed based on the user selected categories. * * @param neededStoriesCount how many stories are intended to be displayed. * This only impacts filtered results guaranteeing an even spread of stories from each category. * - * @return a list of [PocketRecommendedStory]es from the currently selected categories - * topped if necessary with stories from the [POCKET_STORIES_DEFAULT_CATEGORY_NAME] up to [neededStoriesCount]. + * @return a list of [PocketRecommendedStory]es from the currently selected categories. */ fun HomeFragmentState.getFilteredStories( neededStoriesCount: Int @@ -39,43 +36,27 @@ fun HomeFragmentState.getFilteredStories( .sortedByDescending { it.lastInteractedWithTimestamp } val filteredStoriesCount = getFilteredStoriesCount( - pocketStoriesCategories, oldestSortedCategories, neededStoriesCount + oldestSortedCategories, neededStoriesCount ) - // Add general stories at the end of the stories list to show until neededStoriesCount - val generalStoriesTopup = filteredStoriesCount[POCKET_STORIES_DEFAULT_CATEGORY_NAME]?.let { neededTopups -> - pocketStoriesCategories - .find { it.name == POCKET_STORIES_DEFAULT_CATEGORY_NAME } - ?.stories - ?.sortedBy { it.timesShown } - ?.take(neededTopups) - } ?: emptyList() - return oldestSortedCategories .flatMap { category -> category.stories.sortedBy { it.timesShown }.take(filteredStoriesCount[category.name]!!) - }.plus(generalStoriesTopup) - .take(neededStoriesCount) + }.take(neededStoriesCount) } /** * Get how many stories needs to be shown from each currently selected category. * - * If the selected categories together don't have [neededStoriesCount] stories then - * the difference is added from the [POCKET_STORIES_DEFAULT_CATEGORY_NAME] category. - * - * @param allCategories the list of all Pocket stories categories. * @param selectedCategories ordered list of categories from which to return results. * @param neededStoriesCount how many stories are intended to be displayed. * This impacts the results by guaranteeing an even spread of stories from each category in that stories count. * * @return a mapping of how many stories are to be shown from each category from [selectedCategories]. - * The result is topped with stories counts from the [POCKET_STORIES_DEFAULT_CATEGORY_NAME] up to [neededStoriesCount]. */ @VisibleForTesting @Suppress("ReturnCount", "NestedBlockDepth") internal fun getFilteredStoriesCount( - allCategories: List, selectedCategories: List, neededStoriesCount: Int ): Map { @@ -83,17 +64,8 @@ internal fun getFilteredStoriesCount( availableStories + category.stories.size } - when { - totalStoriesInFilteredCategories == neededStoriesCount -> { - return selectedCategories.map { it.name to it.stories.size }.toMap() - } - totalStoriesInFilteredCategories < neededStoriesCount -> { - return selectedCategories.map { it.name to it.stories.size }.toMap() + - allCategories.filter { it.name == POCKET_STORIES_DEFAULT_CATEGORY_NAME }.map { - it.name to (neededStoriesCount - totalStoriesInFilteredCategories).coerceAtMost(it.stories.size) - }.toMap() - } - else -> { + when (totalStoriesInFilteredCategories > neededStoriesCount) { + true -> { val storiesCountFromEachCategory = mutableMapOf() var currentFilteredStoriesCount = 0 @@ -110,6 +82,9 @@ internal fun getFilteredStoriesCount( } } } + false -> { + return selectedCategories.map { it.name to it.stories.size }.toMap() + } } return emptyMap() diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt index 8f825d96c8..6bf220da1e 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt @@ -149,7 +149,7 @@ private fun homeFragmentStateReducer( val updatedCategoriesState = state.copy( pocketStoriesCategories = state.pocketStoriesCategories.map { when (it.name == action.categoryName) { - true -> it.copy(isSelected = true) + true -> it.copy(isSelected = true, lastInteractedWithTimestamp = System.currentTimeMillis()) false -> it } } @@ -163,7 +163,7 @@ private fun homeFragmentStateReducer( // Deselecting a category means the stories to be displayed needs to also be changed. pocketStoriesCategories = state.pocketStoriesCategories.map { when (it.name == action.categoryName) { - true -> it.copy(isSelected = false) + true -> it.copy(isSelected = false, lastInteractedWithTimestamp = System.currentTimeMillis()) false -> it } } diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/pocket/PocketStoriesComposables.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/pocket/PocketStoriesComposables.kt index ca58ae6194..8f4d269586 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/pocket/PocketStoriesComposables.kt +++ b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/pocket/PocketStoriesComposables.kt @@ -50,6 +50,13 @@ import org.mozilla.fenix.theme.FirefoxTheme import kotlin.math.roundToInt import kotlin.random.Random +/** + * Placeholder [PocketRecommendedStory] allowing to combine other items in the same list that shows stories. + * It uses empty values for it's properties ensuring that no conflict is possible since real stories have + * mandatory values. + */ +private val placeholderStory = PocketRecommendedStory("", "", "", "", "", 0, 0) + /** * Displays a single [PocketRecommendedStory]. * @@ -96,25 +103,28 @@ fun PocketStories( client: Client, onExternalLinkClicked: (String) -> Unit ) { - // Show stories in a 3 by 3 grid - val gridLength = 3 + // Show stories in at most 3 rows but on any number of columns depending on the data received. + val maxRowsNo = 3 + val storiesToShow = (stories + placeholderStory).chunked(maxRowsNo) + val listState = rememberLazyListState() val flingBehavior = EagerFlingBehavior(lazyRowState = listState) LazyRow(state = listState, flingBehavior = flingBehavior) { - itemsIndexed(stories.chunked(gridLength)) { rowIndex, columnItems -> - Column(Modifier.padding(end = if (rowIndex < gridLength - 1) 8.dp else 0.dp)) { - for (index in 0 until gridLength) { - columnItems.getOrNull(index)?.let { story -> + itemsIndexed(storiesToShow) { columnIndex, columnItems -> + Column(Modifier.padding(end = if (columnIndex < storiesToShow.size - 1) 8.dp else 0.dp)) { + columnItems.forEachIndexed { rowIndex, story -> + if (story == placeholderStory) { + ListItemTabLargePlaceholder(stringResource(R.string.pocket_stories_placeholder_text)) { + onExternalLinkClicked("http://getpocket.com/explore") + } + } else { PocketStory(story, client) { onExternalLinkClicked(story.url) } - } ?: ListItemTabLargePlaceholder(stringResource(R.string.pocket_stories_placeholder_text)) { - onExternalLinkClicked("http://getpocket.com/explore") } - // Add padding between all rows. Not also after the last. - if (index < gridLength - 1) { + if (rowIndex < maxRowsNo - 1) { Spacer(modifier = Modifier.height(8.dp)) } } @@ -137,7 +147,7 @@ fun PocketStoriesCategories( StaggeredHorizontalGrid( horizontalItemsSpacing = 16.dp ) { - categories.forEach { category -> + categories.filter { it.name != POCKET_STORIES_DEFAULT_CATEGORY_NAME }.forEach { category -> SelectableChip(category.name, category.isSelected) { onCategoryClick(category) } diff --git a/app/src/test/java/org/mozilla/fenix/ext/HomeFragmentStateTest.kt b/app/src/test/java/org/mozilla/fenix/ext/HomeFragmentStateTest.kt index 42a9199af3..97f07c4059 100644 --- a/app/src/test/java/org/mozilla/fenix/ext/HomeFragmentStateTest.kt +++ b/app/src/test/java/org/mozilla/fenix/ext/HomeFragmentStateTest.kt @@ -74,24 +74,6 @@ class HomeFragmentStateTest { assertNull(result.firstOrNull { it.category != otherStoriesCategory.name }) } - @Test - fun `GIVEN a category is selected WHEN getFilteredStories is called for more than in the category THEN results topped with ones from the default category are returned`() { - val homeState = HomeFragmentState( - pocketStoriesCategories = listOf( - otherStoriesCategory.copy(isSelected = true), anotherStoriesCategory, defaultStoriesCategory - ) - ) - - val result = homeState.getFilteredStories(5) - - assertEquals(5, result.size) - assertEquals(3, result.filter { it.category == otherStoriesCategory.name }.size) - assertEquals( - 2, - result.filter { it.category == POCKET_STORIES_DEFAULT_CATEGORY_NAME }.size - ) - } - @Test fun `GIVEN two categories are selected WHEN getFilteredStories is called for fewer than in both THEN only stories from those categories are returned`() { val homeState = HomeFragmentState( @@ -119,27 +101,6 @@ class HomeFragmentStateTest { ) } - @Test - fun `GIVEN two categories are selected WHEN getFilteredStories is called for more than in the categories THEN results topped with ones from the default category are returned`() { - val homeState = HomeFragmentState( - pocketStoriesCategories = listOf( - otherStoriesCategory.copy(isSelected = true), - anotherStoriesCategory.copy(isSelected = true), - defaultStoriesCategory - ) - ) - - val result = homeState.getFilteredStories(8) - - assertEquals(8, result.size) - assertEquals(3, result.filter { it.category == otherStoriesCategory.name }.size) - assertEquals(3, result.filter { it.category == anotherStoriesCategory.name }.size) - assertEquals( - 2, - result.filter { it.category == POCKET_STORIES_DEFAULT_CATEGORY_NAME }.size - ) - } - @Test fun `GIVEN two categories are selected WHEN getFilteredStories is called for an odd number of stories THEN there are more by one stories from the newest category`() { val firstSelectedCategory = otherStoriesCategory.copy(lastInteractedWithTimestamp = 0, isSelected = true) @@ -158,59 +119,36 @@ class HomeFragmentStateTest { } @Test - fun `GIVEN no category is selected WHEN getFilteredStoriesCount is called THEN Pocket stories count only from the default category are returned`() { - val availableCategories = listOf(otherStoriesCategory, defaultStoriesCategory, anotherStoriesCategory) - - var result = getFilteredStoriesCount(availableCategories, emptyList(), 2) - assertEquals(1, result.keys.size) - assertEquals(defaultStoriesCategory.name, result.entries.first().key) - assertEquals(2, result[defaultStoriesCategory.name]) + fun `GIVEN no category is selected WHEN getFilteredStoriesCount is called THEN return an empty result`() { + val result = getFilteredStoriesCount(emptyList(), 1) - result = getFilteredStoriesCount(availableCategories, emptyList(), 5) - assertEquals(1, result.keys.size) - assertEquals(defaultStoriesCategory.name, result.entries.first().key) - assertEquals(3, result[defaultStoriesCategory.name]) + assertTrue(result.isEmpty()) } @Test fun `GIVEN a category is selected WHEN getFilteredStoriesCount is called for at most the stories from this category THEN only stories count only from that category are returned`() { - val availableCategories = listOf(otherStoriesCategory, defaultStoriesCategory, anotherStoriesCategory) - - var result = getFilteredStoriesCount(availableCategories, listOf(otherStoriesCategory), 2) + var result = getFilteredStoriesCount(listOf(otherStoriesCategory), 2) assertEquals(1, result.keys.size) assertEquals(otherStoriesCategory.name, result.entries.first().key) assertEquals(2, result[otherStoriesCategory.name]) - result = getFilteredStoriesCount(availableCategories, listOf(otherStoriesCategory), 3) + result = getFilteredStoriesCount(listOf(otherStoriesCategory), 3) assertEquals(1, result.keys.size) assertEquals(otherStoriesCategory.name, result.entries.first().key) assertEquals(3, result[otherStoriesCategory.name]) } @Test - fun `GIVEN a category is selected WHEN getFilteredStoriesCount is called for more stories than this category has THEN results topped with ones from the default category are returned`() { - val availableCategories = listOf(otherStoriesCategory, defaultStoriesCategory, anotherStoriesCategory) - - val result = getFilteredStoriesCount(availableCategories, listOf(otherStoriesCategory), 5) - - assertEquals(2, result.keys.size) - assertTrue( - result.keys.containsAll( - listOf( - defaultStoriesCategory.name, - otherStoriesCategory.name - ) - ) - ) + fun `GIVEN a category is selected WHEN getFilteredStoriesCount is called for more stories than in this category THEN return only that`() { + var result = getFilteredStoriesCount(listOf(otherStoriesCategory), 4) + assertEquals(1, result.keys.size) + assertEquals(otherStoriesCategory.name, result.entries.first().key) assertEquals(3, result[otherStoriesCategory.name]) - assertEquals(2, result[defaultStoriesCategory.name]) } @Test fun `GIVEN two categories are selected WHEN getFilteredStoriesCount is called for at most the stories count in both THEN only stories counts from those categories are returned`() { - val availableCategories = listOf(otherStoriesCategory, defaultStoriesCategory, anotherStoriesCategory) - - var result = getFilteredStoriesCount(availableCategories, listOf(otherStoriesCategory, anotherStoriesCategory), 2) + var result = getFilteredStoriesCount(listOf(otherStoriesCategory, anotherStoriesCategory), 2) assertEquals(2, result.keys.size) assertTrue( result.keys.containsAll( @@ -223,7 +161,7 @@ class HomeFragmentStateTest { assertEquals(1, result[otherStoriesCategory.name]) assertEquals(1, result[anotherStoriesCategory.name]) - result = getFilteredStoriesCount(availableCategories, listOf(otherStoriesCategory, anotherStoriesCategory), 6) + result = getFilteredStoriesCount(listOf(otherStoriesCategory, anotherStoriesCategory), 6) assertEquals(2, result.keys.size) assertTrue( result.keys.containsAll( @@ -238,16 +176,12 @@ class HomeFragmentStateTest { } @Test - fun `GIVEN two categories are selected WHEN getFilteredStoriesCount is called for more results than in those categories THEN results topped with ones from the default category are returned`() { - val availableCategories = listOf(otherStoriesCategory, defaultStoriesCategory, anotherStoriesCategory) - - val result = getFilteredStoriesCount(availableCategories, listOf(otherStoriesCategory, anotherStoriesCategory), 8) - - assertEquals(3, result.size) + fun `GIVEN two categories are selected WHEN getFilteredStoriesCount is called for more results than stories in both THEN only stories counts from those categories are returned`() { + val result = getFilteredStoriesCount(listOf(otherStoriesCategory, anotherStoriesCategory), 8) + assertEquals(2, result.keys.size) assertTrue( result.keys.containsAll( listOf( - defaultStoriesCategory.name, otherStoriesCategory.name, anotherStoriesCategory.name ) @@ -255,15 +189,11 @@ class HomeFragmentStateTest { ) assertEquals(3, result[otherStoriesCategory.name]) assertEquals(3, result[anotherStoriesCategory.name]) - assertEquals(2, result[defaultStoriesCategory.name]) } @Test fun `GIVEN two categories are selected WHEN getFilteredStoriesCount is called for an odd number of results THEN there are more by one results from first selected category`() { - val availableCategories = listOf(otherStoriesCategory, defaultStoriesCategory, anotherStoriesCategory) - - // The lastInteractedWithTimestamp is not checked in this method but the selected categories order - val result = getFilteredStoriesCount(availableCategories, listOf(otherStoriesCategory, anotherStoriesCategory), 5) + val result = getFilteredStoriesCount(listOf(otherStoriesCategory, anotherStoriesCategory), 5) assertTrue( result.keys.containsAll( diff --git a/app/src/test/java/org/mozilla/fenix/home/HomeFragmentStoreTest.kt b/app/src/test/java/org/mozilla/fenix/home/HomeFragmentStoreTest.kt index 83a89a754e..fb9a5075ff 100644 --- a/app/src/test/java/org/mozilla/fenix/home/HomeFragmentStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/HomeFragmentStoreTest.kt @@ -208,10 +208,9 @@ class HomeFragmentStoreTest { verify { any().getFilteredStories(POCKET_STORIES_TO_SHOW_COUNT) } } - assertTrue( - listOf(otherStoriesCategory.copy(isSelected = true)) - .containsAll(homeFragmentStore.state.pocketStoriesCategories.filter { it.isSelected }) - ) + val selectedCategories = homeFragmentStore.state.pocketStoriesCategories.filter { it.isSelected } + assertEquals(1, selectedCategories.size) + assertTrue(otherStoriesCategory.name === selectedCategories[0].name) assertSame(filteredStories, homeFragmentStore.state.pocketStories) }