Fix LoaderCallback.onLoadFinished uncalled issue

When two loaders started almost at the same time,
it is possible onLoadFinished is never called.

Bug: 256523472
Test: Unit tests passed and manual test on device
Change-Id: I41a041d5878f9930db44775408380d0d4588faba
Signed-off-by: Zhenwei Chen <zhenwec@google.com>

src/com/android/settings/fuelgauge/batteryusage/PowerUsageAdvanced.java

@@ -52,7 +52,6 @@ public class PowerUsageAdvanced extends PowerUsageBase {
     private static final String KEY_REFRESH_TYPE = "refresh_type";
     private static final String KEY_BATTERY_GRAPH = "battery_graph";
     private static final String KEY_APP_LIST = "app_list";
-    private static final int LOADER_BATTERY_USAGE_STATS = 2;
 
     @VisibleForTesting
     BatteryHistoryPreference mHistPref;
@@ -165,7 +164,7 @@ public class PowerUsageAdvanced extends PowerUsageBase {
         bundle.putInt(KEY_REFRESH_TYPE, refreshType);
         if (!mIsChartDataLoaded) {
             mIsChartDataLoaded = true;
-            getLoaderManager().restartLoader(LOADER_BATTERY_USAGE_STATS, bundle,
+            restartLoader(LoaderIndex.BATTERY_HISTORY_LOADER, bundle,
                     mBatteryHistoryLoaderCallbacks);
         }
     }

src/com/android/settings/fuelgauge/batteryusage/PowerUsageBase.java

@@ -24,6 +24,7 @@ import android.os.Bundle;
 import android.os.UserManager;
 import android.util.Log;
 
+import androidx.annotation.IntDef;
 import androidx.annotation.NonNull;
 import androidx.annotation.VisibleForTesting;
 import androidx.loader.app.LoaderManager;
@@ -33,17 +34,19 @@ import com.android.settings.dashboard.DashboardFragment;
 import com.android.settings.fuelgauge.BatteryBroadcastReceiver;
 import com.android.settings.fuelgauge.BatteryUtils;
 
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
 /**
  * Common base class for things that need to show the battery usage graph.
  */
 public abstract class PowerUsageBase extends DashboardFragment {
-
     private static final String TAG = "PowerUsageBase";
-    private static final String KEY_REFRESH_TYPE = "refresh_type";
-    private static final String KEY_INCLUDE_HISTORY = "include_history";
-
-    private static final int LOADER_BATTERY_USAGE_STATS = 1;
 
+    @VisibleForTesting
+    static final String KEY_REFRESH_TYPE = "refresh_type";
+    @VisibleForTesting
+    static final String KEY_INCLUDE_HISTORY = "include_history";
     @VisibleForTesting
     BatteryUsageStats mBatteryUsageStats;
 
@@ -55,6 +58,21 @@ public abstract class PowerUsageBase extends DashboardFragment {
     final BatteryUsageStatsLoaderCallbacks mBatteryUsageStatsLoaderCallbacks =
             new BatteryUsageStatsLoaderCallbacks();
 
+    @Retention(RetentionPolicy.SOURCE)
+    @IntDef({
+            LoaderIndex.BATTERY_USAGE_STATS_LOADER,
+            LoaderIndex.BATTERY_INFO_LOADER,
+            LoaderIndex.BATTERY_TIP_LOADER,
+            LoaderIndex.BATTERY_HISTORY_LOADER
+
+    })
+    public @interface LoaderIndex {
+        int BATTERY_USAGE_STATS_LOADER = 0;
+        int BATTERY_INFO_LOADER = 1;
+        int BATTERY_TIP_LOADER = 2;
+        int BATTERY_HISTORY_LOADER = 3;
+    }
+
     @Override
     public void onAttach(Activity activity) {
         super.onAttach(activity);
@@ -91,10 +109,28 @@ public abstract class PowerUsageBase extends DashboardFragment {
         final Bundle bundle = new Bundle();
         bundle.putInt(KEY_REFRESH_TYPE, refreshType);
         bundle.putBoolean(KEY_INCLUDE_HISTORY, isBatteryHistoryNeeded());
-        getLoaderManager().restartLoader(LOADER_BATTERY_USAGE_STATS, bundle,
+        restartLoader(LoaderIndex.BATTERY_USAGE_STATS_LOADER, bundle,
                 mBatteryUsageStatsLoaderCallbacks);
     }
 
+    protected LoaderManager getLoaderManagerForCurrentFragment() {
+        return LoaderManager.getInstance(this);
+    }
+
+    protected void restartLoader(int loaderId, Bundle bundle,
+            LoaderManager.LoaderCallbacks<?> loaderCallbacks) {
+        LoaderManager loaderManager = getLoaderManagerForCurrentFragment();
+        Loader<?> loader = loaderManager.getLoader(
+                loaderId);
+        if (loader != null && !loader.isReset()) {
+            loaderManager.restartLoader(loaderId, bundle,
+                    loaderCallbacks);
+        } else {
+            loaderManager.initLoader(loaderId, bundle,
+                    loaderCallbacks);
+        }
+    }
+
     protected void onLoadFinished(@BatteryUpdateType int refreshType) {
         refreshUi(refreshType);
     }

src/com/android/settings/fuelgauge/batteryusage/PowerUsageSummary.java

@@ -64,11 +64,6 @@ public class PowerUsageSummary extends PowerUsageBase implements
     @VisibleForTesting
     static final String KEY_BATTERY_USAGE = "battery_usage_summary";
 
-    @VisibleForTesting
-    static final int BATTERY_INFO_LOADER = 1;
-    @VisibleForTesting
-    static final int BATTERY_TIP_LOADER = 2;
-
     @VisibleForTesting
     PowerUsageFeatureProvider mPowerFeatureProvider;
     @VisibleForTesting
@@ -241,7 +236,7 @@ public class PowerUsageSummary extends PowerUsageBase implements
 
     @VisibleForTesting
     void restartBatteryTipLoader() {
-        getLoaderManager().restartLoader(BATTERY_TIP_LOADER, Bundle.EMPTY, mBatteryTipsCallbacks);
+        restartLoader(LoaderIndex.BATTERY_TIP_LOADER, Bundle.EMPTY, mBatteryTipsCallbacks);
     }
 
     @VisibleForTesting
@@ -274,8 +269,7 @@ public class PowerUsageSummary extends PowerUsageBase implements
         if (!mIsBatteryPresent) {
             return;
         }
-        getLoaderManager().restartLoader(BATTERY_INFO_LOADER, Bundle.EMPTY,
+        restartLoader(LoaderIndex.BATTERY_INFO_LOADER, Bundle.EMPTY, mBatteryInfoLoaderCallbacks);
-                mBatteryInfoLoaderCallbacks);
     }
 
     @VisibleForTesting

tests/robotests/src/com/android/settings/fuelgauge/batteryusage/PowerUsageBaseTest.java

@@ -15,18 +15,26 @@
  */
 package com.android.settings.fuelgauge.batteryusage;
 
+import static com.android.settings.fuelgauge.batteryusage.PowerUsageBase.KEY_INCLUDE_HISTORY;
+import static com.android.settings.fuelgauge.batteryusage.PowerUsageBase.KEY_REFRESH_TYPE;
+
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.refEq;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 
 import android.content.Context;
+import android.os.BatteryUsageStats;
 import android.os.Bundle;
 
 import androidx.loader.app.LoaderManager;
+import androidx.loader.content.Loader;
 
+import com.android.settings.fuelgauge.BatteryBroadcastReceiver;
 import com.android.settings.testutils.shadow.ShadowDashboardFragment;
 import com.android.settingslib.core.AbstractPreferenceController;
 
@@ -46,14 +54,15 @@ public class PowerUsageBaseTest {
 
     @Mock
     private LoaderManager mLoaderManager;
+    @Mock
+    private Loader<BatteryUsageStats> mBatteryUsageStatsLoader;
     private TestFragment mFragment;
 
     @Before
     public void setUp() {
         MockitoAnnotations.initMocks(this);
 
-        mFragment = spy(new TestFragment());
+        mFragment = spy(new TestFragment(mLoaderManager));
-        doReturn(mLoaderManager).when(mFragment).getLoaderManager();
     }
 
     @Test
@@ -63,7 +72,62 @@ public class PowerUsageBaseTest {
         verify(mLoaderManager, never()).initLoader(anyInt(), any(Bundle.class), any());
     }
 
-    public static class TestFragment extends PowerUsageBase {
+    @Test
+    public void restartBatteryInfoLoader() {
+        final Bundle bundle = new Bundle();
+        bundle.putInt(KEY_REFRESH_TYPE, BatteryBroadcastReceiver.BatteryUpdateType.BATTERY_STATUS);
+        bundle.putBoolean(KEY_INCLUDE_HISTORY, false);
+        doReturn(mBatteryUsageStatsLoader).when(mLoaderManager).getLoader(
+                PowerUsageBase.LoaderIndex.BATTERY_USAGE_STATS_LOADER);
+        doReturn(false).when(mBatteryUsageStatsLoader).isReset();
+
+        mFragment.restartBatteryStatsLoader(
+                BatteryBroadcastReceiver.BatteryUpdateType.BATTERY_STATUS);
+
+        verify(mLoaderManager)
+                .restartLoader(eq(PowerUsageBase.LoaderIndex.BATTERY_USAGE_STATS_LOADER),
+                        refEq(bundle), any());
+    }
+
+    @Test
+    public void restartBatteryInfoLoader_loaderReset_initLoader() {
+        final Bundle bundle = new Bundle();
+        bundle.putInt(KEY_REFRESH_TYPE, BatteryBroadcastReceiver.BatteryUpdateType.BATTERY_STATUS);
+        bundle.putBoolean(KEY_INCLUDE_HISTORY, false);
+        doReturn(mBatteryUsageStatsLoader).when(mLoaderManager).getLoader(
+                PowerUsageBase.LoaderIndex.BATTERY_USAGE_STATS_LOADER);
+        doReturn(true).when(mBatteryUsageStatsLoader).isReset();
+
+        mFragment.restartBatteryStatsLoader(
+                BatteryBroadcastReceiver.BatteryUpdateType.BATTERY_STATUS);
+
+        verify(mLoaderManager)
+                .initLoader(eq(PowerUsageBase.LoaderIndex.BATTERY_USAGE_STATS_LOADER),
+                        refEq(bundle), any());
+    }
+
+    @Test
+    public void restartBatteryInfoLoader_nullLoader_initLoader() {
+        final Bundle bundle = new Bundle();
+        bundle.putInt(KEY_REFRESH_TYPE, BatteryBroadcastReceiver.BatteryUpdateType.BATTERY_STATUS);
+        bundle.putBoolean(KEY_INCLUDE_HISTORY, false);
+        doReturn(null).when(mLoaderManager).getLoader(
+                PowerUsageBase.LoaderIndex.BATTERY_USAGE_STATS_LOADER);
+
+        mFragment.restartBatteryStatsLoader(
+                BatteryBroadcastReceiver.BatteryUpdateType.BATTERY_STATUS);
+
+        verify(mLoaderManager).initLoader(eq(PowerUsageBase.LoaderIndex.BATTERY_USAGE_STATS_LOADER),
+                refEq(bundle), any());
+    }
+
+    private static class TestFragment extends PowerUsageBase {
+
+        private LoaderManager mLoaderManager;
+
+        TestFragment(LoaderManager loaderManager) {
+            mLoaderManager = loaderManager;
+        }
 
         @Override
         public int getMetricsCategory() {
@@ -94,5 +158,10 @@ public class PowerUsageBaseTest {
         protected List<AbstractPreferenceController> createPreferenceControllers(Context context) {
             return null;
         }
+
+        @Override
+        protected LoaderManager getLoaderManagerForCurrentFragment() {
+            return mLoaderManager;
+        }
     }
 }

tests/robotests/src/com/android/settings/fuelgauge/batteryusage/PowerUsageSummaryTest.java

@@ -15,7 +15,6 @@
  */
 package com.android.settings.fuelgauge.batteryusage;
 
-import static com.android.settings.fuelgauge.batteryusage.PowerUsageSummary.BATTERY_INFO_LOADER;
 import static com.android.settings.fuelgauge.batteryusage.PowerUsageSummary.KEY_BATTERY_ERROR;
 import static com.android.settings.fuelgauge.batteryusage.PowerUsageSummary.KEY_BATTERY_USAGE;
 
@@ -39,14 +38,17 @@ import android.os.Bundle;
 import android.provider.Settings;
 
 import androidx.loader.app.LoaderManager;
+import androidx.loader.content.Loader;
 import androidx.preference.Preference;
 import androidx.preference.PreferenceScreen;
 
 import com.android.settings.R;
 import com.android.settings.SettingsActivity;
 import com.android.settings.fuelgauge.BatteryBroadcastReceiver;
+import com.android.settings.fuelgauge.BatteryInfo;
 import com.android.settings.fuelgauge.BatteryUtils;
 import com.android.settings.fuelgauge.batterytip.BatteryTipPreferenceController;
+import com.android.settings.fuelgauge.batterytip.tips.BatteryTip;
 import com.android.settings.testutils.FakeFeatureFactory;
 import com.android.settings.testutils.XmlTestUtils;
 import com.android.settings.testutils.shadow.ShadowUtils;
@@ -69,8 +71,6 @@ import java.util.List;
 // TODO: Improve this test class so that it starts up the real activity and fragment.
 @RunWith(RobolectricTestRunner.class)
 public class PowerUsageSummaryTest {
-
-    private static final long TIME_SINCE_LAST_FULL_CHARGE_MS = 120 * 60 * 1000;
     private static Intent sAdditionalBatteryInfoIntent;
 
     @BeforeClass
@@ -83,6 +83,10 @@ public class PowerUsageSummaryTest {
     @Mock
     private LoaderManager mLoaderManager;
     @Mock
+    private Loader<BatteryTip> mBatteryTipLoader;
+    @Mock
+    private Loader<BatteryInfo> mBatteryInfoLoader;
+    @Mock
     private ContentResolver mContentResolver;
     @Mock
     private BatteryBroadcastReceiver mBatteryBroadcastReceiver;
@@ -105,11 +109,9 @@ public class PowerUsageSummaryTest {
 
         mRealContext = spy(RuntimeEnvironment.application);
         mFeatureFactory = FakeFeatureFactory.setupForTest();
-        mFragment = spy(new TestFragment(mRealContext));
+        mFragment = spy(new TestFragment(mRealContext, mLoaderManager));
         mFragment.initFeatureProvider();
         doNothing().when(mFragment).restartBatteryStatsLoader(anyInt());
-        doReturn(mock(LoaderManager.class)).when(mFragment).getLoaderManager();
-
         when(mFragment.getActivity()).thenReturn(mSettingsActivity);
         when(mFeatureFactory.powerUsageFeatureProvider.getAdditionalBatteryInfoIntent())
                 .thenReturn(sAdditionalBatteryInfoIntent);
@@ -153,12 +155,58 @@ public class PowerUsageSummaryTest {
     @Test
     public void restartBatteryTipLoader() {
         //TODO: add policy logic here when BatteryTipPolicy is implemented
-        doReturn(mLoaderManager).when(mFragment).getLoaderManager();
+        doReturn(mBatteryTipLoader).when(mLoaderManager).getLoader(
+                PowerUsageBase.LoaderIndex.BATTERY_TIP_LOADER);
+        doReturn(false).when(mBatteryTipLoader).isReset();
 
         mFragment.restartBatteryTipLoader();
 
-        verify(mLoaderManager)
+        verify(mLoaderManager).restartLoader(eq(PowerUsageBase.LoaderIndex.BATTERY_TIP_LOADER),
-                .restartLoader(eq(PowerUsageSummary.BATTERY_TIP_LOADER), eq(Bundle.EMPTY), any());
+                eq(Bundle.EMPTY), any());
+    }
+
+    @Test
+    public void restartBatteryTipLoader_nullLoader_initLoader() {
+        doReturn(null).when(mLoaderManager).getLoader(
+                PowerUsageBase.LoaderIndex.BATTERY_TIP_LOADER);
+
+        mFragment.restartBatteryTipLoader();
+
+        verify(mLoaderManager).initLoader(eq(PowerUsageBase.LoaderIndex.BATTERY_TIP_LOADER),
+                eq(Bundle.EMPTY), any());
+    }
+
+    @Test
+    public void restartBatteryTipLoader_loaderReset_initLoader() {
+        doReturn(mBatteryTipLoader).when(mLoaderManager).getLoader(
+                PowerUsageBase.LoaderIndex.BATTERY_TIP_LOADER);
+        doReturn(true).when(mBatteryTipLoader).isReset();
+
+        mFragment.restartBatteryTipLoader();
+
+
+        verify(mLoaderManager).initLoader(eq(PowerUsageBase.LoaderIndex.BATTERY_TIP_LOADER),
+                eq(Bundle.EMPTY), any());
+    }
+
+
+    @Test
+    public void refreshUi_contextNull_doNothing() {
+        doReturn(null).when(mFragment).getContext();
+
+        mFragment.refreshUi(BatteryBroadcastReceiver.BatteryUpdateType.MANUAL);
+
+        verify(mFragment, never()).restartBatteryTipLoader();
+        verify(mFragment, never()).restartBatteryInfoLoader();
+    }
+
+    @Test
+    public void refreshUi_batteryNotPresent_doNothing() {
+        mFragment.setIsBatteryPresent(false);
+        mFragment.refreshUi(BatteryBroadcastReceiver.BatteryUpdateType.MANUAL);
+
+        verify(mFragment, never()).restartBatteryTipLoader();
+        verify(mFragment, never()).restartBatteryInfoLoader();
     }
 
     @Test
@@ -166,10 +214,12 @@ public class PowerUsageSummaryTest {
         mFragment.mBatteryTipPreferenceController = mock(BatteryTipPreferenceController.class);
         when(mFragment.mBatteryTipPreferenceController.needUpdate()).thenReturn(false);
         mFragment.updateBatteryTipFlag(new Bundle());
+        doNothing().when(mFragment).restartBatteryInfoLoader();
 
         mFragment.refreshUi(BatteryBroadcastReceiver.BatteryUpdateType.MANUAL);
 
         verify(mFragment, never()).restartBatteryTipLoader();
+        verify(mFragment).restartBatteryInfoLoader();
     }
 
     @Test
@@ -177,10 +227,12 @@ public class PowerUsageSummaryTest {
         mFragment.mBatteryTipPreferenceController = mock(BatteryTipPreferenceController.class);
         when(mFragment.mBatteryTipPreferenceController.needUpdate()).thenReturn(true);
         mFragment.updateBatteryTipFlag(new Bundle());
+        doNothing().when(mFragment).restartBatteryInfoLoader();
 
         mFragment.refreshUi(BatteryBroadcastReceiver.BatteryUpdateType.BATTERY_LEVEL);
 
         verify(mFragment, never()).restartBatteryTipLoader();
+        verify(mFragment).restartBatteryInfoLoader();
     }
 
     @Test
@@ -188,10 +240,13 @@ public class PowerUsageSummaryTest {
         mFragment.mBatteryTipPreferenceController = mock(BatteryTipPreferenceController.class);
         when(mFragment.mBatteryTipPreferenceController.needUpdate()).thenReturn(true);
         mFragment.updateBatteryTipFlag(new Bundle());
+        doNothing().when(mFragment).restartBatteryInfoLoader();
+        doNothing().when(mFragment).restartBatteryTipLoader();
 
         mFragment.refreshUi(BatteryBroadcastReceiver.BatteryUpdateType.MANUAL);
 
         verify(mFragment).restartBatteryTipLoader();
+        verify(mFragment).restartBatteryInfoLoader();
     }
 
     @Test
@@ -215,19 +270,68 @@ public class PowerUsageSummaryTest {
     @Test
     public void restartBatteryInfoLoader_contextNull_doNothing() {
         when(mFragment.getContext()).thenReturn(null);
-        when(mFragment.getLoaderManager()).thenReturn(mLoaderManager);
 
         mFragment.restartBatteryInfoLoader();
 
-        verify(mLoaderManager, never()).restartLoader(BATTERY_INFO_LOADER, Bundle.EMPTY,
+        verify(mLoaderManager, never()).restartLoader(
+                PowerUsageBase.LoaderIndex.BATTERY_INFO_LOADER, Bundle.EMPTY,
                 mFragment.mBatteryInfoLoaderCallbacks);
     }
 
-    public static class TestFragment extends PowerUsageSummary {
+    @Test
-        private Context mContext;
+    public void restartBatteryInfoLoader_batteryIsNotPresent_doNothing() {
+        mFragment.setIsBatteryPresent(false);
 
-        public TestFragment(Context context) {
+        mFragment.restartBatteryInfoLoader();
+
+        verify(mLoaderManager, never()).restartLoader(
+                PowerUsageBase.LoaderIndex.BATTERY_INFO_LOADER, Bundle.EMPTY,
+                mFragment.mBatteryInfoLoaderCallbacks);
+    }
+
+    @Test
+    public void restartBatteryInfoLoader() {
+        doReturn(mBatteryInfoLoader).when(mLoaderManager).getLoader(
+                PowerUsageBase.LoaderIndex.BATTERY_INFO_LOADER);
+        doReturn(false).when(mBatteryTipLoader).isReset();
+
+        mFragment.restartBatteryInfoLoader();
+
+        verify(mLoaderManager).restartLoader(eq(PowerUsageBase.LoaderIndex.BATTERY_INFO_LOADER),
+                eq(Bundle.EMPTY), any());
+    }
+
+    @Test
+    public void restartBatteryInfoLoader_nullLoader_initLoader() {
+        doReturn(null).when(mLoaderManager).getLoader(
+                PowerUsageBase.LoaderIndex.BATTERY_INFO_LOADER);
+
+        mFragment.restartBatteryInfoLoader();
+
+        verify(mLoaderManager).initLoader(eq(PowerUsageBase.LoaderIndex.BATTERY_INFO_LOADER),
+                eq(Bundle.EMPTY), any());
+    }
+
+    @Test
+    public void restartBatteryInfoLoader_loaderReset_initLoader() {
+        mFragment.setIsBatteryPresent(true);
+        doReturn(mBatteryInfoLoader).when(mLoaderManager).getLoader(
+                PowerUsageBase.LoaderIndex.BATTERY_INFO_LOADER);
+        doReturn(true).when(mBatteryInfoLoader).isReset();
+
+        mFragment.restartBatteryInfoLoader();
+
+        verify(mLoaderManager).initLoader(eq(PowerUsageBase.LoaderIndex.BATTERY_INFO_LOADER),
+                eq(Bundle.EMPTY), any());
+    }
+
+    private static class TestFragment extends PowerUsageSummary {
+        private Context mContext;
+        private LoaderManager mLoaderManager;
+
+        TestFragment(Context context, LoaderManager loaderManager) {
             mContext = context;
+            mLoaderManager = loaderManager;
         }
 
         @Override
@@ -240,5 +344,15 @@ public class PowerUsageSummaryTest {
             // Override it so we can access this method in test
             return super.getContentResolver();
         }
+
+        public void setIsBatteryPresent(boolean isBatteryPresent) {
+            mIsBatteryPresent = isBatteryPresent;
+        }
+
+        @Override
+        protected LoaderManager getLoaderManagerForCurrentFragment() {
+            return mLoaderManager;
+        }
     }
+
 }