Fix race condition and optimize categoryUpdater refresh

- In SettingsActivity, do not call updateCategories() if nothing
  changed.
- In SummaryLoader, create a mapping between tile key and summary. This
  is necessary to handle a race condition where category is refreshed
  after summary load.
- In DashboardSummary, refresh Tile's summary to latest cache value
  everytime category is refreshed.

Change-Id: I61389b8ba614ba7e34939325bada6e1bd6fa6709
Fix: 63149109
Test: robotests

src/com/android/settings/SettingsActivity.java

@@ -45,7 +45,6 @@ import android.support.v7.preference.PreferenceManager;
 import android.text.TextUtils;
 import android.transition.TransitionManager;
 import android.util.Log;
-import android.view.Menu;
 import android.view.View;
 import android.view.View.OnClickListener;
 import android.view.ViewGroup;
@@ -64,7 +63,6 @@ import com.android.settings.development.DevelopmentSettings;
 import com.android.settings.overlay.FeatureFactory;
 import com.android.settings.search.DynamicIndexableContentMonitor;
 import com.android.settings.search.SearchActivity;
-import com.android.settings.search.SearchFeatureProvider;
 import com.android.settings.wfd.WifiDisplaySettings;
 import com.android.settings.widget.SwitchBar;
 import com.android.settingslib.drawer.DashboardCategory;
@@ -770,68 +768,81 @@ public class SettingsActivity extends SettingsDrawerActivity
         PackageManager pm = getPackageManager();
         final UserManager um = UserManager.get(this);
         final boolean isAdmin = um.isAdminUser();
-
+        boolean somethingChanged = false;
         String packageName = getPackageName();
-        setTileEnabled(new ComponentName(packageName, WifiSettingsActivity.class.getName()),
+        somethingChanged = setTileEnabled(
-                pm.hasSystemFeature(PackageManager.FEATURE_WIFI), isAdmin);
+                new ComponentName(packageName, WifiSettingsActivity.class.getName()),
+                pm.hasSystemFeature(PackageManager.FEATURE_WIFI), isAdmin) || somethingChanged;
 
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
                         Settings.BluetoothSettingsActivity.class.getName()),
-                pm.hasSystemFeature(PackageManager.FEATURE_BLUETOOTH), isAdmin);
+                pm.hasSystemFeature(PackageManager.FEATURE_BLUETOOTH), isAdmin)
+                || somethingChanged;
 
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
                         Settings.DataUsageSummaryActivity.class.getName()),
-                Utils.isBandwidthControlEnabled(), isAdmin);
+                Utils.isBandwidthControlEnabled(), isAdmin)
+                || somethingChanged;
 
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
                         Settings.SimSettingsActivity.class.getName()),
-                Utils.showSimCardTile(this), isAdmin);
+                Utils.showSimCardTile(this), isAdmin)
+                || somethingChanged;
 
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
                         Settings.PowerUsageSummaryActivity.class.getName()),
-                mBatteryPresent, isAdmin);
+                mBatteryPresent, isAdmin) || somethingChanged;
 
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
                         Settings.UserSettingsActivity.class.getName()),
                 UserHandle.MU_ENABLED && UserManager.supportsMultipleUsers()
-                        && !Utils.isMonkeyRunning(), isAdmin);
+                        && !Utils.isMonkeyRunning(), isAdmin)
+                || somethingChanged;
 
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
                         Settings.NetworkDashboardActivity.class.getName()),
-                !UserManager.isDeviceInDemoMode(this), isAdmin);
+                !UserManager.isDeviceInDemoMode(this), isAdmin)
+                || somethingChanged;
 
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
                         Settings.ConnectedDeviceDashboardActivity.class.getName()),
-                !UserManager.isDeviceInDemoMode(this), isAdmin);
+                !UserManager.isDeviceInDemoMode(this), isAdmin)
+                || somethingChanged;
 
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
                         Settings.DateTimeSettingsActivity.class.getName()),
-                !UserManager.isDeviceInDemoMode(this), isAdmin);
+                !UserManager.isDeviceInDemoMode(this), isAdmin)
+                || somethingChanged;
         NfcAdapter adapter = NfcAdapter.getDefaultAdapter(this);
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
                         Settings.PaymentSettingsActivity.class.getName()),
                 pm.hasSystemFeature(PackageManager.FEATURE_NFC)
                         && pm.hasSystemFeature(PackageManager.FEATURE_NFC_HOST_CARD_EMULATION)
