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>
diff --git a/src/com/android/settings/fuelgauge/batteryusage/PowerUsageAdvanced.java b/src/com/android/settings/fuelgauge/batteryusage/PowerUsageAdvanced.java
index e376d85..2dba3c2 100644
--- a/src/com/android/settings/fuelgauge/batteryusage/PowerUsageAdvanced.java
+++ b/src/com/android/settings/fuelgauge/batteryusage/PowerUsageAdvanced.java
@@ -52,7 +52,6 @@
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 @@
bundle.putInt(KEY_REFRESH_TYPE, refreshType);
if (!mIsChartDataLoaded) {
mIsChartDataLoaded = true;
- getLoaderManager().restartLoader(LOADER_BATTERY_USAGE_STATS, bundle,
+ restartLoader(LoaderIndex.BATTERY_HISTORY_LOADER, bundle,
mBatteryHistoryLoaderCallbacks);
}
}
diff --git a/src/com/android/settings/fuelgauge/batteryusage/PowerUsageBase.java b/src/com/android/settings/fuelgauge/batteryusage/PowerUsageBase.java
index ccefdf2..ed3a921 100644
--- a/src/com/android/settings/fuelgauge/batteryusage/PowerUsageBase.java
+++ b/src/com/android/settings/fuelgauge/batteryusage/PowerUsageBase.java
@@ -24,6 +24,7 @@
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.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 @@
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 @@
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);
}
diff --git a/src/com/android/settings/fuelgauge/batteryusage/PowerUsageSummary.java b/src/com/android/settings/fuelgauge/batteryusage/PowerUsageSummary.java
index bca32a7..f266492 100644
--- a/src/com/android/settings/fuelgauge/batteryusage/PowerUsageSummary.java
+++ b/src/com/android/settings/fuelgauge/batteryusage/PowerUsageSummary.java
@@ -65,11 +65,6 @@
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
BatteryUtils mBatteryUtils;
@@ -241,7 +236,7 @@
@VisibleForTesting
void restartBatteryTipLoader() {
- getLoaderManager().restartLoader(BATTERY_TIP_LOADER, Bundle.EMPTY, mBatteryTipsCallbacks);
+ restartLoader(LoaderIndex.BATTERY_TIP_LOADER, Bundle.EMPTY, mBatteryTipsCallbacks);
}
@VisibleForTesting
@@ -274,8 +269,7 @@
if (!mIsBatteryPresent) {
return;
}
- getLoaderManager().restartLoader(BATTERY_INFO_LOADER, Bundle.EMPTY,
- mBatteryInfoLoaderCallbacks);
+ restartLoader(LoaderIndex.BATTERY_INFO_LOADER, Bundle.EMPTY, mBatteryInfoLoaderCallbacks);
}
@VisibleForTesting
diff --git a/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/PowerUsageBaseTest.java b/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/PowerUsageBaseTest.java
index 2700930..6ed10cd 100644
--- a/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/PowerUsageBaseTest.java
+++ b/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 @@
@Mock
private LoaderManager mLoaderManager;
+ @Mock
+ private Loader<BatteryUsageStats> mBatteryUsageStatsLoader;
private TestFragment mFragment;
@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
- mFragment = spy(new TestFragment());
- doReturn(mLoaderManager).when(mFragment).getLoaderManager();
+ mFragment = spy(new TestFragment(mLoaderManager));
}
@Test
@@ -63,7 +72,62 @@
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 @@
protected List<AbstractPreferenceController> createPreferenceControllers(Context context) {
return null;
}
+
+ @Override
+ protected LoaderManager getLoaderManagerForCurrentFragment() {
+ return mLoaderManager;
+ }
}
}
diff --git a/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/PowerUsageSummaryTest.java b/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/PowerUsageSummaryTest.java
index e134490..e44d92c 100644
--- a/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/PowerUsageSummaryTest.java
+++ b/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.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 @@
// 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 @@
@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 @@
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 @@
@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)
- .restartLoader(eq(PowerUsageSummary.BATTERY_TIP_LOADER), eq(Bundle.EMPTY), any());
+ verify(mLoaderManager).restartLoader(eq(PowerUsageBase.LoaderIndex.BATTERY_TIP_LOADER),
+ 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 @@
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 @@
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 @@
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 @@
@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 {
- private Context mContext;
+ @Test
+ 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 @@
// 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;
+ }
}
+
}
