From 8a5e2ffa579beb8f14d60455f1a9e43d19deb704 Mon Sep 17 00:00:00 2001 From: Stypox Date: Sat, 4 Dec 2021 10:22:17 +0100 Subject: [PATCH 1/3] Fix order of local search results --- .../history/dao/SearchHistoryDAO.java | 13 +++++----- .../fragments/list/search/SearchFragment.java | 25 ++++++++----------- .../local/history/HistoryRecordManager.java | 6 ++--- 3 files changed, 20 insertions(+), 24 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/database/history/dao/SearchHistoryDAO.java b/app/src/main/java/org/schabi/newpipe/database/history/dao/SearchHistoryDAO.java index a0010a419..8a281bdb4 100644 --- a/app/src/main/java/org/schabi/newpipe/database/history/dao/SearchHistoryDAO.java +++ b/app/src/main/java/org/schabi/newpipe/database/history/dao/SearchHistoryDAO.java @@ -19,6 +19,7 @@ import static org.schabi.newpipe.database.history.model.SearchHistoryEntry.TABLE @Dao public interface SearchHistoryDAO extends HistoryDAO { String ORDER_BY_CREATION_DATE = " ORDER BY " + CREATION_DATE + " DESC"; + String ORDER_BY_MAX_CREATION_DATE = " ORDER BY MAX(" + CREATION_DATE + ") DESC"; @Query("SELECT * FROM " + TABLE_NAME + " WHERE " + ID + " = (SELECT MAX(" + ID + ") FROM " + TABLE_NAME + ")") @@ -36,16 +37,16 @@ public interface SearchHistoryDAO extends HistoryDAO { @Override Flowable> getAll(); - @Query("SELECT * FROM " + TABLE_NAME + " GROUP BY " + SEARCH + ORDER_BY_CREATION_DATE - + " LIMIT :limit") - Flowable> getUniqueEntries(int limit); + @Query("SELECT " + SEARCH + " FROM " + TABLE_NAME + " GROUP BY " + SEARCH + + ORDER_BY_MAX_CREATION_DATE + " LIMIT :limit") + Flowable> getUniqueEntries(int limit); @Query("SELECT * FROM " + TABLE_NAME + " WHERE " + SERVICE_ID + " = :serviceId" + ORDER_BY_CREATION_DATE) @Override Flowable> listByService(int serviceId); - @Query("SELECT * FROM " + TABLE_NAME + " WHERE " + SEARCH + " LIKE :query || '%'" - + " GROUP BY " + SEARCH + " LIMIT :limit") - Flowable> getSimilarEntries(String query, int limit); + @Query("SELECT " + SEARCH + " FROM " + TABLE_NAME + " WHERE " + SEARCH + " LIKE :query || '%'" + + " GROUP BY " + SEARCH + ORDER_BY_MAX_CREATION_DATE + " LIMIT :limit") + Flowable> getSimilarEntries(String query, int limit); } diff --git a/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java index d4d73f74f..3f199f411 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java @@ -1,5 +1,10 @@ package org.schabi.newpipe.fragments.list.search; +import static androidx.recyclerview.widget.ItemTouchHelper.Callback.makeMovementFlags; +import static org.schabi.newpipe.ktx.ViewUtils.animate; +import static org.schabi.newpipe.util.ExtractorHelper.showMetaInfoInTextView; +import static java.util.Arrays.asList; + import android.app.Activity; import android.content.Context; import android.content.Intent; @@ -36,7 +41,6 @@ import androidx.recyclerview.widget.ItemTouchHelper; import androidx.recyclerview.widget.RecyclerView; import org.schabi.newpipe.R; -import org.schabi.newpipe.database.history.model.SearchHistoryEntry; import org.schabi.newpipe.databinding.FragmentSearchBinding; import org.schabi.newpipe.error.ErrorActivity; import org.schabi.newpipe.error.ErrorInfo; @@ -68,12 +72,11 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Queue; -import java.util.Set; import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; import icepick.State; import io.reactivex.rxjava3.android.schedulers.AndroidSchedulers; @@ -84,11 +87,6 @@ import io.reactivex.rxjava3.disposables.Disposable; import io.reactivex.rxjava3.schedulers.Schedulers; import io.reactivex.rxjava3.subjects.PublishSubject; -import static androidx.recyclerview.widget.ItemTouchHelper.Callback.makeMovementFlags; -import static java.util.Arrays.asList; -import static org.schabi.newpipe.ktx.ViewUtils.animate; -import static org.schabi.newpipe.util.ExtractorHelper.showMetaInfoInTextView; - public class SearchFragment extends BaseListFragment> implements BackPressable { /*////////////////////////////////////////////////////////////////////////// @@ -743,13 +741,10 @@ public class SearchFragment extends BaseListFragment { - final Set result = new HashSet<>(); // remove duplicates - for (final SearchHistoryEntry entry : searchHistoryEntries) { - result.add(new SuggestionItem(true, entry.getSearch())); - } - return new ArrayList<>(result); - }); + .map(searchHistoryEntries -> + searchHistoryEntries.stream() + .map(entry -> new SuggestionItem(true, entry)) + .collect(Collectors.toList())); } private Observable> getRemoteSuggestionsObservable(final String query) { diff --git a/app/src/main/java/org/schabi/newpipe/local/history/HistoryRecordManager.java b/app/src/main/java/org/schabi/newpipe/local/history/HistoryRecordManager.java index d94088cd0..45445cf58 100644 --- a/app/src/main/java/org/schabi/newpipe/local/history/HistoryRecordManager.java +++ b/app/src/main/java/org/schabi/newpipe/local/history/HistoryRecordManager.java @@ -244,9 +244,9 @@ public class HistoryRecordManager { .subscribeOn(Schedulers.io()); } - public Flowable> getRelatedSearches(final String query, - final int similarQueryLimit, - final int uniqueQueryLimit) { + public Flowable> getRelatedSearches(final String query, + final int similarQueryLimit, + final int uniqueQueryLimit) { return query.length() > 0 ? searchHistoryTable.getSimilarEntries(query, similarQueryLimit) : searchHistoryTable.getUniqueEntries(uniqueQueryLimit); From 7d6688f4976d1c998938843f5c29d5db8c0ea7fc Mon Sep 17 00:00:00 2001 From: Stypox Date: Wed, 8 Dec 2021 14:02:16 +0100 Subject: [PATCH 2/3] Add DatabaseMocker to mock NewPipeDatabase --- .../playlist/LocalPlaylistManagerTest.kt | 13 ++------ .../schabi/newpipe/testUtil/TestDatabase.kt | 32 +++++++++++++++++++ 2 files changed, 35 insertions(+), 10 deletions(-) create mode 100644 app/src/androidTest/java/org/schabi/newpipe/testUtil/TestDatabase.kt diff --git a/app/src/androidTest/java/org/schabi/newpipe/local/playlist/LocalPlaylistManagerTest.kt b/app/src/androidTest/java/org/schabi/newpipe/local/playlist/LocalPlaylistManagerTest.kt index 0a00b0443..249492d8f 100644 --- a/app/src/androidTest/java/org/schabi/newpipe/local/playlist/LocalPlaylistManagerTest.kt +++ b/app/src/androidTest/java/org/schabi/newpipe/local/playlist/LocalPlaylistManagerTest.kt @@ -1,7 +1,5 @@ package org.schabi.newpipe.local.playlist -import androidx.room.Room -import androidx.test.core.app.ApplicationProvider import org.junit.After import org.junit.Before import org.junit.Rule @@ -10,6 +8,7 @@ import org.junit.rules.Timeout import org.schabi.newpipe.database.AppDatabase import org.schabi.newpipe.database.stream.model.StreamEntity import org.schabi.newpipe.extractor.stream.StreamType +import org.schabi.newpipe.testUtil.TestDatabase import org.schabi.newpipe.testUtil.TrampolineSchedulerRule import java.util.concurrent.TimeUnit @@ -22,17 +21,11 @@ class LocalPlaylistManagerTest { val trampolineScheduler = TrampolineSchedulerRule() @get:Rule - val timeout = Timeout(10, TimeUnit.SECONDS) + val timeout = Timeout(1, TimeUnit.SECONDS) @Before fun setup() { - database = Room.inMemoryDatabaseBuilder( - ApplicationProvider.getApplicationContext(), - AppDatabase::class.java - ) - .allowMainThreadQueries() - .build() - + database = TestDatabase.createReplacingNewPipeDatabase() manager = LocalPlaylistManager(database) } diff --git a/app/src/androidTest/java/org/schabi/newpipe/testUtil/TestDatabase.kt b/app/src/androidTest/java/org/schabi/newpipe/testUtil/TestDatabase.kt new file mode 100644 index 000000000..8b1a261be --- /dev/null +++ b/app/src/androidTest/java/org/schabi/newpipe/testUtil/TestDatabase.kt @@ -0,0 +1,32 @@ +package org.schabi.newpipe.testUtil + +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import org.junit.Assert.assertSame +import org.schabi.newpipe.NewPipeDatabase +import org.schabi.newpipe.database.AppDatabase + +class TestDatabase { + companion object { + fun createReplacingNewPipeDatabase(): AppDatabase { + val database = Room.inMemoryDatabaseBuilder( + ApplicationProvider.getApplicationContext(), + AppDatabase::class.java + ) + .allowMainThreadQueries() + .build() + + val databaseField = NewPipeDatabase::class.java.getDeclaredField("databaseInstance") + databaseField.isAccessible = true + databaseField.set(NewPipeDatabase::class, database) + + assertSame( + "Mocking database failed!", + database, + NewPipeDatabase.getInstance(ApplicationProvider.getApplicationContext()) + ) + + return database + } + } +} From 2963cd5c6e167640f790ceafafee9f4312fa0773 Mon Sep 17 00:00:00 2001 From: Stypox Date: Wed, 8 Dec 2021 14:12:09 +0100 Subject: [PATCH 3/3] Add HistoryRecordManagerTest --- .../local/history/HistoryRecordManagerTest.kt | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 app/src/androidTest/java/org/schabi/newpipe/local/history/HistoryRecordManagerTest.kt diff --git a/app/src/androidTest/java/org/schabi/newpipe/local/history/HistoryRecordManagerTest.kt b/app/src/androidTest/java/org/schabi/newpipe/local/history/HistoryRecordManagerTest.kt new file mode 100644 index 000000000..3f3a038d8 --- /dev/null +++ b/app/src/androidTest/java/org/schabi/newpipe/local/history/HistoryRecordManagerTest.kt @@ -0,0 +1,153 @@ +package org.schabi.newpipe.local.history + +import androidx.test.core.app.ApplicationProvider +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.rules.Timeout +import org.schabi.newpipe.database.AppDatabase +import org.schabi.newpipe.database.history.model.SearchHistoryEntry +import org.schabi.newpipe.testUtil.TestDatabase +import org.schabi.newpipe.testUtil.TrampolineSchedulerRule +import java.time.OffsetDateTime +import java.util.concurrent.TimeUnit + +class HistoryRecordManagerTest { + + private lateinit var manager: HistoryRecordManager + private lateinit var database: AppDatabase + + @get:Rule + val trampolineScheduler = TrampolineSchedulerRule() + + @get:Rule + val timeout = Timeout(1, TimeUnit.SECONDS) + + @Before + fun setup() { + database = TestDatabase.createReplacingNewPipeDatabase() + manager = HistoryRecordManager(ApplicationProvider.getApplicationContext()) + } + + @After + fun cleanUp() { + database.close() + } + + @Test + fun onSearched() { + manager.onSearched(0, "Hello").test().await().assertValue(1) + + // For some reason the Flowable returned by getAll() never completes, so we can't assert + // that the number of Lists it returns is exactly 1, we can only check if the first List is + // correct. Why on earth has a Flowable been used instead of a Single for getAll()?!? + val entities = database.searchHistoryDAO().all.blockingFirst() + assertEquals(1, entities.size) + assertEquals(1, entities[0].id) + assertEquals(0, entities[0].serviceId) + assertEquals("Hello", entities[0].search) + } + + @Test + fun deleteSearchHistory() { + val entries = listOf( + SearchHistoryEntry(OffsetDateTime.now(), 0, "A"), + SearchHistoryEntry(OffsetDateTime.now(), 2, "A"), + SearchHistoryEntry(OffsetDateTime.now(), 1, "B"), + SearchHistoryEntry(OffsetDateTime.now(), 0, "B"), + ) + + // make sure all 4 were inserted + database.searchHistoryDAO().insertAll(entries) + assertEquals(entries.size, database.searchHistoryDAO().all.blockingFirst().size) + + // try to delete only "A" entries, "B" entries should be untouched + manager.deleteSearchHistory("A").test().await().assertValue(2) + val entities = database.searchHistoryDAO().all.blockingFirst() + assertEquals(2, entities.size) + assertTrue(entries[2].hasEqualValues(entities[0])) + assertTrue(entries[3].hasEqualValues(entities[1])) + + // assert that nothing happens if we delete a search query that does exist in the db + manager.deleteSearchHistory("A").test().await().assertValue(0) + val entities2 = database.searchHistoryDAO().all.blockingFirst() + assertEquals(2, entities2.size) + assertTrue(entries[2].hasEqualValues(entities2[0])) + assertTrue(entries[3].hasEqualValues(entities2[1])) + + // delete all remaining entries + manager.deleteSearchHistory("B").test().await().assertValue(2) + assertEquals(0, database.searchHistoryDAO().all.blockingFirst().size) + } + + @Test + fun deleteCompleteSearchHistory() { + val entries = listOf( + SearchHistoryEntry(OffsetDateTime.now(), 1, "A"), + SearchHistoryEntry(OffsetDateTime.now(), 2, "B"), + SearchHistoryEntry(OffsetDateTime.now(), 0, "C"), + ) + + // make sure all 3 were inserted + database.searchHistoryDAO().insertAll(entries) + assertEquals(entries.size, database.searchHistoryDAO().all.blockingFirst().size) + + // should remove everything + manager.deleteCompleteSearchHistory().test().await().assertValue(entries.size) + assertEquals(0, database.searchHistoryDAO().all.blockingFirst().size) + } + + @Test + fun getRelatedSearches_emptyQuery() { + // make sure all entries were inserted + database.searchHistoryDAO().insertAll(RELATED_SEARCHES_ENTRIES) + assertEquals( + RELATED_SEARCHES_ENTRIES.size, + database.searchHistoryDAO().all.blockingFirst().size + ) + + // make sure correct number of searches is returned and in correct order + val searches = manager.getRelatedSearches("", 6, 4).blockingFirst() + assertEquals(4, searches.size) + assertEquals(RELATED_SEARCHES_ENTRIES[6].search, searches[0]) // A (even if in two places) + assertEquals(RELATED_SEARCHES_ENTRIES[4].search, searches[1]) // B + assertEquals(RELATED_SEARCHES_ENTRIES[5].search, searches[2]) // AA + assertEquals(RELATED_SEARCHES_ENTRIES[2].search, searches[3]) // BA + } + + @Test + fun getRelatedSearched_nonEmptyQuery() { + // make sure all entries were inserted + database.searchHistoryDAO().insertAll(RELATED_SEARCHES_ENTRIES) + assertEquals( + RELATED_SEARCHES_ENTRIES.size, + database.searchHistoryDAO().all.blockingFirst().size + ) + + // make sure correct number of searches is returned and in correct order + val searches = manager.getRelatedSearches("A", 3, 5).blockingFirst() + assertEquals(3, searches.size) + assertEquals(RELATED_SEARCHES_ENTRIES[6].search, searches[0]) // A (even if in two places) + assertEquals(RELATED_SEARCHES_ENTRIES[5].search, searches[1]) // AA + assertEquals(RELATED_SEARCHES_ENTRIES[1].search, searches[2]) // BA + + // also make sure that the string comparison is case insensitive + val searches2 = manager.getRelatedSearches("a", 3, 5).blockingFirst() + assertEquals(searches, searches2) + } + + companion object { + val RELATED_SEARCHES_ENTRIES = listOf( + SearchHistoryEntry(OffsetDateTime.now().minusSeconds(7), 2, "AC"), + SearchHistoryEntry(OffsetDateTime.now().minusSeconds(6), 0, "ABC"), + SearchHistoryEntry(OffsetDateTime.now().minusSeconds(5), 1, "BA"), + SearchHistoryEntry(OffsetDateTime.now().minusSeconds(4), 3, "A"), + SearchHistoryEntry(OffsetDateTime.now().minusSeconds(2), 0, "B"), + SearchHistoryEntry(OffsetDateTime.now().minusSeconds(3), 2, "AA"), + SearchHistoryEntry(OffsetDateTime.now().minusSeconds(1), 1, "A"), + ) + } +}