From b13e1499009fab20bfdeedcc125f41e12891d5ba Mon Sep 17 00:00:00 2001 From: Grisha Kruglov Date: Thu, 16 Dec 2021 17:08:13 -0800 Subject: [PATCH] [fenix] Closes https://github.com/mozilla-mobile/fenix/issues/22764: Convert History.id to an Int position We were converting Long timestamps into Ints (and getting negative numbers back), and treating that as, basically, a position for the paging API; the paging API would pass us back the obscure negative number back as an offset, and we'll mishandle it resulting in an infinite loop. This patch removes all of the Long -> Int conversions, and introduces an explicit 'position' that is calculated once we have a full page of results completed. --- .../history/PagedHistoryProvider.kt | 50 ++--- .../library/history/HistoryDataSource.kt | 48 ++++- .../library/history/HistoryFragmentStore.kt | 12 +- .../fenix/library/history/HistoryView.kt | 2 +- .../view/HistoryMetadataGroupAdapter.kt | 2 +- .../history/PagedHistoryProviderTest.kt | 29 +-- .../library/history/HistoryAdapterTest.kt | 13 -- .../library/history/HistoryDataSourceTest.kt | 204 ++++++++++++++++++ .../HistoryMetadataGroupFragmentStoreTest.kt | 2 - .../HistoryMetadataGroupControllerTest.kt | 2 - .../HistoryMetadataGroupItemViewHolderTest.kt | 1 - 11 files changed, 281 insertions(+), 84 deletions(-) create mode 100644 app/src/test/java/org/mozilla/fenix/library/history/HistoryDataSourceTest.kt diff --git a/app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt b/app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt index e94b1989f6..dee137c074 100644 --- a/app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt +++ b/app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt @@ -28,7 +28,7 @@ interface PagedHistoryProvider { * @param numberOfItems How many items to fetch * @param onComplete A callback that returns the list of [History] */ - fun getHistory(offset: Long, numberOfItems: Long, onComplete: (List) -> Unit) + fun getHistory(offset: Int, numberOfItems: Int, onComplete: (List) -> Unit) } /** @@ -63,8 +63,8 @@ class DefaultPagedHistoryProvider( @Suppress("LongMethod") override fun getHistory( - offset: Long, - numberOfItems: Long, + offset: Int, + numberOfItems: Int, onComplete: (List) -> Unit, ) { // A PagedList DataSource runs on a background thread automatically. @@ -75,14 +75,13 @@ class DefaultPagedHistoryProvider( if (showHistorySearchGroups) { // We need to re-fetch all the history metadata if the offset resets back at 0 // in the case of a pull to refresh. - if (historyGroups == null || offset == 0L) { + if (historyGroups == null || offset == 0) { historyGroups = historyStorage.getHistoryMetadataSince(Long.MIN_VALUE) .sortedByDescending { it.createdAt } .filter { it.key.searchTerm != null } .groupBy { it.key.searchTerm!! } .map { (searchTerm, items) -> History.Group( - id = items.first().createdAt.toInt(), title = searchTerm, visitedAt = items.first().createdAt, items = items.map { it.toHistoryMetadata() } @@ -94,11 +93,11 @@ class DefaultPagedHistoryProvider( } else { history = historyStorage .getVisitsPaginated( - offset, - numberOfItems, + offset.toLong(), + numberOfItems.toLong(), excludeTypes = excludedVisitTypes ) - .mapIndexed(transformVisitInfoToHistoryItem(offset.toInt())) + .map { transformVisitInfoToHistoryItem(it) } } onComplete(history) @@ -142,17 +141,17 @@ class DefaultPagedHistoryProvider( @Suppress("MagicNumber") private suspend fun getHistoryAndSearchGroups( - offset: Long, - numberOfItems: Long, + offset: Int, + numberOfItems: Int, ): List { val result = mutableListOf() val history: List = historyStorage .getVisitsPaginated( - offset, - numberOfItems, + offset.toLong(), + numberOfItems.toLong(), excludeTypes = excludedVisitTypes ) - .mapIndexed(transformVisitInfoToHistoryItem(offset.toInt())) + .map { transformVisitInfoToHistoryItem(it) } // We'll use this list to filter out redirects from metadata groups below. val redirectsInThePage = if (history.isNotEmpty()) { @@ -173,7 +172,7 @@ class DefaultPagedHistoryProvider( // History metadata items are recorded after their associated visited info, we add an // additional buffer time to the most recent visit to account for a history group // appearing as the most recent item. - val visitedAtBuffer = if (offset == 0L) BUFFER_TIME else 0 + val visitedAtBuffer = if (offset == 0) BUFFER_TIME else 0 // Get the history groups that fit within the range of visited times in the current history // items. @@ -208,19 +207,16 @@ class DefaultPagedHistoryProvider( .sortedByDescending { it.visitedAt } } - private fun transformVisitInfoToHistoryItem(offset: Int): (id: Int, visit: VisitInfo) -> History.Regular { - return { id, visit -> - val title = visit.title - ?.takeIf(String::isNotEmpty) - ?: visit.url.tryGetHostFromUrl() - - History.Regular( - id = offset + id, - title = title, - url = visit.url, - visitedAt = visit.visitTime - ) - } + private fun transformVisitInfoToHistoryItem(visit: VisitInfo): History.Regular { + val title = visit.title + ?.takeIf(String::isNotEmpty) + ?: visit.url.tryGetHostFromUrl() + + return History.Regular( + title = title, + url = visit.url, + visitedAt = visit.visitTime + ) } } diff --git a/app/src/main/java/org/mozilla/fenix/library/history/HistoryDataSource.kt b/app/src/main/java/org/mozilla/fenix/library/history/HistoryDataSource.kt index a9b7c60089..fd70d88428 100644 --- a/app/src/main/java/org/mozilla/fenix/library/history/HistoryDataSource.kt +++ b/app/src/main/java/org/mozilla/fenix/library/history/HistoryDataSource.kt @@ -4,6 +4,7 @@ package org.mozilla.fenix.library.history +import androidx.annotation.VisibleForTesting import androidx.paging.ItemKeyedDataSource import org.mozilla.fenix.components.history.PagedHistoryProvider @@ -14,26 +15,61 @@ class HistoryDataSource( // Because the pagination is not based off of the key // we want to start at 1, not 0 to be able to send the correct offset // to the `historyProvider.getHistory` call. - override fun getKey(item: History): Int = item.id + 1 + override fun getKey(item: History): Int = item.position!! + 1 override fun loadInitial( params: LoadInitialParams, callback: LoadInitialCallback ) { - historyProvider.getHistory(INITIAL_OFFSET, params.requestedLoadSize.toLong()) { history -> - callback.onResult(history) + historyProvider.getHistory(INITIAL_OFFSET, params.requestedLoadSize) { history -> + val positionedHistory = assignPositions(offset = INITIAL_OFFSET, history) + callback.onResult(positionedHistory) } } override fun loadAfter(params: LoadParams, callback: LoadCallback) { - historyProvider.getHistory(params.key.toLong(), params.requestedLoadSize.toLong()) { history -> - callback.onResult(history) + historyProvider.getHistory(params.key, params.requestedLoadSize) { history -> + val positionedHistory = assignPositions(offset = params.key, history) + callback.onResult(positionedHistory) } } override fun loadBefore(params: LoadParams, callback: LoadCallback) { /* noop */ } companion object { - private const val INITIAL_OFFSET = 0L + private const val INITIAL_OFFSET = 0 + + @VisibleForTesting + internal fun assignPositions(offset: Int, history: List): List { + return history.foldIndexed(listOf()) { index, prev, item -> + // Only offset once while folding, so that we don't accumulate the offset for each element. + val itemOffset = if (index == 0) { + offset + } else { + 0 + } + val previousPosition = prev.lastOrNull()?.position ?: 0 + when (item) { + is History.Group -> { + // XXX considering an empty group to have a non-zero offset is the obvious + // limitation of the current approach, and indicates that we're conflating + // two concepts here - position of an element for the sake of a RecyclerView, + // and an offset for the sake of our history pagination API. + val groupOffset = if (item.items.isEmpty()) { + 1 + } else { + item.items.size + } + prev + item.copy(position = previousPosition + itemOffset + groupOffset) + } + is History.Metadata -> { + prev + item.copy(position = previousPosition + itemOffset + 1) + } + is History.Regular -> { + prev + item.copy(position = previousPosition + itemOffset + 1) + } + } + } + } } } diff --git a/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragmentStore.kt b/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragmentStore.kt index 587e292f80..2fe0d98ef3 100644 --- a/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragmentStore.kt +++ b/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragmentStore.kt @@ -17,7 +17,7 @@ import mozilla.components.support.ktx.kotlin.tryGetHostFromUrl * Class representing a history entry. */ sealed class History : Parcelable { - abstract val id: Int + abstract val position: Int? abstract val title: String abstract val visitedAt: Long abstract val selected: Boolean @@ -32,7 +32,7 @@ sealed class History : Parcelable { * @property selected Whether or not the history item is selected. */ @Parcelize data class Regular( - override val id: Int, + override val position: Int? = null, override val title: String, val url: String, override val visitedAt: Long, @@ -52,7 +52,7 @@ sealed class History : Parcelable { * @property selected Whether or not the history metadata item is selected. */ @Parcelize data class Metadata( - override val id: Int, + override val position: Int? = null, override val title: String, val url: String, override val visitedAt: Long, @@ -71,7 +71,7 @@ sealed class History : Parcelable { * @property selected Whether or not the history group is selected. */ @Parcelize data class Group( - override val id: Int, + override val position: Int? = null, override val title: String, override val visitedAt: Long, val items: List, @@ -82,9 +82,9 @@ sealed class History : Parcelable { /** * Extension function for converting a [HistoryMetadata] into a [History.Metadata]. */ -fun HistoryMetadata.toHistoryMetadata(): History.Metadata { +fun HistoryMetadata.toHistoryMetadata(position: Int? = null): History.Metadata { return History.Metadata( - id = createdAt.toInt(), + position = position, title = title?.takeIf(String::isNotEmpty) ?: key.url.tryGetHostFromUrl(), url = key.url, diff --git a/app/src/main/java/org/mozilla/fenix/library/history/HistoryView.kt b/app/src/main/java/org/mozilla/fenix/library/history/HistoryView.kt index 24f818367c..c771b76c04 100644 --- a/app/src/main/java/org/mozilla/fenix/library/history/HistoryView.kt +++ b/app/src/main/java/org/mozilla/fenix/library/history/HistoryView.kt @@ -76,7 +76,7 @@ class HistoryView( val unselectedItems = oldMode.selectedItems - state.mode.selectedItems state.mode.selectedItems.union(unselectedItems).forEach { item -> - historyAdapter.notifyItemChanged(item.id) + historyAdapter.notifyItemChanged(item.position!!) } } diff --git a/app/src/main/java/org/mozilla/fenix/library/historymetadata/view/HistoryMetadataGroupAdapter.kt b/app/src/main/java/org/mozilla/fenix/library/historymetadata/view/HistoryMetadataGroupAdapter.kt index e0a5d6bbbe..524b784055 100644 --- a/app/src/main/java/org/mozilla/fenix/library/historymetadata/view/HistoryMetadataGroupAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/library/historymetadata/view/HistoryMetadataGroupAdapter.kt @@ -46,7 +46,7 @@ class HistoryMetadataGroupAdapter( internal object DiffCallback : DiffUtil.ItemCallback() { override fun areContentsTheSame(oldItem: History.Metadata, newItem: History.Metadata): Boolean = - oldItem.id == newItem.id + oldItem.position == newItem.position override fun areItemsTheSame(oldItem: History.Metadata, newItem: History.Metadata): Boolean = oldItem == newItem diff --git a/app/src/test/java/org/mozilla/fenix/components/history/PagedHistoryProviderTest.kt b/app/src/test/java/org/mozilla/fenix/components/history/PagedHistoryProviderTest.kt index 81a5742a75..986b2f8ffe 100644 --- a/app/src/test/java/org/mozilla/fenix/components/history/PagedHistoryProviderTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/history/PagedHistoryProviderTest.kt @@ -93,7 +93,7 @@ class PagedHistoryProviderTest { coEvery { storage.getHistoryMetadataSince(any()) } returns listOf(historyEntry1, historyEntry2, historyEntry3) var actualResults: List? = null - provider.getHistory(10L, 5) { + provider.getHistory(10, 5) { actualResults = it } @@ -115,13 +115,11 @@ class PagedHistoryProviderTest { val results = listOf( History.Group( - id = historyEntry1.createdAt.toInt(), title = historyEntry1.key.searchTerm!!, visitedAt = historyEntry1.createdAt, // Results are de-duped by URL and sorted descending by createdAt/visitedAt items = listOf( History.Metadata( - id = historyEntry1.createdAt.toInt(), title = historyEntry1.title!!, url = historyEntry1.key.url, visitedAt = historyEntry1.createdAt, @@ -129,7 +127,6 @@ class PagedHistoryProviderTest { historyMetadataKey = historyMetadataKey1 ), History.Metadata( - id = historyEntry3.createdAt.toInt(), title = historyEntry3.title!!, url = historyEntry3.key.url, visitedAt = historyEntry3.createdAt, @@ -139,7 +136,6 @@ class PagedHistoryProviderTest { ) ), History.Regular( - id = 12, title = visitInfo3.title!!, url = visitInfo3.url, visitedAt = visitInfo3.visitTime @@ -180,7 +176,7 @@ class PagedHistoryProviderTest { coEvery { storage.getHistoryMetadataSince(any()) } returns listOf(historyEntry1) var actualResults: List? = null - provider.getHistory(0L, 5) { + provider.getHistory(0, 5) { actualResults = it } @@ -202,13 +198,11 @@ class PagedHistoryProviderTest { val results = listOf( History.Group( - id = historyEntry1.createdAt.toInt(), title = historyEntry1.key.searchTerm!!, visitedAt = historyEntry1.createdAt, // Results are de-duped by URL and sorted descending by createdAt/visitedAt items = listOf( History.Metadata( - id = historyEntry1.createdAt.toInt(), title = historyEntry1.title!!, url = historyEntry1.key.url, visitedAt = historyEntry1.createdAt, @@ -254,7 +248,7 @@ class PagedHistoryProviderTest { coEvery { storage.getHistoryMetadataSince(any()) } returns listOf(historyEntry1) var actualResults: List? = null - provider.getHistory(0L, 5) { + provider.getHistory(0, 5) { actualResults = it } @@ -276,13 +270,11 @@ class PagedHistoryProviderTest { val results = listOf( History.Group( - id = historyEntry1.createdAt.toInt(), title = historyEntry1.key.searchTerm!!, visitedAt = historyEntry1.createdAt, // Results are de-duped by URL and sorted descending by createdAt/visitedAt items = listOf( History.Metadata( - id = historyEntry1.createdAt.toInt(), title = historyEntry1.title!!, url = historyEntry1.key.url, visitedAt = historyEntry1.createdAt, @@ -405,7 +397,7 @@ class PagedHistoryProviderTest { coEvery { storage.getHistoryMetadataSince(any()) } returns listOf(historyEntry1, historyEntry2, historyEntry3, historyEntry4) var actualResults: List? = null - provider.getHistory(10L, 5) { + provider.getHistory(10, 5) { actualResults = it } @@ -427,12 +419,10 @@ class PagedHistoryProviderTest { val results = listOf( History.Group( - id = historyEntry2.createdAt.toInt(), title = historyEntry2.key.searchTerm!!, visitedAt = historyEntry2.createdAt, items = listOf( History.Metadata( - id = historyEntry2.createdAt.toInt(), title = historyEntry2.title!!, url = historyEntry2.key.url, visitedAt = historyEntry2.createdAt, @@ -440,7 +430,6 @@ class PagedHistoryProviderTest { historyMetadataKey = historyMetadataKey2 ), History.Metadata( - id = historyEntry1.createdAt.toInt(), title = historyEntry1.title!!, url = historyEntry1.key.url, visitedAt = historyEntry1.createdAt, @@ -457,37 +446,31 @@ class PagedHistoryProviderTest { fun `WHEN removeConsecutiveDuplicates is called THEN all consecutive duplicates must be removed`() { val results = listOf( History.Group( - id = 1, title = "Group 1", visitedAt = 0, items = emptyList() ), History.Regular( - id = 2, title = "No duplicate item", url = "url", visitedAt = 0 ), History.Regular( - id = 3, title = "Duplicate item 1", url = "url", visitedAt = 0 ), History.Regular( - id = 4, title = "Duplicate item 2", url = "url", visitedAt = 0 ), History.Group( - id = 5, title = "Group 5", visitedAt = 0, items = emptyList() ), History.Regular( - id = 6, title = "No duplicate item", url = "url", visitedAt = 0 @@ -496,25 +479,21 @@ class PagedHistoryProviderTest { val expectedList = listOf( History.Group( - id = 1, title = "Group 1", visitedAt = 0, items = emptyList() ), History.Regular( - id = 2, title = "No duplicate item", url = "url", visitedAt = 0 ), History.Group( - id = 5, title = "Group 5", visitedAt = 0, items = emptyList() ), History.Regular( - id = 6, title = "No duplicate item", url = "url", visitedAt = 0 diff --git a/app/src/test/java/org/mozilla/fenix/library/history/HistoryAdapterTest.kt b/app/src/test/java/org/mozilla/fenix/library/history/HistoryAdapterTest.kt index 550f8b6119..8bfe6dc93a 100644 --- a/app/src/test/java/org/mozilla/fenix/library/history/HistoryAdapterTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/history/HistoryAdapterTest.kt @@ -14,7 +14,6 @@ class HistoryAdapterTest { @Test fun `WHEN grouping history item with future date THEN item is grouped to today`() { val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = System.currentTimeMillis() + DateUtils.WEEK_IN_MILLIS @@ -27,7 +26,6 @@ class HistoryAdapterTest { @Test fun `WHEN grouping history item with today's date THEN item is grouped to today`() { val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = System.currentTimeMillis() - DateUtils.MINUTE_IN_MILLIS @@ -46,7 +44,6 @@ class HistoryAdapterTest { } val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = calendar.timeInMillis @@ -65,7 +62,6 @@ class HistoryAdapterTest { } val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = calendar.timeInMillis - DateUtils.HOUR_IN_MILLIS @@ -83,7 +79,6 @@ class HistoryAdapterTest { calendar.set(Calendar.SECOND, 0) val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = calendar.timeInMillis - (DateUtils.HOUR_IN_MILLIS * 23) @@ -102,7 +97,6 @@ class HistoryAdapterTest { } val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = calendar.timeInMillis - (DateUtils.HOUR_IN_MILLIS * 25) @@ -115,7 +109,6 @@ class HistoryAdapterTest { @Test fun `WHEN grouping history item with 3 days ago date THEN item is grouped to this week`() { val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = System.currentTimeMillis() - (DateUtils.DAY_IN_MILLIS * 3) @@ -128,7 +121,6 @@ class HistoryAdapterTest { @Test fun `WHEN grouping history item with 6 days ago date THEN item is grouped to this week`() { val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = System.currentTimeMillis() - (DateUtils.DAY_IN_MILLIS * 6) @@ -141,7 +133,6 @@ class HistoryAdapterTest { @Test fun `WHEN grouping history item with 8 days ago date THEN item is grouped to this month`() { val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = System.currentTimeMillis() - (DateUtils.DAY_IN_MILLIS * 8) @@ -154,7 +145,6 @@ class HistoryAdapterTest { @Test fun `WHEN grouping history item with 29 days ago date THEN item is grouped to this month`() { val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = System.currentTimeMillis() - (DateUtils.DAY_IN_MILLIS * 29) @@ -167,7 +157,6 @@ class HistoryAdapterTest { @Test fun `WHEN grouping history item with 31 days ago date THEN item is grouped to older`() { val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = System.currentTimeMillis() - (DateUtils.DAY_IN_MILLIS * 31) @@ -180,7 +169,6 @@ class HistoryAdapterTest { @Test fun `WHEN grouping history item with zero date THEN item is grouped to older`() { val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = 0 @@ -193,7 +181,6 @@ class HistoryAdapterTest { @Test fun `WHEN grouping history item with negative date THEN item is grouped to older`() { val history = History.Regular( - id = 1, title = "test item", url = "url", visitedAt = -100 diff --git a/app/src/test/java/org/mozilla/fenix/library/history/HistoryDataSourceTest.kt b/app/src/test/java/org/mozilla/fenix/library/history/HistoryDataSourceTest.kt new file mode 100644 index 0000000000..87a469c6e2 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/library/history/HistoryDataSourceTest.kt @@ -0,0 +1,204 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.library.history + +import mozilla.components.concept.storage.HistoryMetadataKey +import org.junit.Assert.assertEquals +import org.junit.Test + +class HistoryDataSourceTest { + private val testCases = listOf( + listOf() to listOf(), + + listOf(1) to listOf( + TestHistory.Regular("http://www.mozilla.com") + ), + listOf(1, 2) to listOf( + TestHistory.Regular("http://www.mozilla.com"), + TestHistory.Regular("http://www.mozilla.com/2") + ), + listOf(1, 2) to listOf( + TestHistory.Metadata("http://www.mozilla.com"), + TestHistory.Regular("http://www.mozilla.com/2") + ), + listOf(1, 2, 3) to listOf( + TestHistory.Metadata("http://www.mozilla.com"), + TestHistory.Regular("http://www.mozilla.com/2"), + TestHistory.Metadata("http://www.mozilla.com"), + ), + listOf(1, 2, 3, 4, 5) to listOf( + TestHistory.Metadata("http://www.mozilla.com"), + TestHistory.Regular("http://www.mozilla.com/2"), + TestHistory.Metadata("http://www.mozilla.com"), + TestHistory.Regular("http://www.mozilla.com/3"), + TestHistory.Regular("http://www.mozilla.com/2"), + ), + listOf(1, 2, 3) to listOf( + TestHistory.Metadata("http://www.mozilla.com"), + TestHistory.Regular("http://www.mozilla.com/2"), + TestHistory.Group("firefox", items = listOf()) + ), + listOf(1, 2, 3) to listOf( + TestHistory.Metadata("http://www.mozilla.com"), + TestHistory.Regular("http://www.mozilla.com/2"), + TestHistory.Group( + "firefox", + items = listOf( + "http://www.firefox.com" + ) + ) + ), + listOf(1, 2, 7) to listOf( + TestHistory.Metadata("http://www.mozilla.com"), + TestHistory.Regular("http://www.mozilla.com/2"), + TestHistory.Group( + "firefox", + items = listOf( + "http://www.firefox.com", + "http://www.firefox.com/2", + "http://www.firefox.com/3", + "http://www.firefox.com/4", + "http://www.firefox.com/5" + ) + ) + ), + listOf(5, 6, 7) to listOf( + TestHistory.Group( + "firefox", + items = listOf( + "http://www.firefox.com", + "http://www.firefox.com/2", + "http://www.firefox.com/3", + "http://www.firefox.com/4", + "http://www.firefox.com/5" + ) + ), + TestHistory.Metadata("http://www.mozilla.com"), + TestHistory.Regular("http://www.mozilla.com/2") + ), + listOf(1, 6, 7) to listOf( + TestHistory.Metadata("http://www.mozilla.com"), + TestHistory.Group( + "firefox", + items = listOf( + "http://www.firefox.com", + "http://www.firefox.com/2", + "http://www.firefox.com/3", + "http://www.firefox.com/4", + "http://www.firefox.com/5" + ) + ), + TestHistory.Regular("http://www.mozilla.com/2") + ), + listOf(1, 6, 8, 9) to listOf( + TestHistory.Metadata("http://www.mozilla.com"), + TestHistory.Group( + "firefox", + items = listOf( + "http://www.firefox.com", + "http://www.firefox.com/2", + "http://www.firefox.com/3", + "http://www.firefox.com/4", + "http://www.firefox.com/5" + ) + ), + TestHistory.Group( + "mdn", + items = listOf( + "https://developer.mozilla.org/en-US/1", + "https://developer.mozilla.org/en-US/2" + ) + ), + TestHistory.Regular("http://www.mozilla.com/2") + ), + listOf(1) to listOf( + TestHistory.Group( + "mozilla", + items = listOf( + "http://www.mozilla.com" + ) + ) + ) + ) + + @Test + fun `assign positions basics - initial offset`() { + testCases.forEach { + verifyPositions(it.first, offset = 0, it.second) + } + } + + @Test + fun `assign position basics - positive offset`() { + val offset = 25 + testCases.forEach { + verifyPositions(it.first.map { pos -> pos + offset }, offset = offset, it.second) + } + } + + @Test + fun `assign position basics - negative offset`() { + // Even though conceptually it doesn't make sense for us to handle negative offsets, + // as far as simple positioning logic is concerned there's no harm in doing the naive thing. + // Assertions around offset being a positive value should happen elsewhere, before we're + // even dealing with positions. + val offset = -25 + testCases.forEach { + verifyPositions(it.first.map { pos -> pos + offset }, offset = offset, it.second) + } + } + + private fun verifyPositions(expectedPositions: List, offset: Int, history: List) { + assertEquals( + "For case $history with offset $offset", + expectedPositions, + HistoryDataSource.assignPositions( + offset = offset, + history.toHistoryWithoutPositions() + ).map { it.position } + ) + } + + private sealed class TestHistory { + data class Regular(val url: String) : TestHistory() + data class Metadata(val url: String) : TestHistory() + data class Group(val title: String, val items: List) : TestHistory() + } + + // For position tests, we just care about the basic tree structure here, + // the details (view times, timestamps, etc) don't matter. + private fun List.toHistoryWithoutPositions(): List { + return this.map { + when (it) { + is TestHistory.Regular -> { + History.Regular(title = it.url, url = it.url, visitedAt = 0) + } + is TestHistory.Metadata -> { + History.Metadata( + title = it.url, + url = it.url, + visitedAt = 0, + totalViewTime = 0, + historyMetadataKey = HistoryMetadataKey(url = it.url) + ) + } + is TestHistory.Group -> { + History.Group( + title = it.title, visitedAt = 0, + items = it.items.map { item -> + History.Metadata( + title = item, + url = item, + visitedAt = 0, + totalViewTime = 0, + historyMetadataKey = HistoryMetadataKey(url = item) + ) + } + ) + } + } + } + } +} diff --git a/app/src/test/java/org/mozilla/fenix/library/historymetadata/HistoryMetadataGroupFragmentStoreTest.kt b/app/src/test/java/org/mozilla/fenix/library/historymetadata/HistoryMetadataGroupFragmentStoreTest.kt index 8552851bb9..87ff396175 100644 --- a/app/src/test/java/org/mozilla/fenix/library/historymetadata/HistoryMetadataGroupFragmentStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/historymetadata/HistoryMetadataGroupFragmentStoreTest.kt @@ -19,7 +19,6 @@ class HistoryMetadataGroupFragmentStoreTest { private lateinit var store: HistoryMetadataGroupFragmentStore private val mozillaHistoryMetadataItem = History.Metadata( - id = 0, title = "Mozilla", url = "mozilla.org", visitedAt = 0, @@ -27,7 +26,6 @@ class HistoryMetadataGroupFragmentStoreTest { historyMetadataKey = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null) ) private val firefoxHistoryMetadataItem = History.Metadata( - id = 0, title = "Firefox", url = "firefox.com", visitedAt = 0, diff --git a/app/src/test/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupControllerTest.kt b/app/src/test/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupControllerTest.kt index 23db70cef6..196c0ab854 100644 --- a/app/src/test/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupControllerTest.kt @@ -53,7 +53,6 @@ class HistoryMetadataGroupControllerTest { private val searchTerm = "mozilla" private val historyMetadataKey = HistoryMetadataKey("http://www.mozilla.com", searchTerm, null) private val mozillaHistoryMetadataItem = History.Metadata( - id = 0, title = "Mozilla", url = "mozilla.org", visitedAt = 0, @@ -61,7 +60,6 @@ class HistoryMetadataGroupControllerTest { historyMetadataKey = historyMetadataKey ) private val firefoxHistoryMetadataItem = History.Metadata( - id = 0, title = "Firefox", url = "firefox.com", visitedAt = 0, diff --git a/app/src/test/java/org/mozilla/fenix/library/historymetadata/view/HistoryMetadataGroupItemViewHolderTest.kt b/app/src/test/java/org/mozilla/fenix/library/historymetadata/view/HistoryMetadataGroupItemViewHolderTest.kt index de0458f099..574b5a8bdf 100644 --- a/app/src/test/java/org/mozilla/fenix/library/historymetadata/view/HistoryMetadataGroupItemViewHolderTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/historymetadata/view/HistoryMetadataGroupItemViewHolderTest.kt @@ -29,7 +29,6 @@ class HistoryMetadataGroupItemViewHolderTest { private lateinit var selectionHolder: SelectionHolder private val item = History.Metadata( - id = 0, title = "Mozilla", url = "mozilla.org", visitedAt = 0,