-                        && adapter != null && adapter.isEnabled(), isAdmin);
+                        && adapter != null && adapter.isEnabled(), isAdmin)
+                || somethingChanged;
 
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
                         Settings.PrintSettingsActivity.class.getName()),
-                pm.hasSystemFeature(PackageManager.FEATURE_PRINTING), isAdmin);
+                pm.hasSystemFeature(PackageManager.FEATURE_PRINTING), isAdmin)
+                || somethingChanged;
 
         final boolean showDev = mDevelopmentPreferences.getBoolean(
                 DevelopmentSettings.PREF_SHOW, android.os.Build.TYPE.equals("eng"))
                 && !um.hasUserRestriction(UserManager.DISALLOW_DEBUGGING_FEATURES);
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
                         Settings.DevelopmentSettingsActivity.class.getName()),
-                showDev, isAdmin);
+                showDev, isAdmin)
+                || somethingChanged;
 
         // Enable/disable backup settings depending on whether the user is admin.
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
-                        BackupSettingsActivity.class.getName()), true, isAdmin);
+                BackupSettingsActivity.class.getName()), true, isAdmin)
+                || somethingChanged;
 
-        setTileEnabled(new ComponentName(packageName,
+        somethingChanged = setTileEnabled(new ComponentName(packageName,
                         Settings.WifiDisplaySettingsActivity.class.getName()),
-                WifiDisplaySettings.isAvailable(this), isAdmin);
+                WifiDisplaySettings.isAvailable(this), isAdmin)
+                || somethingChanged;
 
         if (UserHandle.MU_ENABLED && !isAdmin) {
 
@@ -848,7 +859,8 @@ public class SettingsActivity extends SettingsDrawerActivity
                                 SettingsGateway.SETTINGS_FOR_RESTRICTED, name);
                         if (packageName.equals(component.getPackageName())
                                 && !isEnabledForRestricted) {
-                            setTileEnabled(component, false, isAdmin);
+                            somethingChanged = setTileEnabled(component, false, isAdmin)
+                                    || somethingChanged;
                         }
                     }
                 }
@@ -856,16 +868,24 @@ public class SettingsActivity extends SettingsDrawerActivity
         }
 
         // Final step, refresh categories.
+        if (somethingChanged) {
+            Log.d(LOG_TAG, "Enabled state changed for some tiles, reloading all categories");
             updateCategories();
+        } else {
+            Log.d(LOG_TAG, "No enabled state changed, skipping updateCategory call");
+        }
     }
 
-    private void setTileEnabled(ComponentName component, boolean enabled, boolean isAdmin) {
+    /**
+     * @return whether or not the enabled state actually changed.
+     */
+    private boolean setTileEnabled(ComponentName component, boolean enabled, boolean isAdmin) {
         if (UserHandle.MU_ENABLED && !isAdmin && getPackageName().equals(component.getPackageName())
                 && !ArrayUtils.contains(SettingsGateway.SETTINGS_FOR_RESTRICTED,
                 component.getClassName())) {
             enabled = false;
         }
-        setTileEnabled(component, enabled);
+        return setTileEnabled(component, enabled);
     }
 
     private void getMetaData() {

src/com/android/settings/dashboard/DashboardSummary.java

@@ -294,10 +294,12 @@ public class DashboardSummary extends InstrumentedFragment
 
         final DashboardCategory category = mDashboardFeatureProvider.getTilesForCategory(
                 CategoryKey.CATEGORY_HOMEPAGE);
+        mSummaryLoader.updateSummaryToCache(category);
         if (suggestions != null) {
             mAdapter.setCategoriesAndSuggestions(category, suggestions);
         } else {
             mAdapter.setCategory(category);
         }
     }
+
 }

src/com/android/settings/dashboard/SummaryLoader.java

@@ -34,7 +34,6 @@ import android.util.Log;
 import com.android.settings.SettingsActivity;
 import com.android.settings.overlay.FeatureFactory;
 import com.android.settingslib.drawer.DashboardCategory;
-import com.android.settingslib.drawer.SettingsDrawerActivity;
 import com.android.settingslib.drawer.Tile;
 
 import java.lang.reflect.Field;
