From 31c2c7bf129242c92f19092631157919907dfb37 Mon Sep 17 00:00:00 2001 From: Grisha Kruglov Date: Thu, 4 Nov 2021 00:06:55 -0700 Subject: [PATCH] [fenix] No issue: history view search group missing Our boundary conditions for matching search groups to visits was wrong. This change switches the boundary buffer to only be applied to history items, not the metadata items. In other words, when checking if any of the metadata items match the current "page" of history, we'll be looking to see if the item falls within this time window: buffer - oldest history item <= metadata item <= newest history item + buffer There's a separate problem with buffer though: it's reset to 0 when requested offset is >0, but that requires a larger refactor of this code, for a separate PR. --- .../history/PagedHistoryProvider.kt | 4 +- .../history/PagedHistoryProviderTest.kt | 146 ++++++++++++++++++ 2 files changed, 148 insertions(+), 2 deletions(-) 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 7069fecc1c..eb78ff0915 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 @@ -156,8 +156,8 @@ class DefaultPagedHistoryProvider( val historyGroupsInOffset = if (history.isNotEmpty()) { historyGroups?.filter { it.items.any { item -> - history.last().visitedAt <= item.visitedAt - visitedAtBuffer && - item.visitedAt - visitedAtBuffer <= (history.first().visitedAt + visitedAtBuffer) + (history.last().visitedAt - visitedAtBuffer) <= item.visitedAt && + item.visitedAt <= (history.first().visitedAt + visitedAtBuffer) } } ?: emptyList() } else { 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 32a041c154..5b04b16b39 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 @@ -146,4 +146,150 @@ class PagedHistoryProviderTest { ) assertEquals(results, actualResults) } + + @Test + fun `history metadata matching lower bound`() { + val provider = DefaultPagedHistoryProvider( + historyStorage = storage, + showHistorySearchGroups = true + ) + // Oldest history visit on the page is 15 seconds (buffer time) newer than matching + // metadata record. + val visitInfo1 = VisitInfo( + url = "http://www.mozilla.com", + title = "mozilla", + visitTime = 25000, + visitType = VisitType.LINK, + previewImageUrl = null + ) + + val historyMetadataKey1 = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null) + val historyEntry1 = HistoryMetadata( + key = historyMetadataKey1, + title = "mozilla", + createdAt = 10000, + updatedAt = 10, + totalViewTime = 10, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + + coEvery { storage.getVisitsPaginated(any(), any(), any()) } returns listOf(visitInfo1) + coEvery { storage.getHistoryMetadataSince(any()) } returns listOf(historyEntry1) + + var actualResults: List? = null + provider.getHistory(0L, 5) { + actualResults = it + } + + coVerify { + storage.getVisitsPaginated( + offset = 0L, + count = 5, + excludeTypes = listOf( + VisitType.NOT_A_VISIT, + VisitType.DOWNLOAD, + VisitType.REDIRECT_TEMPORARY, + VisitType.RELOAD, + VisitType.EMBED, + VisitType.FRAMED_LINK, + VisitType.REDIRECT_PERMANENT + ) + ) + } + + 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, + totalViewTime = historyEntry1.totalViewTime, + historyMetadataKey = historyMetadataKey1 + ) + ) + ) + ) + + assertEquals(results, actualResults) + } + + @Test + fun `history metadata matching upper bound`() { + val provider = DefaultPagedHistoryProvider( + historyStorage = storage, + showHistorySearchGroups = true + ) + // Newest history visit on the page is 15 seconds (buffer time) older than matching + // metadata record. + val visitInfo1 = VisitInfo( + url = "http://www.mozilla.com", + title = "mozilla", + visitTime = 10000, + visitType = VisitType.LINK, + previewImageUrl = null + ) + + val historyMetadataKey1 = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null) + val historyEntry1 = HistoryMetadata( + key = historyMetadataKey1, + title = "mozilla", + createdAt = 25000, + updatedAt = 10, + totalViewTime = 10, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + + coEvery { storage.getVisitsPaginated(any(), any(), any()) } returns listOf(visitInfo1) + coEvery { storage.getHistoryMetadataSince(any()) } returns listOf(historyEntry1) + + var actualResults: List? = null + provider.getHistory(0L, 5) { + actualResults = it + } + + coVerify { + storage.getVisitsPaginated( + offset = 0L, + count = 5, + excludeTypes = listOf( + VisitType.NOT_A_VISIT, + VisitType.DOWNLOAD, + VisitType.REDIRECT_TEMPORARY, + VisitType.RELOAD, + VisitType.EMBED, + VisitType.FRAMED_LINK, + VisitType.REDIRECT_PERMANENT + ) + ) + } + + 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, + totalViewTime = historyEntry1.totalViewTime, + historyMetadataKey = historyMetadataKey1 + ) + ) + ) + ) + + assertEquals(results, actualResults) + } }