From 46bcdda9b976d4e22d4d58efb40288e6c22d6f61 Mon Sep 17 00:00:00 2001 From: ykhung Date: Tue, 25 May 2021 14:34:50 +0800 Subject: [PATCH] Disable app usage item if this item is not clickable some items are not clickable to launch the restriction page in the battery usage list, we will apply the disabled visual in the preferrence item to improve the UX (avoid users click the item without any action) Bug: 188751551 Bug: 188663505 Test: make SettingsgRoboTests Change-Id: Ib8925b8e191117543bb1c74d6d01191e3043fc73 --- .../BatteryChartPreferenceController.java | 23 ++--- .../settings/fuelgauge/BatteryDiffEntry.java | 85 ++++++++++++++++-- .../settings/fuelgauge/BatteryEntry.java | 11 ++- .../BatteryChartPreferenceControllerTest.java | 10 +-- .../fuelgauge/BatteryDiffEntryTest.java | 90 +++++++++++++++++-- 5 files changed, 178 insertions(+), 41 deletions(-) diff --git a/src/com/android/settings/fuelgauge/BatteryChartPreferenceController.java b/src/com/android/settings/fuelgauge/BatteryChartPreferenceController.java index dbbafe6bdf6..ffbd2d90e4e 100644 --- a/src/com/android/settings/fuelgauge/BatteryChartPreferenceController.java +++ b/src/com/android/settings/fuelgauge/BatteryChartPreferenceController.java @@ -189,6 +189,8 @@ public class BatteryChartPreferenceController extends AbstractPreferenceControll mPrefContext = screen.getContext(); mAppListPrefGroup = screen.findPreference(mPreferenceKey); mAppListPrefGroup.setOrderingAsAdded(false); + mAppListPrefGroup.setTitle( + mPrefContext.getString(R.string.battery_app_usage_for_past_24)); mFooterPreference = screen.findPreference(KEY_FOOTER_PREF); // Removes footer first until usage data is loaded to avoid flashing. if (mFooterPreference != null) { @@ -216,15 +218,6 @@ public class BatteryChartPreferenceController extends AbstractPreferenceControll final BatteryHistEntry histEntry = diffEntry.mBatteryHistEntry; final String packageName = histEntry.mPackageName; final boolean isAppEntry = histEntry.isAppEntry(); - // Checks whether the package is installed or not. - boolean isValidPackage = true; - if (isAppEntry) { - if (mBatteryUtils == null) { - mBatteryUtils = BatteryUtils.getInstance(mPrefContext); - } - isValidPackage = mBatteryUtils.getPackageUid(packageName) - != BatteryUtils.UID_NULL; - } mMetricsFeatureProvider.action( mPrefContext, isAppEntry @@ -233,15 +226,12 @@ public class BatteryChartPreferenceController extends AbstractPreferenceControll new Pair(ConvertUtils.METRIC_KEY_PACKAGE, packageName), new Pair(ConvertUtils.METRIC_KEY_BATTERY_LEVEL, histEntry.mBatteryLevel), new Pair(ConvertUtils.METRIC_KEY_BATTERY_USAGE, powerPref.getPercent())); - Log.d(TAG, String.format("handleClick() label=%s key=%s isValid:%b\n%s", - diffEntry.getAppLabel(), histEntry.getKey(), isValidPackage, histEntry)); - if (isValidPackage) { - AdvancedPowerUsageDetail.startBatteryDetailPage( + Log.d(TAG, String.format("handleClick() label=%s key=%s enntry=\n%s", + diffEntry.getAppLabel(), histEntry.getKey(), histEntry)); + AdvancedPowerUsageDetail.startBatteryDetailPage( mActivity, mFragment, diffEntry, powerPref.getPercent(), isValidToShowSummary(packageName), getSlotInformation()); - return true; - } - return false; + return true; } @Override @@ -434,6 +424,7 @@ public class BatteryChartPreferenceController extends AbstractPreferenceControll pref.setSingleLineTitle(true); // Sets the BatteryDiffEntry to preference for launching detailed page. pref.setBatteryDiffEntry(entry); + pref.setEnabled(entry.validForRestriction()); setPreferenceSummary(pref, entry); if (!isAdded) { mAppListPrefGroup.addPreference(pref); diff --git a/src/com/android/settings/fuelgauge/BatteryDiffEntry.java b/src/com/android/settings/fuelgauge/BatteryDiffEntry.java index 9db29f33fb7..c6b2d4542e2 100644 --- a/src/com/android/settings/fuelgauge/BatteryDiffEntry.java +++ b/src/com/android/settings/fuelgauge/BatteryDiffEntry.java @@ -39,6 +39,8 @@ public class BatteryDiffEntry { static Locale sCurrentLocale = null; // Caches app label and icon to improve loading performance. static final Map sResourceCache = new HashMap<>(); + // Whether a specific item is valid to launch restriction page? + static final Map sValidForRestriction = new HashMap<>(); /** A comparator for {@link BatteryDiffEntry} based on consumed percentage. */ public static final Comparator COMPARATOR = @@ -60,6 +62,7 @@ public class BatteryDiffEntry { @VisibleForTesting String mAppLabel = null; @VisibleForTesting Drawable mAppIcon = null; @VisibleForTesting boolean mIsLoaded = false; + @VisibleForTesting boolean mValidForRestriction = true; public BatteryDiffEntry( Context context, @@ -129,6 +132,12 @@ public class BatteryDiffEntry { ? mDefaultPackageName : mBatteryHistEntry.mPackageName; } + /** Whether this item is valid for users to launch restriction page? */ + public boolean validForRestriction() { + loadLabelAndIcon(); + return mValidForRestriction; + } + /** Whether the current BatteryDiffEntry is system component or not. */ public boolean isSystemEntry() { switch (mBatteryHistEntry.mConsumerType) { @@ -146,7 +155,29 @@ public class BatteryDiffEntry { if (mIsLoaded) { return; } + // Checks whether we have cached data or not first before fetching. + final BatteryEntry.NameAndIcon nameAndIcon = getCache(); + if (nameAndIcon != null) { + mAppLabel = nameAndIcon.name; + mAppIcon = nameAndIcon.icon; + mAppIconId = nameAndIcon.iconId; + } + final Boolean validForRestriction = sValidForRestriction.get(getKey()); + if (validForRestriction != null) { + mValidForRestriction = validForRestriction; + } + // Both nameAndIcon and restriction configuration have cached data. + if (nameAndIcon != null && validForRestriction != null) { + Log.w(TAG, String.format("cannot find cache data nameAndIcon:%s " + + "validForRestriction:%s", nameAndIcon, validForRestriction)); + return; + } mIsLoaded = true; + + // Configures whether we can launch restriction page or not. + updateRestrictionFlagState(); + sValidForRestriction.put(getKey(), Boolean.valueOf(mValidForRestriction)); + // Loads application icon and label based on consumer type. switch (mBatteryHistEntry.mConsumerType) { case ConvertUtils.CONSUMER_TYPE_USER_BATTERY: @@ -156,6 +187,9 @@ public class BatteryDiffEntry { if (nameAndIconForUser != null) { mAppIcon = nameAndIconForUser.icon; mAppLabel = nameAndIconForUser.name; + sResourceCache.put( + getKey(), + new BatteryEntry.NameAndIcon(mAppLabel, mAppIcon, /*iconId=*/ 0)); } break; case ConvertUtils.CONSUMER_TYPE_SYSTEM_BATTERY: @@ -168,15 +202,12 @@ public class BatteryDiffEntry { mAppIconId = nameAndIconForSystem.iconId; mAppIcon = mContext.getDrawable(nameAndIconForSystem.iconId); } + sResourceCache.put( + getKey(), + new BatteryEntry.NameAndIcon(mAppLabel, mAppIcon, mAppIconId)); } break; case ConvertUtils.CONSUMER_TYPE_UID_BATTERY: - final BatteryEntry.NameAndIcon nameAndIcon = getCache(); - if (nameAndIcon != null) { - mAppLabel = nameAndIcon.name; - mAppIcon = nameAndIcon.icon; - break; - } loadNameAndIconForUid(); // Uses application default icon if we cannot find it from package. if (mAppIcon == null) { @@ -186,13 +217,47 @@ public class BatteryDiffEntry { mAppIcon = getBadgeIconForUser(mAppIcon); if (mAppLabel != null || mAppIcon != null) { sResourceCache.put( - mBatteryHistEntry.getKey(), + getKey(), new BatteryEntry.NameAndIcon(mAppLabel, mAppIcon, /*iconId=*/ 0)); } break; } } + @VisibleForTesting + String getKey() { + return mBatteryHistEntry.getKey(); + } + + @VisibleForTesting + void updateRestrictionFlagState() { + mValidForRestriction = true; + if (!mBatteryHistEntry.isAppEntry()) { + return; + } + final boolean isValidPackage = + BatteryUtils.getInstance(mContext).getPackageUid(getPackageName()) + != BatteryUtils.UID_NULL; + if (!isValidPackage) { + mValidForRestriction = false; + return; + } + try { + mValidForRestriction = + mContext.getPackageManager().getPackageInfo( + getPackageName(), + PackageManager.MATCH_DISABLED_COMPONENTS + | PackageManager.MATCH_ANY_USER + | PackageManager.GET_SIGNATURES + | PackageManager.GET_PERMISSIONS) + != null; + } catch (Exception e) { + Log.e(TAG, String.format("getPackageInfo() error %s for package=%s", + e.getCause(), getPackageName())); + mValidForRestriction = false; + } + } + private BatteryEntry.NameAndIcon getCache() { final Locale locale = Locale.getDefault(); if (sCurrentLocale != locale) { @@ -201,7 +266,7 @@ public class BatteryDiffEntry { sCurrentLocale = locale; clearCache(); } - return sResourceCache.get(mBatteryHistEntry.getKey()); + return sResourceCache.get(getKey()); } private void loadNameAndIconForUid() { @@ -258,7 +323,8 @@ public class BatteryDiffEntry { public String toString() { final StringBuilder builder = new StringBuilder() .append("BatteryDiffEntry{") - .append("\n\tname=" + mAppLabel) + .append(String.format("\n\tname=%s restrictable=%b", + mAppLabel, mValidForRestriction)) .append(String.format("\n\tconsume=%.2f%% %f/%f", mPercentOfTotal, mConsumePower, mTotalConsumePower)) .append(String.format("\n\tforeground:%s background:%s", @@ -274,6 +340,7 @@ public class BatteryDiffEntry { static void clearCache() { sResourceCache.clear(); + sValidForRestriction.clear(); } private Drawable getBadgeIconForUser(Drawable icon) { diff --git a/src/com/android/settings/fuelgauge/BatteryEntry.java b/src/com/android/settings/fuelgauge/BatteryEntry.java index 636d2651a8d..125409cd84f 100644 --- a/src/com/android/settings/fuelgauge/BatteryEntry.java +++ b/src/com/android/settings/fuelgauge/BatteryEntry.java @@ -241,7 +241,10 @@ public class BatteryEntry { mBatteryConsumer = null; mIsHidden = false; mPowerComponentId = powerComponentId; - mConsumedPower = devicePowerMah; + mConsumedPower = + powerComponentId == BatteryConsumer.POWER_COMPONENT_SCREEN + ? devicePowerMah + : devicePowerMah - appsPowerMah; mUsageDurationMs = usageDurationMs; mConsumerType = ConvertUtils.CONSUMER_TYPE_SYSTEM_BATTERY; @@ -264,8 +267,10 @@ public class BatteryEntry { iconId = R.drawable.ic_power_system; icon = context.getDrawable(iconId); name = powerComponentName; - - mConsumedPower = devicePowerMah; + mConsumedPower = + powerComponentId == BatteryConsumer.POWER_COMPONENT_SCREEN + ? devicePowerMah + : devicePowerMah - appsPowerMah; mConsumerType = ConvertUtils.CONSUMER_TYPE_SYSTEM_BATTERY; } diff --git a/tests/robotests/src/com/android/settings/fuelgauge/BatteryChartPreferenceControllerTest.java b/tests/robotests/src/com/android/settings/fuelgauge/BatteryChartPreferenceControllerTest.java index 9e2f65d6f3a..606dc194239 100644 --- a/tests/robotests/src/com/android/settings/fuelgauge/BatteryChartPreferenceControllerTest.java +++ b/tests/robotests/src/com/android/settings/fuelgauge/BatteryChartPreferenceControllerTest.java @@ -307,6 +307,7 @@ public final class BatteryChartPreferenceControllerTest { doReturn(appLabel).when(mBatteryDiffEntry).getAppLabel(); doReturn(PREF_KEY).when(mBatteryHistEntry).getKey(); doReturn(null).when(mAppListGroup).findPreference(PREF_KEY); + doReturn(false).when(mBatteryDiffEntry).validForRestriction(); mBatteryChartPreferenceController.addPreferenceToScreen( Arrays.asList(mBatteryDiffEntry)); @@ -324,6 +325,7 @@ public final class BatteryChartPreferenceControllerTest { assertThat(pref.getOrder()).isEqualTo(1); assertThat(pref.getBatteryDiffEntry()).isSameInstanceAs(mBatteryDiffEntry); assertThat(pref.isSingleLineTitle()).isTrue(); + assertThat(pref.isEnabled()).isFalse(); } @Test @@ -353,7 +355,7 @@ public final class BatteryChartPreferenceControllerTest { } @Test - public void testHandlePreferenceTreeClick_validPackageName_returnTrue() { + public void testHandlePreferenceTreeClick_forAppEntry_returnTrue() { doReturn(false).when(mBatteryHistEntry).isAppEntry(); doReturn(mBatteryDiffEntry).when(mPowerGaugePreference).getBatteryDiffEntry(); @@ -371,15 +373,13 @@ public final class BatteryChartPreferenceControllerTest { } @Test - public void testHandlePreferenceTreeClick_appEntryWithInvalidPackage_returnFalse() { + public void testHandlePreferenceTreeClick_forSystemEntry_returnTrue() { mBatteryChartPreferenceController.mBatteryUtils = mBatteryUtils; doReturn(true).when(mBatteryHistEntry).isAppEntry(); - doReturn(BatteryUtils.UID_NULL).when(mBatteryUtils) - .getPackageUid(mBatteryHistEntry.mPackageName); doReturn(mBatteryDiffEntry).when(mPowerGaugePreference).getBatteryDiffEntry(); assertThat(mBatteryChartPreferenceController.handlePreferenceTreeClick( - mPowerGaugePreference)).isFalse(); + mPowerGaugePreference)).isTrue(); verify(mMetricsFeatureProvider) .action( mContext, diff --git a/tests/robotests/src/com/android/settings/fuelgauge/BatteryDiffEntryTest.java b/tests/robotests/src/com/android/settings/fuelgauge/BatteryDiffEntryTest.java index 586016399b3..0df53f1be41 100644 --- a/tests/robotests/src/com/android/settings/fuelgauge/BatteryDiffEntryTest.java +++ b/tests/robotests/src/com/android/settings/fuelgauge/BatteryDiffEntryTest.java @@ -17,12 +17,15 @@ package com.android.settings.fuelgauge; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Matchers.anyInt; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.spy; import android.content.ContentValues; import android.content.Context; import android.content.pm.ApplicationInfo; +import android.content.pm.PackageInfo; import android.content.pm.PackageManager; import android.graphics.drawable.Drawable; import android.os.BatteryConsumer; @@ -56,11 +59,13 @@ public final class BatteryDiffEntryTest { @Mock private Drawable mockDrawable2; @Mock private Drawable mockBadgedDrawable; @Mock private BatteryHistEntry mBatteryHistEntry; + @Mock private PackageInfo mockPackageInfo; @Before public void setUp() { MockitoAnnotations.initMocks(this); mContext = spy(RuntimeEnvironment.application); + doReturn(mContext).when(mContext).getApplicationContext(); doReturn(mockUserManager).when(mContext).getSystemService(UserManager.class); doReturn(mockPackageManager).when(mContext).getPackageManager(); BatteryDiffEntry.clearCache(); @@ -110,6 +115,7 @@ public final class BatteryDiffEntryTest { @Test public void testLoadLabelAndIcon_forSystemBattery_returnExpectedResult() { + final String expectedName = "Ambient display"; // Generates fake testing data. final ContentValues values = getContentValuesWithType( ConvertUtils.CONSUMER_TYPE_SYSTEM_BATTERY); @@ -119,13 +125,22 @@ public final class BatteryDiffEntryTest { final BatteryDiffEntry entry = createBatteryDiffEntry(10, batteryHistEntry); - assertThat(entry.getAppLabel()).isEqualTo("Ambient display"); + assertThat(entry.getAppLabel()).isEqualTo(expectedName); assertThat(entry.getAppIconId()).isEqualTo(R.drawable.ic_settings_aod); - assertThat(BatteryDiffEntry.sResourceCache).isEmpty(); + assertThat(BatteryDiffEntry.sResourceCache).hasSize(1); + // Verifies the app label in the cache. + final BatteryEntry.NameAndIcon nameAndIcon = + BatteryDiffEntry.sResourceCache.get(entry.getKey()); + assertThat(nameAndIcon.name).isEqualTo(expectedName); + assertThat(nameAndIcon.iconId).isEqualTo(R.drawable.ic_settings_aod); + // Verifies the restrictable flag in the cache. + assertThat(entry.mValidForRestriction).isTrue(); + assertThat(BatteryDiffEntry.sValidForRestriction.get(entry.getKey())).isTrue(); } @Test public void testLoadLabelAndIcon_forUserBattery_returnExpectedResult() { + final String expectedName = "Removed user"; doReturn(null).when(mockUserManager).getUserInfo(1001); // Generates fake testing data. final ContentValues values = getContentValuesWithType( @@ -135,10 +150,18 @@ public final class BatteryDiffEntryTest { final BatteryDiffEntry entry = createBatteryDiffEntry(10, batteryHistEntry); - assertThat(entry.getAppLabel()).isEqualTo("Removed user"); + assertThat(entry.getAppLabel()).isEqualTo(expectedName); assertThat(entry.getAppIcon()).isNull(); assertThat(entry.getAppIconId()).isEqualTo(0); - assertThat(BatteryDiffEntry.sResourceCache).isEmpty(); + assertThat(BatteryDiffEntry.sResourceCache).hasSize(1); + // Verifies the app label in the cache. + final BatteryEntry.NameAndIcon nameAndIcon = + BatteryDiffEntry.sResourceCache.get(entry.getKey()); + assertThat(nameAndIcon.name).isEqualTo(expectedName); + assertThat(nameAndIcon.iconId).isEqualTo(0); + // Verifies the restrictable flag in the cache. + assertThat(entry.mValidForRestriction).isTrue(); + assertThat(BatteryDiffEntry.sValidForRestriction.get(entry.getKey())).isTrue(); } @Test @@ -162,8 +185,11 @@ public final class BatteryDiffEntryTest { assertThat(BatteryDiffEntry.sResourceCache).hasSize(1); // Verifies the app label in the cache. final BatteryEntry.NameAndIcon nameAndIcon = - BatteryDiffEntry.sResourceCache.get(batteryHistEntry.getKey()); + BatteryDiffEntry.sResourceCache.get(entry.getKey()); assertThat(nameAndIcon.name).isEqualTo(expectedAppLabel); + // Verifies the restrictable flag in the cache. + assertThat(entry.mValidForRestriction).isFalse(); + assertThat(BatteryDiffEntry.sValidForRestriction.get(entry.getKey())).isFalse(); } @Test @@ -179,7 +205,7 @@ public final class BatteryDiffEntryTest { assertThat(BatteryDiffEntry.sResourceCache).hasSize(1); // Verifies the app label in the cache. final BatteryEntry.NameAndIcon nameAndIcon = - BatteryDiffEntry.sResourceCache.get(batteryHistEntry.getKey()); + BatteryDiffEntry.sResourceCache.get(entry.getKey()); assertThat(nameAndIcon.name).isEqualTo(expectedAppLabel); } @@ -225,10 +251,24 @@ public final class BatteryDiffEntryTest { assertThat(BatteryDiffEntry.sResourceCache).hasSize(1); // Verifies the app label in the cache. final BatteryEntry.NameAndIcon nameAndIcon = - BatteryDiffEntry.sResourceCache.get(entry.mBatteryHistEntry.getKey()); + BatteryDiffEntry.sResourceCache.get(entry.getKey()); assertThat(nameAndIcon.icon).isEqualTo(mockBadgedDrawable); } + @Test + public void testClearCache_clearDataForResourcesAndFlags() { + BatteryDiffEntry.sResourceCache.put( + "fake application key", + new BatteryEntry.NameAndIcon("app label", null, /*iconId=*/ 0)); + BatteryDiffEntry.sValidForRestriction.put( + "fake application key", Boolean.valueOf(false)); + + BatteryDiffEntry.clearCache(); + + assertThat(BatteryDiffEntry.sResourceCache).isEmpty(); + assertThat(BatteryDiffEntry.sValidForRestriction).isEmpty(); + } + @Test public void testClearCache_switchLocale_clearCacheIconAndLabel() throws Exception { final int userId = UserHandle.getUserId(1001); @@ -248,7 +288,7 @@ public final class BatteryDiffEntryTest { assertThat(entry2.getAppIcon()).isEqualTo(mockDrawable2); // Verifies the cache is updated into the new drawable. final BatteryEntry.NameAndIcon nameAndIcon = - BatteryDiffEntry.sResourceCache.get(entry2.mBatteryHistEntry.getKey()); + BatteryDiffEntry.sResourceCache.get(entry2.getKey()); assertThat(nameAndIcon.icon).isEqualTo(mockDrawable2); } @@ -297,6 +337,40 @@ public final class BatteryDiffEntryTest { assertThat(entry.isSystemEntry()).isTrue(); } + @Test + public void testUpdateRestrictionFlagState_updateFlagAsExpected() throws Exception { + final String expectedAppLabel = "fake app label"; + final String fakePackageName = "com.fake.google.com"; + final ContentValues values = getContentValuesWithType( + ConvertUtils.CONSUMER_TYPE_UID_BATTERY); + values.put("uid", /*invalid uid*/ 10001); + values.put("packageName", fakePackageName); + final BatteryDiffEntry entry = + createBatteryDiffEntry(10, new BatteryHistEntry(values)); + + entry.updateRestrictionFlagState(); + // Sets false if the app entry cannot be found. + assertThat(entry.mValidForRestriction).isFalse(); + + doReturn(BatteryUtils.UID_NULL).when(mockPackageManager).getPackageUid( + entry.getPackageName(), PackageManager.GET_META_DATA); + entry.updateRestrictionFlagState(); + // Sets false if the app is invalid package name. + assertThat(entry.mValidForRestriction).isFalse(); + + doReturn(1000).when(mockPackageManager).getPackageUid( + entry.getPackageName(), PackageManager.GET_META_DATA); + entry.updateRestrictionFlagState(); + // Sets false if the app PackageInfo cannot be found. + assertThat(entry.mValidForRestriction).isFalse(); + + doReturn(mockPackageInfo).when(mockPackageManager).getPackageInfo( + eq(entry.getPackageName()), anyInt()); + entry.updateRestrictionFlagState(); + // Sets true if package is valid and PackageInfo can be found. + assertThat(entry.mValidForRestriction).isTrue(); + } + private BatteryDiffEntry createBatteryDiffEntry( int consumerType, long uid, boolean isHidden) { final ContentValues values = getContentValuesWithType(consumerType);