From 359bce4f501389eedc1b58879d0581f2afaf0e3b Mon Sep 17 00:00:00 2001 From: Matthew Fritze Date: Wed, 22 Mar 2017 13:51:36 -0700 Subject: [PATCH] Prevent search crashes from uninstalled apps All search results are now refreshed when resuming the search fragment, to prevent crashes from results that no longer exist. Bug: 34817357 Test: make RunSettingsRoboTests Change-Id: I96a0cbfee711ab9dee49d56bfdc4e885202d9ecd --- .../search2/DatabaseIndexingManager.java | 4 +- .../settings/search2/SearchFragment.java | 21 ++++- .../settings/search2/SearchResult.java | 24 ++++- .../search2/SearchResultDiffCallback.java | 56 ++++++++++++ .../search2/SearchResultsAdapter.java | 29 +++--- .../search/SearchResultsAdapterTest.java | 89 ++++++++++++++----- 6 files changed, 182 insertions(+), 41 deletions(-) create mode 100644 src/com/android/settings/search2/SearchResultDiffCallback.java diff --git a/src/com/android/settings/search2/DatabaseIndexingManager.java b/src/com/android/settings/search2/DatabaseIndexingManager.java index 30aad76b48e..c2e6ba8dc27 100644 --- a/src/com/android/settings/search2/DatabaseIndexingManager.java +++ b/src/com/android/settings/search2/DatabaseIndexingManager.java @@ -517,7 +517,7 @@ public class DatabaseIndexingManager { if (count > 0) { while (cursor.moveToNext()) { final int providerRank = cursor.getInt(COLUMN_INDEX_XML_RES_RANK); - + // TODO remove provider rank final int xmlResId = cursor.getInt(COLUMN_INDEX_XML_RES_RESID); final String className = cursor.getString(COLUMN_INDEX_XML_RES_CLASS_NAME); @@ -562,7 +562,7 @@ public class DatabaseIndexingManager { if (count > 0) { while (cursor.moveToNext()) { final int providerRank = cursor.getInt(COLUMN_INDEX_RAW_RANK); - + // TODO Remove rank final String title = cursor.getString(COLUMN_INDEX_RAW_TITLE); final String summaryOn = cursor.getString(COLUMN_INDEX_RAW_SUMMARY_ON); final String summaryOff = cursor.getString(COLUMN_INDEX_RAW_SUMMARY_OFF); diff --git a/src/com/android/settings/search2/SearchFragment.java b/src/com/android/settings/search2/SearchFragment.java index a8d219c771a..545a2255445 100644 --- a/src/com/android/settings/search2/SearchFragment.java +++ b/src/com/android/settings/search2/SearchFragment.java @@ -167,6 +167,17 @@ public class SearchFragment extends InstrumentedFragment implements SearchView.O return view; } + @Override + public void onResume() { + super.onResume(); + if (TextUtils.isEmpty(mQuery)) { + return; + } + final String query = mQuery; + mQuery = ""; + onQueryTextChange(query); + } + @Override public void onStop() { super.onStop(); @@ -206,7 +217,6 @@ public class SearchFragment extends InstrumentedFragment implements SearchView.O mResultClickCount = 0; mNeverEnteredQuery = false; mQuery = query; - mSearchAdapter.clearResults(); if (isEmptyQuery) { final LoaderManager loaderManager = getLoaderManager(); @@ -251,8 +261,15 @@ public class SearchFragment extends InstrumentedFragment implements SearchView.O if (mUnfinishedLoadersCount.decrementAndGet() != 0) { return; } + final int resultCount = mSearchAdapter.displaySearchResults(mQuery); - mNoResultsView.setVisibility(resultCount == 0 ? View.VISIBLE : View.GONE); + + if (resultCount == 0) { + mNoResultsView.setVisibility(View.VISIBLE); + } else { + mNoResultsView.setVisibility(View.GONE); + mResultsRecyclerView.scrollToPosition(0); + } mSearchFeatureProvider.showFeedbackButton(this, getView()); } diff --git a/src/com/android/settings/search2/SearchResult.java b/src/com/android/settings/search2/SearchResult.java index 6b27d89eacc..36fe5b3e592 100644 --- a/src/com/android/settings/search2/SearchResult.java +++ b/src/com/android/settings/search2/SearchResult.java @@ -93,7 +93,7 @@ public class SearchResult implements Comparable { icon = builder.mIcon; payload = builder.mResultPayload; viewType = payload.getType(); - stableId = Objects.hash(title, summary, breadcrumbs, rank, icon, payload, viewType); + stableId = Objects.hash(title, summary, breadcrumbs, rank, viewType); } @Override @@ -104,6 +104,22 @@ public class SearchResult implements Comparable { return this.rank - searchResult.rank; } + @Override + public boolean equals(Object that) { + if (this == that) { + return true; + } + if (!(that instanceof SearchResult)) { + return false; + } + return this.stableId == ((SearchResult) that).stableId; + } + + @Override + public int hashCode() { + return (int) stableId; + } + public static class Builder { protected CharSequence mTitle; protected CharSequence mSummary; @@ -127,19 +143,19 @@ public class SearchResult implements Comparable { return this; } - public Builder addRank(int rank) { + public Builder addRank(int rank) { if (rank >= 0 && rank <= 9) { mRank = rank; } return this; } - public Builder addIcon(Drawable icon) { + public Builder addIcon(Drawable icon) { mIcon = icon; return this; } - public Builder addPayload(ResultPayload payload) { + public Builder addPayload(ResultPayload payload) { mResultPayload = payload; return this; } diff --git a/src/com/android/settings/search2/SearchResultDiffCallback.java b/src/com/android/settings/search2/SearchResultDiffCallback.java new file mode 100644 index 00000000000..9bd1bde6300 --- /dev/null +++ b/src/com/android/settings/search2/SearchResultDiffCallback.java @@ -0,0 +1,56 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package com.android.settings.search2; + +import android.support.v7.util.DiffUtil; + +import java.util.List; + +/** + * Callback for DiffUtil to elegantly update search data when the query changes. + */ +public class SearchResultDiffCallback extends DiffUtil.Callback { + + private List mOldList; + private List mNewList; + + public SearchResultDiffCallback(List oldList, List newList) { + mOldList = oldList; + mNewList = newList; + } + + @Override + public int getOldListSize() { + return mOldList.size(); + } + + @Override + public int getNewListSize() { + return mNewList.size(); + } + + @Override + public boolean areItemsTheSame(int oldItemPosition, int newItemPosition) { + return mOldList.get(oldItemPosition).equals(mNewList.get(newItemPosition)); + } + + @Override + public boolean areContentsTheSame(int oldItemPosition, int newItemPosition) { + return mOldList.get(oldItemPosition).equals(mNewList.get(newItemPosition)); + } +} diff --git a/src/com/android/settings/search2/SearchResultsAdapter.java b/src/com/android/settings/search2/SearchResultsAdapter.java index d0ea5bf3fcd..5da5966ba3d 100644 --- a/src/com/android/settings/search2/SearchResultsAdapter.java +++ b/src/com/android/settings/search2/SearchResultsAdapter.java @@ -19,6 +19,7 @@ package com.android.settings.search2; import android.content.Context; import android.support.annotation.MainThread; import android.support.annotation.VisibleForTesting; +import android.support.v7.util.DiffUtil; import android.support.v7.widget.RecyclerView; import android.util.ArrayMap; import android.view.LayoutInflater; @@ -37,8 +38,9 @@ import static com.android.settings.search2.SearchResult.TOP_RANK; public class SearchResultsAdapter extends RecyclerView.Adapter { - private final List mSearchResults; private final SearchFragment mFragment; + + private List mSearchResults; private Map> mResultsMap; private final SearchFeatureProvider mSearchFeatureProvider; @@ -132,7 +134,7 @@ public class SearchResultsAdapter extends RecyclerView.Adapter .get(InstalledAppResultLoader.class.getName()); final int dbSize = (databaseResults != null) ? databaseResults.size() : 0; final int appSize = (installedAppResults != null) ? installedAppResults.size() : 0; - final List results = new ArrayList<>(dbSize + appSize); + final List newResults = new ArrayList<>(dbSize + appSize); int dbIndex = 0; int appIndex = 0; @@ -140,29 +142,34 @@ public class SearchResultsAdapter extends RecyclerView.Adapter while (rank <= BOTTOM_RANK) { while ((dbIndex < dbSize) && (databaseResults.get(dbIndex).rank == rank)) { - results.add(databaseResults.get(dbIndex++)); + newResults.add(databaseResults.get(dbIndex++)); } while ((appIndex < appSize) && (installedAppResults.get(appIndex).rank == rank)) { - results.add(installedAppResults.get(appIndex++)); + newResults.add(installedAppResults.get(appIndex++)); } rank++; } while (dbIndex < dbSize) { - results.add(databaseResults.get(dbIndex++)); + newResults.add(databaseResults.get(dbIndex++)); } while (appIndex < appSize) { - results.add(installedAppResults.get(appIndex++)); + newResults.add(installedAppResults.get(appIndex++)); } - if (mSearchFeatureProvider - .isSmartSearchRankingEnabled(mFragment.getContext().getApplicationContext())) { + final boolean isSmartSearchRankingEnabled = mSearchFeatureProvider + .isSmartSearchRankingEnabled(mFragment.getContext().getApplicationContext()); + + if (isSmartSearchRankingEnabled) { // TODO: run this in parallel to loading the results if takes too long - mSearchFeatureProvider.rankSearchResults(query, results); + mSearchFeatureProvider.rankSearchResults(query, newResults); } - mSearchResults.addAll(results); - notifyDataSetChanged(); + final DiffUtil.DiffResult diffResult = DiffUtil.calculateDiff( + new SearchResultDiffCallback(mSearchResults, newResults), + isSmartSearchRankingEnabled); + mSearchResults = newResults; + diffResult.dispatchUpdatesTo(this); return mSearchResults.size(); } diff --git a/tests/robotests/src/com/android/settings/search/SearchResultsAdapterTest.java b/tests/robotests/src/com/android/settings/search/SearchResultsAdapterTest.java index c507c7996e2..3007f5e26f8 100644 --- a/tests/robotests/src/com/android/settings/search/SearchResultsAdapterTest.java +++ b/tests/robotests/src/com/android/settings/search/SearchResultsAdapterTest.java @@ -57,6 +57,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import static com.google.common.truth.Truth.assertThat; @@ -75,6 +76,8 @@ public class SearchResultsAdapterTest { private Context mContext; private String mLoaderClassName; + private String[] TITLES = {"alpha", "bravo", "charlie", "appAlpha", "appBravo", "appCharlie"}; + @Before public void setUp() { MockitoAnnotations.initMocks(this); @@ -86,13 +89,13 @@ public class SearchResultsAdapterTest { } @Test - public void testNoResultsAdded_EmptyListReturned() { + public void testNoResultsAdded_emptyListReturned() { List updatedResults = mAdapter.getSearchResults(); assertThat(updatedResults).isEmpty(); } @Test - public void testSingleSourceMerge_ExactCopyReturned() { + public void testSingleSourceMerge_exactCopyReturned() { ArrayList intentResults = getIntentSampleResults(); mAdapter.addSearchResults(intentResults, mLoaderClassName); mAdapter.displaySearchResults(""); @@ -102,7 +105,7 @@ public class SearchResultsAdapterTest { } @Test - public void testCreateViewHolder_ReturnsIntentResult() { + public void testCreateViewHolder_returnsIntentResult() { ViewGroup group = new FrameLayout(mContext); SearchViewHolder view = mAdapter.onCreateViewHolder(group, ResultPayload.PayloadType.INTENT); @@ -110,7 +113,7 @@ public class SearchResultsAdapterTest { } @Test - public void testCreateViewHolder_ReturnsInlineSwitchResult() { + public void testCreateViewHolder_returnsInlineSwitchResult() { ViewGroup group = new FrameLayout(mContext); SearchViewHolder view = mAdapter.onCreateViewHolder(group, ResultPayload.PayloadType.INLINE_SWITCH); @@ -118,20 +121,42 @@ public class SearchResultsAdapterTest { } @Test - public void testEndToEndSearch_ProperResultsMerged() { - mAdapter.addSearchResults(getDummyAppResults(), - InstalledAppResultLoader.class.getName()); - mAdapter.addSearchResults(getDummyDbResults(), - DatabaseResultLoader.class.getName()); + public void testEndToEndSearch_properResultsMerged_correctOrder() { + mAdapter.addSearchResults(getDummyAppResults(), InstalledAppResultLoader.class.getName()); + mAdapter.addSearchResults(getDummyDbResults(), DatabaseResultLoader.class.getName()); int count = mAdapter.displaySearchResults(""); List results = mAdapter.getSearchResults(); - assertThat(results.get(0).title).isEqualTo("alpha"); - assertThat(results.get(1).title).isEqualTo("appAlpha"); - assertThat(results.get(2).title).isEqualTo("appBravo"); - assertThat(results.get(3).title).isEqualTo("bravo"); - assertThat(results.get(4).title).isEqualTo("appCharlie"); - assertThat(results.get(5).title).isEqualTo("Charlie"); + assertThat(results.get(0).title).isEqualTo(TITLES[0]); // alpha + assertThat(results.get(1).title).isEqualTo(TITLES[3]); // appAlpha + assertThat(results.get(2).title).isEqualTo(TITLES[4]); // appBravo + assertThat(results.get(3).title).isEqualTo(TITLES[1]); // bravo + assertThat(results.get(4).title).isEqualTo(TITLES[5]); // appCharlie + assertThat(results.get(5).title).isEqualTo(TITLES[2]); // charlie + assertThat(count).isEqualTo(6); + } + + @Test + public void testEndToEndSearch_addResults_resultsAddedInOrder() { + List appResults = getDummyAppResults(); + List dbResults = getDummyDbResults(); + // Add two individual items + mAdapter.addSearchResults(appResults.subList(0,1), + InstalledAppResultLoader.class.getName()); + mAdapter.addSearchResults(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()); + int count = mAdapter.displaySearchResults(""); + + List results = mAdapter.getSearchResults(); + assertThat(results.get(0).title).isEqualTo(TITLES[0]); // alpha + assertThat(results.get(1).title).isEqualTo(TITLES[3]); // appAlpha + assertThat(results.get(2).title).isEqualTo(TITLES[4]); // appBravo + assertThat(results.get(3).title).isEqualTo(TITLES[1]); // bravo + assertThat(results.get(4).title).isEqualTo(TITLES[5]); // appCharlie + assertThat(results.get(5).title).isEqualTo(TITLES[2]); // charlie assertThat(count).isEqualTo(6); } @@ -146,26 +171,46 @@ public class SearchResultsAdapterTest { @Test public void testDisplayResults_ShouldRunSmartRankingIfEnabled() { when(mSearchFeatureProvider.isSmartSearchRankingEnabled(any())) - .thenReturn(true); + .thenReturn(true); mAdapter.displaySearchResults(""); verify(mSearchFeatureProvider, times(1)).rankSearchResults(anyString(), anyList()); } + @Test + public void testEndToEndSearch_removeResults_resultsAdded() { + List appResults = getDummyAppResults(); + List dbResults = getDummyDbResults(); + // Add list of items + mAdapter.addSearchResults(appResults, InstalledAppResultLoader.class.getName()); + mAdapter.addSearchResults(dbResults, DatabaseResultLoader.class.getName()); + mAdapter.displaySearchResults(""); + // Add subset of items + mAdapter.addSearchResults(appResults.subList(0,1), + InstalledAppResultLoader.class.getName()); + mAdapter.addSearchResults(dbResults.subList(0,1), DatabaseResultLoader.class.getName()); + int count = mAdapter.displaySearchResults(""); + + List results = mAdapter.getSearchResults(); + assertThat(results.get(0).title).isEqualTo(TITLES[0]); + assertThat(results.get(1).title).isEqualTo(TITLES[3]); + assertThat(count).isEqualTo(2); + } + private List getDummyDbResults() { List results = new ArrayList<>(); ResultPayload payload = new ResultPayload(new Intent()); SearchResult.Builder builder = new SearchResult.Builder(); builder.addPayload(payload); - builder.addTitle("alpha") + builder.addTitle(TITLES[0]) .addRank(1); results.add(builder.build()); - builder.addTitle("bravo") + builder.addTitle(TITLES[1]) .addRank(3); results.add(builder.build()); - builder.addTitle("Charlie") + builder.addTitle(TITLES[2]) .addRank(6); results.add(builder.build()); @@ -178,15 +223,15 @@ public class SearchResultsAdapterTest { AppSearchResult.Builder builder = new AppSearchResult.Builder(); builder.addPayload(payload); - builder.addTitle("appAlpha") + builder.addTitle(TITLES[3]) .addRank(1); results.add(builder.build()); - builder.addTitle("appBravo") + builder.addTitle(TITLES[4]) .addRank(2); results.add(builder.build()); - builder.addTitle("appCharlie") + builder.addTitle(TITLES[5]) .addRank(4); results.add(builder.build());