@@ -47,7 +46,8 @@ public class SummaryLoader {
     public static final String SUMMARY_PROVIDER_FACTORY = "SUMMARY_PROVIDER_FACTORY";
 
     private final Activity mActivity;
-    private final ArrayMap<SummaryProvider, ComponentName> mSummaryMap = new ArrayMap<>();
+    private final ArrayMap<SummaryProvider, ComponentName> mSummaryProviderMap = new ArrayMap<>();
+    private final ArrayMap<String, CharSequence> mSummaryTextMap = new ArrayMap<>();
     private final DashboardFeatureProvider mDashboardFeatureProvider;
     private final String mCategoryKey;
 
@@ -111,7 +111,7 @@ public class SummaryLoader {
     }
 
     public void setSummary(SummaryProvider provider, final CharSequence summary) {
-        final ComponentName component = mSummaryMap.get(provider);
+        final ComponentName component = mSummaryProviderMap.get(provider);
         mHandler.post(new Runnable() {
             @Override
             public void run() {
@@ -142,6 +142,7 @@ public class SummaryLoader {
             }
             return;
         }
+        mSummaryTextMap.put(mDashboardFeatureProvider.getDashboardKeyForTile(tile), summary);
         tile.summary = summary;
         if (mSummaryConsumer != null) {
             mSummaryConsumer.notifySummaryChanged(tile);
@@ -223,11 +224,27 @@ public class SummaryLoader {
         });
     }
 
+    /**
+     * Updates all tile's summary to latest cached version. This is necessary to handle the case
+     * where category is updated after summary change.
+     */
+    public void updateSummaryToCache(DashboardCategory category) {
+        if (category == null) {
+            return;
+        }
+        for (Tile tile : category.tiles) {
+            final String key = mDashboardFeatureProvider.getDashboardKeyForTile(tile);
+            if (mSummaryTextMap.containsKey(key)) {
+                tile.summary = mSummaryTextMap.get(key);
+            }
+        }
+    }
+
     private synchronized void setListeningW(boolean listening) {
         if (mWorkerListening == listening) return;
         mWorkerListening = listening;
         if (DEBUG) Log.d(TAG, "Listening " + listening);
-        for (SummaryProvider p : mSummaryMap.keySet()) {
+        for (SummaryProvider p : mSummaryProviderMap.keySet()) {
             try {
                 p.setListening(listening);
             } catch (Exception e) {
@@ -240,28 +257,10 @@ public class SummaryLoader {
         SummaryProvider provider = getSummaryProvider(tile);
         if (provider != null) {
             if (DEBUG) Log.d(TAG, "Creating " + tile);
-            mSummaryMap.put(provider, tile.intent.getComponent());
+            mSummaryProviderMap.put(provider, tile.intent.getComponent());
         }
     }
 
-    private Tile getTileFromCategory(List<DashboardCategory> categories, ComponentName component) {
-        if (categories == null) {
-            if (DEBUG) {
-                Log.d(TAG, "Category is null, can't find tile");
-            }
-            return null;
-        }
-        final int categorySize = categories.size();
-        for (int i = 0; i < categorySize; i++) {
-            final DashboardCategory category = categories.get(i);
-            final Tile tile = getTileFromCategory(category, component);
-            if (tile != null) {
-                return tile;
-            }
-        }
-        return null;
-    }
-
     private Tile getTileFromCategory(DashboardCategory category, ComponentName component) {
         if (category == null || category.tiles == null) {
             return null;
@@ -276,6 +275,8 @@ public class SummaryLoader {
         return null;
     }
 
+
+
     public interface SummaryProvider {
         void setListening(boolean listening);
     }

tests/robotests/src/com/android/settings/dashboard/DashboardSummaryTest.java

@@ -19,11 +19,12 @@ package com.android.settings.dashboard;
 import android.app.Activity;
 import android.support.v7.widget.LinearLayoutManager;
 
-import com.android.settings.testutils.SettingsRobolectricTestRunner;
 import com.android.settings.TestConfig;
 import com.android.settings.dashboard.conditional.ConditionManager;
 import com.android.settings.dashboard.conditional.FocusRecyclerView;
+import com.android.settings.testutils.SettingsRobolectricTestRunner;
 import com.android.settingslib.drawer.CategoryKey;
+import com.android.settingslib.drawer.DashboardCategory;
 import com.android.settingslib.drawer.Tile;
 
 import org.junit.Before;
@@ -34,6 +35,7 @@ import org.mockito.MockitoAnnotations;
 import org.robolectric.annotation.Config;
 import org.robolectric.util.ReflectionHelpers;
 
+import static org.mockito.ArgumentMatchers.nullable;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
@@ -57,6 +59,8 @@ public class DashboardSummaryTest {
     private LinearLayoutManager mLayoutManager;
     @Mock
     private ConditionManager mConditionManager;
+    @Mock
+    private SummaryLoader mSummaryLoader;
 
     private DashboardSummary mSummary;
 
@@ -70,12 +74,15 @@ public class DashboardSummaryTest {
         ReflectionHelpers.setField(mSummary, "mDashboard", mDashboard);
         ReflectionHelpers.setField(mSummary, "mLayoutManager", mLayoutManager);
         ReflectionHelpers.setField(mSummary, "mConditionManager", mConditionManager);
+        ReflectionHelpers.setField(mSummary, "mSummaryLoader", mSummaryLoader);
     }
 
     @Test
     public void updateCategoryAndSuggestion_shouldGetCategoryFromFeatureProvider() {
         doReturn(mock(Activity.class)).when(mSummary).getActivity();
         mSummary.updateCategoryAndSuggestion(null);
+
+        verify(mSummaryLoader).updateSummaryToCache(nullable(DashboardCategory.class));
         verify(mDashboardFeatureProvider).getTilesForCategory(CategoryKey.CATEGORY_HOMEPAGE);
     }
 

tests/robotests/src/com/android/settings/dashboard/SummaryLoaderTest.java

@@ -17,13 +17,21 @@
 package com.android.settings.dashboard;
 
 import android.app.Activity;
-import com.android.settings.testutils.SettingsRobolectricTestRunner;
+import android.content.Context;
+import android.content.Intent;
+
 import com.android.settings.TestConfig;
+import com.android.settings.testutils.FakeFeatureFactory;
+import com.android.settings.testutils.SettingsRobolectricTestRunner;
 import com.android.settingslib.drawer.DashboardCategory;
 import com.android.settingslib.drawer.Tile;
+
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.Answers;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
 import org.robolectric.Robolectric;
 import org.robolectric.annotation.Config;
 
@@ -31,18 +39,27 @@ import java.util.ArrayList;
 import java.util.List;
 
 import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Mockito.when;
 
 @RunWith(SettingsRobolectricTestRunner.class)
 @Config(manifest = TestConfig.MANIFEST_PATH, sdk = TestConfig.SDK_VERSION)
 public class SummaryLoaderTest {
+
     private static final String SUMMARY_1 = "summary1";
     private static final String SUMMARY_2 = "summary2";
+
+    @Mock(answer = Answers.RETURNS_DEEP_STUBS)
+    private Context mContext;
     private SummaryLoader mSummaryLoader;
     private boolean mCallbackInvoked;
     private Tile mTile;
+    private FakeFeatureFactory mFeatureFactory;
 
     @Before
     public void SetUp() {
+        MockitoAnnotations.initMocks(this);
+        mFeatureFactory = FakeFeatureFactory.setupForTest(mContext);
+
         mTile = new Tile();
         mTile.summary = SUMMARY_1;
         mCallbackInvoked = false;
@@ -71,4 +88,23 @@ public class SummaryLoaderTest {
 
         assertThat(mCallbackInvoked).isTrue();
     }
+
+    @Test
+    public void testUpdateSummaryToCache_hasCache_shouldUpdate() {
+
+        final String testSummary = "test_summary";
+        final DashboardCategory category = new DashboardCategory();
+        final Tile tile = new Tile();
+        tile.key = "123";
+        tile.intent = new Intent();
+        category.addTile(tile);
+        when(mFeatureFactory.dashboardFeatureProvider.getDashboardKeyForTile(tile))
+                .thenReturn(tile.key);
+
+        mSummaryLoader.updateSummaryIfNeeded(tile, testSummary);
+        tile.summary = null;
+        mSummaryLoader.updateSummaryToCache(category);
+
+        assertThat(tile.summary).isEqualTo(testSummary);
+    }
 }