From 683ccdf97bd8b3ed30b7014f2df17227cff92561 Mon Sep 17 00:00:00 2001 From: Soroosh Mariooryad Date: Thu, 25 May 2017 14:15:00 -0700 Subject: [PATCH] Move static search ranking from DatabaseResultLoader to Search Adapter. This will avoid unnecessary static ranking if smart ranking is used. Since loader does not need to provided sorted collection of results, the loading data type has changed from List<> to Set<>. This will also faster lookup in the Adapter. Fixes: 38447799 Test: make RunSettingsRoboTests Change-Id: I448b29bd4e8700c8ec4b5766cbeab4b3087ae39a --- .../settings/search/DatabaseResultLoader.java | 30 +++++++--------- .../search/InstalledAppResultLoader.java | 13 ++++--- .../settings/search/SearchFragment.java | 11 +++--- .../settings/search/SearchResultsAdapter.java | 28 ++++++++++----- .../search/DatabaseResultLoaderTest.java | 17 +++++---- .../search/InstalledAppResultLoaderTest.java | 17 +++++---- .../settings/search/MockAppLoader.java | 10 +++--- .../android/settings/search/MockDBLoader.java | 10 +++--- .../settings/search/SearchFragmentTest.java | 4 +-- .../search/SearchResultsAdapterTest.java | 36 ++++++++++++------- 10 files changed, 100 insertions(+), 76 deletions(-) diff --git a/src/com/android/settings/search/DatabaseResultLoader.java b/src/com/android/settings/search/DatabaseResultLoader.java index 03f5cb43a43..26bfd52716f 100644 --- a/src/com/android/settings/search/DatabaseResultLoader.java +++ b/src/com/android/settings/search/DatabaseResultLoader.java @@ -25,10 +25,7 @@ import android.text.TextUtils; import com.android.settings.dashboard.SiteMapManager; import com.android.settings.utils.AsyncLoader; -import java.util.ArrayList; -import java.util.Collections; import java.util.HashSet; -import java.util.List; import java.util.Set; import static com.android.settings.search.IndexDatabaseHelper.IndexColumns; @@ -37,7 +34,7 @@ import static com.android.settings.search.IndexDatabaseHelper.Tables.TABLE_PREFS /** * AsyncTask to retrieve Settings, First party app and any intent based results. */ -public class DatabaseResultLoader extends AsyncLoader> { +public class DatabaseResultLoader extends AsyncLoader> { private static final String LOG = "DatabaseResultLoader"; /* These indices are used to match the columns of the this loader's SELECT statement. @@ -114,25 +111,22 @@ public class DatabaseResultLoader extends AsyncLoader result) { + protected void onDiscardResult(Set result) { // TODO Search } @Override - public List loadInBackground() { + public Set loadInBackground() { if (mQueryText == null || mQueryText.isEmpty()) { return null; } - final Set resultSet = new HashSet<>(); + final Set results = new HashSet<>(); - resultSet.addAll(firstWordQuery(MATCH_COLUMNS_PRIMARY, BASE_RANKS[0])); - resultSet.addAll(secondaryWordQuery(MATCH_COLUMNS_PRIMARY, BASE_RANKS[1])); - resultSet.addAll(anyWordQuery(MATCH_COLUMNS_SECONDARY, BASE_RANKS[2])); - resultSet.addAll(anyWordQuery(MATCH_COLUMNS_TERTIARY, BASE_RANKS[3])); - - final List results = new ArrayList<>(resultSet); - Collections.sort(results); + results.addAll(firstWordQuery(MATCH_COLUMNS_PRIMARY, BASE_RANKS[0])); + results.addAll(secondaryWordQuery(MATCH_COLUMNS_PRIMARY, BASE_RANKS[1])); + results.addAll(anyWordQuery(MATCH_COLUMNS_SECONDARY, BASE_RANKS[2])); + results.addAll(anyWordQuery(MATCH_COLUMNS_TERTIARY, BASE_RANKS[3])); return results; } @@ -159,7 +153,7 @@ public class DatabaseResultLoader extends AsyncLoader firstWordQuery(String[] matchColumns, int baseRank) { final String whereClause = buildSingleWordWhereClause(matchColumns); @@ -175,7 +169,7 @@ public class DatabaseResultLoader extends AsyncLoader secondaryWordQuery(String[] matchColumns, int baseRank) { final String whereClause = buildSingleWordWhereClause(matchColumns); @@ -190,7 +184,7 @@ public class DatabaseResultLoader extends AsyncLoader anyWordQuery(String[] matchColumns, int baseRank) { final String whereClause = buildTwoWordWhereClause(matchColumns); @@ -205,7 +199,7 @@ public class DatabaseResultLoader extends AsyncLoader query(String whereClause, String[] selection, int baseRank) { SQLiteDatabase database = IndexDatabaseHelper.getInstance(mContext).getReadableDatabase(); diff --git a/src/com/android/settings/search/InstalledAppResultLoader.java b/src/com/android/settings/search/InstalledAppResultLoader.java index 76f3a00240f..25a3e8913d4 100644 --- a/src/com/android/settings/search/InstalledAppResultLoader.java +++ b/src/com/android/settings/search/InstalledAppResultLoader.java @@ -37,14 +37,14 @@ import com.android.settings.applications.PackageManagerWrapper; import com.android.settings.dashboard.SiteMapManager; import com.android.settings.utils.AsyncLoader; -import java.util.ArrayList; -import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Set; /** * Search loader for installed apps. */ -public class InstalledAppResultLoader extends AsyncLoader> { +public class InstalledAppResultLoader extends AsyncLoader> { private static final int NAME_NO_MATCH = -1; private static final Intent LAUNCHER_PROBE = new Intent(Intent.ACTION_MAIN) @@ -67,8 +67,8 @@ public class InstalledAppResultLoader extends AsyncLoader loadInBackground() { - final List results = new ArrayList<>(); + public Set loadInBackground() { + final Set results = new HashSet<>(); final PackageManager pm = mPackageManager.getPackageManager(); for (UserInfo user : getUsersToCount()) { @@ -103,7 +103,6 @@ public class InstalledAppResultLoader extends AsyncLoader result) { + protected void onDiscardResult(Set result) { } diff --git a/src/com/android/settings/search/SearchFragment.java b/src/com/android/settings/search/SearchFragment.java index e50558bad6e..00eb591a273 100644 --- a/src/com/android/settings/search/SearchFragment.java +++ b/src/com/android/settings/search/SearchFragment.java @@ -44,6 +44,7 @@ import com.android.settings.core.instrumentation.MetricsFeatureProvider; import com.android.settings.overlay.FeatureFactory; import java.util.List; +import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; /** @@ -57,7 +58,7 @@ import java.util.concurrent.atomic.AtomicInteger; * the query if the user has entered text. */ public class SearchFragment extends InstrumentedFragment implements SearchView.OnQueryTextListener, - LoaderManager.LoaderCallbacks>, IndexingCallback { + LoaderManager.LoaderCallbacks>, IndexingCallback { private static final String TAG = "SearchFragment"; @VisibleForTesting @@ -251,7 +252,7 @@ public class SearchFragment extends InstrumentedFragment implements SearchView.O } @Override - public Loader> onCreateLoader(int id, Bundle args) { + public Loader> onCreateLoader(int id, Bundle args) { final Activity activity = getActivity(); switch (id) { @@ -265,8 +266,8 @@ public class SearchFragment extends InstrumentedFragment implements SearchView.O } @Override - public void onLoadFinished(Loader> loader, - List data) { + public void onLoadFinished(Loader> loader, + Set data) { mSearchAdapter.addSearchResults(data, loader.getClass().getName()); if (mUnfinishedLoadersCount.decrementAndGet() != 0) { return; @@ -284,7 +285,7 @@ public class SearchFragment extends InstrumentedFragment implements SearchView.O } @Override - public void onLoaderReset(Loader> loader) { + public void onLoaderReset(Loader> loader) { } /** diff --git a/src/com/android/settings/search/SearchResultsAdapter.java b/src/com/android/settings/search/SearchResultsAdapter.java index 7861b0832ab..31e07933a7f 100644 --- a/src/com/android/settings/search/SearchResultsAdapter.java +++ b/src/com/android/settings/search/SearchResultsAdapter.java @@ -30,15 +30,17 @@ import android.view.ViewGroup; import com.android.settings.R; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Set; public class SearchResultsAdapter extends RecyclerView.Adapter { private final SearchFragment mFragment; private List mSearchResults; - private Map> mResultsMap; + private Map> mResultsMap; private final SearchFeatureProvider mSearchFeatureProvider; public SearchResultsAdapter(SearchFragment fragment, @@ -98,7 +100,7 @@ public class SearchResultsAdapter extends RecyclerView.Adapter * @param loaderClassName class name of the loader. */ @MainThread - public void addSearchResults(List results, String loaderClassName) { + public void addSearchResults(Set results, String loaderClassName) { if (results == null) { return; } @@ -125,12 +127,22 @@ public class SearchResultsAdapter extends RecyclerView.Adapter * @return Number of matched results */ public int displaySearchResults(String query) { - final List databaseResults = mResultsMap - .get(DatabaseResultLoader.class.getName()); - final List installedAppResults = mResultsMap - .get(InstalledAppResultLoader.class.getName()); - final int dbSize = (databaseResults != null) ? databaseResults.size() : 0; - final int appSize = (installedAppResults != null) ? installedAppResults.size() : 0; + List databaseResults = null; + List installedAppResults = null; + final String dbLoaderKey = DatabaseResultLoader.class.getName(); + final String appLoaderKey = InstalledAppResultLoader.class.getName(); + int dbSize = 0; + int appSize = 0; + if (mResultsMap.containsKey(dbLoaderKey)) { + databaseResults = new ArrayList<>(mResultsMap.get(dbLoaderKey)); + dbSize = databaseResults.size(); + Collections.sort(databaseResults); + } + if (mResultsMap.containsKey(appLoaderKey)) { + installedAppResults = new ArrayList<>(mResultsMap.get(appLoaderKey)); + appSize = installedAppResults.size(); + Collections.sort(installedAppResults); + } final List newResults = new ArrayList<>(dbSize + appSize); int dbIndex = 0; diff --git a/tests/robotests/src/com/android/settings/search/DatabaseResultLoaderTest.java b/tests/robotests/src/com/android/settings/search/DatabaseResultLoaderTest.java index bcd33715c08..3d38a2745c6 100644 --- a/tests/robotests/src/com/android/settings/search/DatabaseResultLoaderTest.java +++ b/tests/robotests/src/com/android/settings/search/DatabaseResultLoaderTest.java @@ -45,7 +45,10 @@ import org.mockito.MockitoAnnotations; import org.robolectric.RuntimeEnvironment; import org.robolectric.annotation.Config; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; +import java.util.Set; import static com.google.common.truth.Truth.assertThat; import static org.mockito.Matchers.anyString; @@ -224,17 +227,19 @@ public class DatabaseResultLoaderTest { } @Test - public void testSpecialCaseTwoWords_firstWordMatches_ranksHigher() { + public void testSpecialCaseTwoWords_multipleResults() { final String caseOne = "Apple pear"; final String caseTwo = "Banana apple"; insertSpecialCase(caseOne); insertSpecialCase(caseTwo); DatabaseResultLoader loader = new DatabaseResultLoader(mContext, "App", null); - List results = loader.loadInBackground(); - - assertThat(results.get(0).title).isEqualTo(caseOne); - assertThat(results.get(1).title).isEqualTo(caseTwo); - assertThat(results.get(0).rank).isLessThan(results.get(1).rank); + Set results = loader.loadInBackground(); + Set expectedTitles = new HashSet<>(Arrays.asList(caseOne, caseTwo)); + Set actualTitles = new HashSet<>(); + for (SearchResult result : results) { + actualTitles.add(result.title); + } + assertThat(actualTitles).isEqualTo(expectedTitles); } private void insertSpecialCase(String specialCase) { diff --git a/tests/robotests/src/com/android/settings/search/InstalledAppResultLoaderTest.java b/tests/robotests/src/com/android/settings/search/InstalledAppResultLoaderTest.java index e1670919574..1a30157b73f 100644 --- a/tests/robotests/src/com/android/settings/search/InstalledAppResultLoaderTest.java +++ b/tests/robotests/src/com/android/settings/search/InstalledAppResultLoaderTest.java @@ -43,7 +43,9 @@ import org.robolectric.annotation.Config; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashSet; import java.util.List; +import java.util.Set; import static android.content.pm.ApplicationInfo.FLAG_SYSTEM; import static android.content.pm.ApplicationInfo.FLAG_UPDATED_SYSTEM_APP; @@ -185,18 +187,19 @@ public class InstalledAppResultLoaderTest { } @Test - public void query_matchingQuery_shouldRankBasedOnSimilarity() { + public void query_matchingQuery_multipleResults() { final String query = "app"; mLoader = new InstalledAppResultLoader(mContext, mPackageManagerWrapper, query, mSiteMapManager); - final List results = mLoader.loadInBackground(); + final Set results = mLoader.loadInBackground(); - // List is sorted by rank - assertThat(results.get(0).rank).isAtMost(results.get(1).rank); - assertThat(results.get(0).title).isEqualTo("app4"); - assertThat(results.get(1).title).isEqualTo("app"); - assertThat(results.get(2).title).isEqualTo("appBuffer"); + Set expectedTitles = new HashSet<>(Arrays.asList("app4", "app", "appBuffer")); + Set actualTitles = new HashSet<>(); + for (SearchResult result : results) { + actualTitles.add(result.title); + } + assertThat(actualTitles).isEqualTo(expectedTitles); } @Test diff --git a/tests/robotests/src/com/android/settings/search/MockAppLoader.java b/tests/robotests/src/com/android/settings/search/MockAppLoader.java index 35e56a15d31..c68cbdf623d 100644 --- a/tests/robotests/src/com/android/settings/search/MockAppLoader.java +++ b/tests/robotests/src/com/android/settings/search/MockAppLoader.java @@ -21,8 +21,8 @@ import android.content.Context; import com.android.settings.search.InstalledAppResultLoader; import com.android.settings.search.SearchResult; -import java.util.ArrayList; -import java.util.List; +import java.util.HashSet; +import java.util.Set; /** * Mock loader to subvert the requirements of returning data while also driving the Loader @@ -35,12 +35,12 @@ class MockAppLoader extends InstalledAppResultLoader { } @Override - public List loadInBackground() { - return new ArrayList<>(); + public Set loadInBackground() { + return new HashSet<>(); } @Override - protected void onDiscardResult(List result) { + protected void onDiscardResult(Set result) { } } diff --git a/tests/robotests/src/com/android/settings/search/MockDBLoader.java b/tests/robotests/src/com/android/settings/search/MockDBLoader.java index 562e3751e15..b28c1ed97f6 100644 --- a/tests/robotests/src/com/android/settings/search/MockDBLoader.java +++ b/tests/robotests/src/com/android/settings/search/MockDBLoader.java @@ -21,8 +21,8 @@ import android.content.Context; import com.android.settings.search.DatabaseResultLoader; import com.android.settings.search.SearchResult; -import java.util.ArrayList; -import java.util.List; +import java.util.HashSet; +import java.util.Set; /** * Mock loader to subvert the requirements of returning data while also driving the Loader @@ -35,12 +35,12 @@ class MockDBLoader extends DatabaseResultLoader { } @Override - public List loadInBackground() { - return new ArrayList<>(); + public Set loadInBackground() { + return new HashSet<>(); } @Override - protected void onDiscardResult(List result) { + protected void onDiscardResult(Set result) { } } diff --git a/tests/robotests/src/com/android/settings/search/SearchFragmentTest.java b/tests/robotests/src/com/android/settings/search/SearchFragmentTest.java index 94dc2332c94..b7154cec83b 100644 --- a/tests/robotests/src/com/android/settings/search/SearchFragmentTest.java +++ b/tests/robotests/src/com/android/settings/search/SearchFragmentTest.java @@ -41,7 +41,7 @@ import org.robolectric.annotation.Config; import org.robolectric.util.ActivityController; import org.robolectric.util.ReflectionHelpers; -import java.util.List; +import java.util.Set; import static com.google.common.truth.Truth.assertThat; import static org.mockito.Matchers.any; @@ -249,7 +249,7 @@ public class SearchFragmentTest { Robolectric.flushForegroundThreadScheduler(); - verify(fragment, times(2)).onLoadFinished(any(Loader.class), any(List.class)); + verify(fragment, times(2)).onLoadFinished(any(Loader.class), any(Set.class)); } @Test diff --git a/tests/robotests/src/com/android/settings/search/SearchResultsAdapterTest.java b/tests/robotests/src/com/android/settings/search/SearchResultsAdapterTest.java index 5af85506554..829034863f3 100644 --- a/tests/robotests/src/com/android/settings/search/SearchResultsAdapterTest.java +++ b/tests/robotests/src/com/android/settings/search/SearchResultsAdapterTest.java @@ -46,8 +46,10 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Objects; +import java.util.Set; import static com.google.common.truth.Truth.assertThat; @@ -85,7 +87,7 @@ public class SearchResultsAdapterTest { @Test public void testSingleSourceMerge_exactCopyReturned() { - ArrayList intentResults = getIntentSampleResults(); + Set intentResults = getIntentSampleResults(); mAdapter.addSearchResults(intentResults, mLoaderClassName); mAdapter.displaySearchResults(""); @@ -111,8 +113,10 @@ public class SearchResultsAdapterTest { @Test public void testEndToEndSearch_properResultsMerged_correctOrder() { - mAdapter.addSearchResults(getDummyAppResults(), InstalledAppResultLoader.class.getName()); - mAdapter.addSearchResults(getDummyDbResults(), DatabaseResultLoader.class.getName()); + mAdapter.addSearchResults(new HashSet(getDummyAppResults()), + InstalledAppResultLoader.class.getName()); + mAdapter.addSearchResults(new HashSet(getDummyDbResults()), + DatabaseResultLoader.class.getName()); int count = mAdapter.displaySearchResults(""); List results = mAdapter.getSearchResults(); @@ -130,13 +134,16 @@ public class SearchResultsAdapterTest { List appResults = getDummyAppResults(); List dbResults = getDummyDbResults(); // Add two individual items - mAdapter.addSearchResults(appResults.subList(0,1), + mAdapter.addSearchResults(new HashSet(appResults.subList(0, 1)), InstalledAppResultLoader.class.getName()); - mAdapter.addSearchResults(dbResults.subList(0,1), DatabaseResultLoader.class.getName()); + mAdapter.addSearchResults(new HashSet(dbResults.subList(0, 1)), + DatabaseResultLoader.class.getName()); mAdapter.displaySearchResults(""); // Add super-set of items - mAdapter.addSearchResults(appResults, InstalledAppResultLoader.class.getName()); - mAdapter.addSearchResults(dbResults, DatabaseResultLoader.class.getName()); + mAdapter.addSearchResults( + new HashSet(appResults), InstalledAppResultLoader.class.getName()); + mAdapter.addSearchResults( + new HashSet(dbResults), DatabaseResultLoader.class.getName()); int count = mAdapter.displaySearchResults(""); List results = mAdapter.getSearchResults(); @@ -170,13 +177,16 @@ public class SearchResultsAdapterTest { List appResults = getDummyAppResults(); List dbResults = getDummyDbResults(); // Add list of items - mAdapter.addSearchResults(appResults, InstalledAppResultLoader.class.getName()); - mAdapter.addSearchResults(dbResults, DatabaseResultLoader.class.getName()); + mAdapter.addSearchResults(new HashSet(appResults), + InstalledAppResultLoader.class.getName()); + mAdapter.addSearchResults(new HashSet(dbResults), + DatabaseResultLoader.class.getName()); mAdapter.displaySearchResults(""); // Add subset of items - mAdapter.addSearchResults(appResults.subList(0,1), + mAdapter.addSearchResults(new HashSet(appResults.subList(0, 1)), InstalledAppResultLoader.class.getName()); - mAdapter.addSearchResults(dbResults.subList(0,1), DatabaseResultLoader.class.getName()); + mAdapter.addSearchResults(new HashSet<>(dbResults.subList(0, 1)), + DatabaseResultLoader.class.getName()); int count = mAdapter.displaySearchResults(""); List results = mAdapter.getSearchResults(); @@ -231,8 +241,8 @@ public class SearchResultsAdapterTest { return results; } - private ArrayList getIntentSampleResults() { - ArrayList sampleResults = new ArrayList<>(); + private Set getIntentSampleResults() { + Set sampleResults = new HashSet<>(); ArrayList breadcrumbs = new ArrayList<>(); final Drawable icon = mContext.getDrawable(R.drawable.ic_search_history); final ResultPayload payload = new ResultPayload(null);