Merge pull request #7491 from Stypox/fix-search-order

Fix order of local search results

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"),
+        )
+    }
+}

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(
+        database = TestDatabase.createReplacingNewPipeDatabase()
-            ApplicationProvider.getApplicationContext(),
-            AppDatabase::class.java
-        )
-            .allowMainThreadQueries()
-            .build()
-
         manager = LocalPlaylistManager(database)
     }
 

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
+        }
+    }
+}

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<SearchHistoryEntry> {
     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<SearchHistoryEntry> {
     @Override
     Flowable<List<SearchHistoryEntry>> getAll();
 
-    @Query("SELECT * FROM " + TABLE_NAME + " GROUP BY " + SEARCH + ORDER_BY_CREATION_DATE
+    @Query("SELECT " + SEARCH + " FROM " + TABLE_NAME + " GROUP BY " + SEARCH
-            + " LIMIT :limit")
+            + ORDER_BY_MAX_CREATION_DATE + " LIMIT :limit")
-    Flowable<List<SearchHistoryEntry>> getUniqueEntries(int limit);
+    Flowable<List<String>> getUniqueEntries(int limit);
 
     @Query("SELECT * FROM " + TABLE_NAME
             + " WHERE " + SERVICE_ID + " = :serviceId" + ORDER_BY_CREATION_DATE)
     @Override
     Flowable<List<SearchHistoryEntry>> listByService(int serviceId);
 
-    @Query("SELECT * FROM " + TABLE_NAME + " WHERE " + SEARCH + " LIKE :query || '%'"
+    @Query("SELECT " + SEARCH + " FROM " + TABLE_NAME + " WHERE " + SEARCH + " LIKE :query || '%'"
-            + " GROUP BY " + SEARCH + " LIMIT :limit")
+            + " GROUP BY " + SEARCH + ORDER_BY_MAX_CREATION_DATE + " LIMIT :limit")
-    Flowable<List<SearchHistoryEntry>> getSimilarEntries(String query, int limit);
+    Flowable<List<String>> getSimilarEntries(String query, int limit);
 }

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<SearchInfo, ListExtractor.InfoItemsPage<?>>
         implements BackPressable {
     /*//////////////////////////////////////////////////////////////////////////
@@ -743,13 +741,10 @@ public class SearchFragment extends BaseListFragment<SearchInfo, ListExtractor.I
         return historyRecordManager
                 .getRelatedSearches(query, similarQueryLimit, 25)
                 .toObservable()
-                .map(searchHistoryEntries -> {
+                .map(searchHistoryEntries ->
-                    final Set<SuggestionItem> result = new HashSet<>(); // remove duplicates
+                    searchHistoryEntries.stream()
-                    for (final SearchHistoryEntry entry : searchHistoryEntries) {
+                            .map(entry -> new SuggestionItem(true, entry))
-                        result.add(new SuggestionItem(true, entry.getSearch()));
+                            .collect(Collectors.toList()));
-                    }
-                    return new ArrayList<>(result);
-                });
     }
 
     private Observable<List<SuggestionItem>> getRemoteSuggestionsObservable(final String query) {

app/src/main/java/org/schabi/newpipe/local/history/HistoryRecordManager.java

@@ -244,9 +244,9 @@ public class HistoryRecordManager {
                 .subscribeOn(Schedulers.io());
     }
 
-    public Flowable<List<SearchHistoryEntry>> getRelatedSearches(final String query,
+    public Flowable<List<String>> getRelatedSearches(final String query,
-                                                                 final int similarQueryLimit,
+                                                     final int similarQueryLimit,
-                                                                 final int uniqueQueryLimit) {
+                                                     final int uniqueQueryLimit) {
         return query.length() > 0
                 ? searchHistoryTable.getSimilarEntries(query, similarQueryLimit)
                 : searchHistoryTable.getUniqueEntries(uniqueQueryLimit);