From b2e2bf41a4a8909ac94a2702c403282bb191372d Mon Sep 17 00:00:00 2001 From: Al-Mothafar Al-Hasan Date: Sat, 18 Jul 2026 12:03:15 +0300 Subject: [PATCH] Details refresh: update rows in place, one read per figure per tick (#161) Problem 1: every 3-second foreground tick tore the whole details table down (removeAllViews) and rebuilt it, losing accessibility focus and churning allocations. The table now keeps its rows keyed by label and updates the value cells in place; rows are only created/removed when their key appears or vanishes (rate warm-up, time-to-full gating). The diffing (syncRows) is a static binder-driven helper with Robolectric tests. Problem 2: redundant system reads per tick. BatteryDO now carries the OS cycle count from the same intent everything else comes from; the fragment passes the snapshot capacity and cycle count into new BatteryHealthTracker overloads instead of re-reading BatteryManager and the sticky broadcast. Insights does one sticky read and one capacity estimate per refresh, feeding pure estimatedHealthForCycles/gradeForCycles cores. Problem 3: MainActivity looked up the gauge (and details fragment) on every tick/lifecycle hop; both are now fields set in onCreate. Closes #161 Co-Authored-By: Claude Fable 5 --- .../model/BatteryDO.java | 12 ++ .../service/BatteryHealthTracker.java | 77 +++++--- .../service/SystemService.java | 11 +- .../ui/BatteryInsightsActivity.java | 27 ++- .../ui/MainActivity.java | 40 ++--- .../ui/fragment/BatteryDetailsFragment.java | 167 ++++++++++++++---- .../BatteryHealthTrackerStateTest.java | 4 +- .../service/BatteryHealthTrackerTest.java | 62 +++++++ .../service/SystemServiceTest.java | 56 +++++- .../fragment/BatteryDetailsRowSyncTest.java | 132 ++++++++++++++ 10 files changed, 503 insertions(+), 85 deletions(-) create mode 100644 app/src/test/java/com/almothafar/simplebatterynotifier/ui/fragment/BatteryDetailsRowSyncTest.java diff --git a/app/src/main/java/com/almothafar/simplebatterynotifier/model/BatteryDO.java b/app/src/main/java/com/almothafar/simplebatterynotifier/model/BatteryDO.java index 83ed721..d1f8ffb 100644 --- a/app/src/main/java/com/almothafar/simplebatterynotifier/model/BatteryDO.java +++ b/app/src/main/java/com/almothafar/simplebatterynotifier/model/BatteryDO.java @@ -25,6 +25,9 @@ public final class BatteryDO { // Instantaneous current in µA from BATTERY_PROPERTY_CURRENT_NOW; Integer.MIN_VALUE when the device // doesn't report it. Sign convention varies by OEM, so callers derive direction from the status. private int currentMicroAmps = Integer.MIN_VALUE; + // OS-reported charge cycle count from EXTRA_CYCLE_COUNT (Android 14+); -1 when the device doesn't + // report it. Carried on the snapshot so per-tick consumers don't re-read the sticky broadcast (#159/#161). + private int cycleCount = -1; private String technology; private String powerSource; private String health; @@ -212,6 +215,15 @@ public BatteryDO setCurrentMicroAmps(final int currentMicroAmps) { return this; } + public int getCycleCount() { + return cycleCount; + } + + public BatteryDO setCycleCount(final int cycleCount) { + this.cycleCount = cycleCount; + return this; + } + public String getPowerSource() { return powerSource; } diff --git a/app/src/main/java/com/almothafar/simplebatterynotifier/service/BatteryHealthTracker.java b/app/src/main/java/com/almothafar/simplebatterynotifier/service/BatteryHealthTracker.java index a5709d8..b47d144 100644 --- a/app/src/main/java/com/almothafar/simplebatterynotifier/service/BatteryHealthTracker.java +++ b/app/src/main/java/com/almothafar/simplebatterynotifier/service/BatteryHealthTracker.java @@ -189,7 +189,24 @@ public static int getEffectiveCycleCount(final Context context) { if (isNull(context)) { return 0; } - final int osCycleCount = SystemService.getChargeCycleCount(context); + return getEffectiveCycleCount(context, SystemService.getChargeCycleCount(context)); + } + + /** + * Same as {@link #getEffectiveCycleCount(Context)}, for callers that already hold the OS cycle + * count (e.g. from a {@code BatteryDO} snapshot) — avoids a second sticky-broadcast read on the + * per-tick refresh path (#161). + * + * @param context Application context + * @param osCycleCount the already-read OS cycle count, or -1/0 when the device doesn't report one + * + * @return Charge cycle count (OS-reported if available, else the tracked estimate) plus any + * debug-injected cycles + */ + public static int getEffectiveCycleCount(final Context context, final int osCycleCount) { + if (isNull(context)) { + return 0; + } final int base = osCycleCount > 0 ? osCycleCount : getChargeCycles(context); return base + getDebugChargeCycles(context); } @@ -201,17 +218,15 @@ public static int getEffectiveCycleCount(final Context context) { * The distinction matters for honesty (#114): the OS count covers the battery's whole life, while * the app-tracked estimate only counts charge delivered since the app was installed — on an older * phone it starts at zero and therefore understates real wear. The insights screen labels the - * health basis differently for the two sources. + * health basis differently for the two sources. Takes the already-read count so the caller's + * single sticky read serves both this and {@link #getEffectiveCycleCount(Context, int)} (#161). * - * @param context Application context + * @param osCycleCount the already-read OS cycle count, or -1/0 when the device doesn't report one * * @return true when the OS reports a cycle count for this device */ - public static boolean isCycleCountFromOs(final Context context) { - if (isNull(context)) { - return false; - } - return SystemService.getChargeCycleCount(context) > 0; + public static boolean isCycleCountFromOs(final int osCycleCount) { + return osCycleCount > 0; } /** @@ -345,17 +360,18 @@ public static void setDesignCapacity(final Context context, final int mAh) { * capacity. It is shown regardless of charge level so it stays consistent with the displayed * Capacity (#103); when no design capacity is set, callers fall back to the cycle-based estimate. * - * @param context Application context + * @param context Application context + * @param currentFullMah the already-read current full capacity estimate in mAh (0 when unknown), + * e.g. {@code batteryDO.getCapacity()} — passed in so one capacity estimate + * serves the whole refresh (#161) * * @return Measured health percentage (1-100), or -1 when it cannot be determined */ - public static int getMeasuredHealthPercentage(final Context context) { + public static int getMeasuredHealthPercentage(final Context context, final int currentFullMah) { if (isNull(context)) { return -1; } - return computeMeasuredHealth( - SystemService.getBatteryCapacity(context), - getDesignCapacity(context)); + return computeMeasuredHealth(currentFullMah, getDesignCapacity(context)); } /** @@ -394,18 +410,19 @@ static int computeMeasuredHealth(final int currentFullMah, final int designMah) * keeps the figure but adds a warning — see issue #94. A genuinely worn-out battery can also read * this low, a possibility the UI surfaces to the user. * - * @param context Application context + * @param context Application context + * @param currentFullMah the already-read current full capacity estimate in mAh (0 when unknown), + * e.g. {@code batteryDO.getCapacity()} — passed in so one capacity estimate + * serves the whole refresh (#161) * * @return true when a design capacity is set, a charge-counter estimate exists, and the estimate is * outside the plausible window around the design capacity */ - public static boolean isBatteryReadingUnreliable(final Context context) { + public static boolean isBatteryReadingUnreliable(final Context context, final int currentFullMah) { if (isNull(context)) { return false; } - return isEstimateImplausible( - SystemService.getBatteryCapacity(context), - getDesignCapacity(context)); + return isEstimateImplausible(currentFullMah, getDesignCapacity(context)); } /** @@ -492,8 +509,18 @@ public static int getDaysSinceFirstUse(final Context context) { * @return Estimated battery health percentage (0-100) */ public static int getEstimatedHealthPercentage(final Context context) { - final int cycles = getEffectiveCycleCount(context); + return estimatedHealthForCycles(getEffectiveCycleCount(context)); + } + /** + * Pure core of {@link #getEstimatedHealthPercentage}, taking an already-known cycle count so a + * caller's single read can serve every cycle-derived figure on the screen (#161). + * + * @param cycles effective charge cycle count + * + * @return Estimated battery health percentage (40-100) + */ + public static int estimatedHealthForCycles(final int cycles) { // Excellent health: 0-300 cycles (100% to 95%) if (cycles < EXCELLENT_THRESHOLD) { return 100 - (cycles * 5 / EXCELLENT_THRESHOLD); @@ -527,8 +554,18 @@ public static int getEstimatedHealthPercentage(final Context context) { * @return the matching {@link BatteryHealthGrade} */ public static BatteryHealthGrade getHealthGrade(final Context context) { - final int cycles = getEffectiveCycleCount(context); + return gradeForCycles(getEffectiveCycleCount(context)); + } + /** + * Pure core of {@link #getHealthGrade}, taking an already-known cycle count (#161). Uses the same + * thresholds as {@link #estimatedHealthForCycles} so percentage and grade always agree. + * + * @param cycles effective charge cycle count + * + * @return the matching {@link BatteryHealthGrade} + */ + public static BatteryHealthGrade gradeForCycles(final int cycles) { if (cycles < EXCELLENT_THRESHOLD) { return BatteryHealthGrade.EXCELLENT; } else if (cycles < GOOD_THRESHOLD) { diff --git a/app/src/main/java/com/almothafar/simplebatterynotifier/service/SystemService.java b/app/src/main/java/com/almothafar/simplebatterynotifier/service/SystemService.java index c022cda..bda8063 100644 --- a/app/src/main/java/com/almothafar/simplebatterynotifier/service/SystemService.java +++ b/app/src/main/java/com/almothafar/simplebatterynotifier/service/SystemService.java @@ -150,7 +150,12 @@ private static BatteryExtras extractBatteryExtras(final Intent batteryStatus) { final boolean present = extras != null && extras.getBoolean(BatteryManager.EXTRA_PRESENT); final String technology = extras != null ? extras.getString(BatteryManager.EXTRA_TECHNOLOGY) : ""; - return new BatteryExtras(level, scale, status, health, plugged, temperature, voltage, present, technology); + // Absent below Android 14 (and on devices whose fuel gauge doesn't report it) — normalized to + // -1 like getChargeCycleCount, so snapshot consumers don't need a second sticky read (#161). + final int rawCycleCount = batteryStatus.getIntExtra(BatteryManager.EXTRA_CYCLE_COUNT, -1); + final int cycleCount = rawCycleCount > 0 ? rawCycleCount : -1; + + return new BatteryExtras(level, scale, status, health, plugged, temperature, voltage, present, technology, cycleCount); } /** @@ -210,6 +215,7 @@ private static BatteryDO buildBatteryDataObject(final BatteryExtras extras, .setCapacity(batteryCapacity) .setCurrentMicroAmps(currentMicroAmps) .setChargeCounterMicroAmpHours(chargeCounterUah) + .setCycleCount(extras.cycleCount) .setIntHealth(extras.health); // Determine health status and set it on the battery object @@ -614,6 +620,7 @@ private record BatteryExtras(int level, int temperature, int voltage, boolean present, - String technology) { + String technology, + int cycleCount) { } } diff --git a/app/src/main/java/com/almothafar/simplebatterynotifier/ui/BatteryInsightsActivity.java b/app/src/main/java/com/almothafar/simplebatterynotifier/ui/BatteryInsightsActivity.java index 6eab747..7d0d28e 100644 --- a/app/src/main/java/com/almothafar/simplebatterynotifier/ui/BatteryInsightsActivity.java +++ b/app/src/main/java/com/almothafar/simplebatterynotifier/ui/BatteryInsightsActivity.java @@ -91,16 +91,22 @@ protected void onResume() { * Updates all health data displays with current values from BatteryHealthTracker. */ private void updateHealthData() { + // One sticky read and one capacity estimate per refresh (#161): every cycle- and + // capacity-derived figure below is computed from these two values. + final int osCycles = SystemService.getChargeCycleCount(this); + final int cycles = BatteryHealthTracker.getEffectiveCycleCount(this, osCycles); + final int capacityMah = SystemService.getBatteryCapacity(this); + // Always show the resolved health figure (measured, else cycle-based). - showResolvedHealth(); + showResolvedHealth(cycles, BatteryHealthTracker.isCycleCountFromOs(osCycles), capacityMah); // When the device's charge counter can't be trusted (#94) the figure may be wrong: keep showing // it, but flag it with a tappable warning that explains why (and the failing-battery edge case). - healthWarningIcon.setVisibility(BatteryHealthTracker.isBatteryReadingUnreliable(this) + healthWarningIcon.setVisibility(BatteryHealthTracker.isBatteryReadingUnreliable(this, capacityMah) ? View.VISIBLE : View.GONE); // Metrics and the design-capacity row are shown the same way in every state. - chargeCyclesText.setText(String.valueOf(BatteryHealthTracker.getEffectiveCycleCount(this))); + chargeCyclesText.setText(String.valueOf(cycles)); daysInUseText.setText(String.valueOf(BatteryHealthTracker.getDaysSinceFirstUse(this))); final int designCapacity = BatteryHealthTracker.getDesignCapacity(this); @@ -113,17 +119,22 @@ private void updateHealthData() { /** * Shows the resolved health figure: the measured percentage (current capacity vs. user-entered * design capacity) when available, otherwise the cycle-based estimate. See issue #32 / #7. + * Works entirely from the values the caller already read this refresh (#161). + * + * @param cycles the effective charge cycle count for this refresh + * @param cyclesFromOs whether that count came from the OS (labels the basis honestly, #114) + * @param capacityMah the current full capacity estimate in mAh (0 when unknown) */ - private void showResolvedHealth() { - final int measuredHealth = BatteryHealthTracker.getMeasuredHealthPercentage(this); + private void showResolvedHealth(final int cycles, final boolean cyclesFromOs, final int capacityMah) { + final int measuredHealth = BatteryHealthTracker.getMeasuredHealthPercentage(this, capacityMah); final boolean measured = measuredHealth >= 0; final int healthPercentage = measured ? measuredHealth - : BatteryHealthTracker.getEstimatedHealthPercentage(this); + : BatteryHealthTracker.estimatedHealthForCycles(cycles); final BatteryHealthGrade grade = measured ? BatteryHealthTracker.gradeForPercentage(measuredHealth) - : BatteryHealthTracker.getHealthGrade(this); + : BatteryHealthTracker.gradeForCycles(cycles); // Update health percentage and color it based on grade healthPercentageText.setText(healthPercentage + "%"); @@ -138,7 +149,7 @@ private void showResolvedHealth() { // wear on a phone older than the app (#114). final int basisRes = measured ? R.string.health_basis_measured - : BatteryHealthTracker.isCycleCountFromOs(this) + : cyclesFromOs ? R.string.health_basis_estimated_os : R.string.health_basis_estimated_tracked; healthBasisText.setText(basisRes); diff --git a/app/src/main/java/com/almothafar/simplebatterynotifier/ui/MainActivity.java b/app/src/main/java/com/almothafar/simplebatterynotifier/ui/MainActivity.java index 0b7ae46..96064bf 100644 --- a/app/src/main/java/com/almothafar/simplebatterynotifier/ui/MainActivity.java +++ b/app/src/main/java/com/almothafar/simplebatterynotifier/ui/MainActivity.java @@ -28,7 +28,6 @@ import androidx.annotation.NonNull; import androidx.appcompat.widget.Toolbar; import androidx.core.content.ContextCompat; -import androidx.fragment.app.FragmentManager; import androidx.preference.PreferenceManager; import com.almothafar.simplebatterynotifier.R; import com.almothafar.simplebatterynotifier.model.BatteryDO; @@ -62,9 +61,12 @@ public class MainActivity extends BaseActivity { private ActivityResultLauncher settingsLauncher; private ActivityResultLauncher notificationPermissionLauncher; - // UI elements + // UI elements, looked up once in onCreate — the refresh loop runs every 3 s, so per-tick + // findViewById/findFragmentById traversals are pointless work (#161). private MaterialButton batteryInsightsButton; private RangeSlider thresholdSlider; + private HorseshoeProgressBar batteryGauge; + private BatteryDetailsFragment batteryDetailsFragment; // Self-reposting refresh loop, bound to the foreground lifecycle (started in onPostResume, // stopped in onPause) so it never stacks across resumes or keeps polling in the background. @@ -165,6 +167,9 @@ protected void onCreate(final Bundle savedInstanceState) { // Initialize UI elements batteryInsightsButton = findViewById(R.id.batteryInsightsButton); + batteryGauge = findViewById(R.id.batteryPercentage); + // The details fragment is declared in the layout, so it exists for the activity's lifetime. + batteryDetailsFragment = (BatteryDetailsFragment) getSupportFragmentManager().findFragmentById(R.id.detailsFragmentLayout); // Set up button click listeners batteryInsightsButton.setOnClickListener(v -> openBatteryInsights()); @@ -195,8 +200,7 @@ protected void onPostResume() { // Resume the motion paused in onPause(); restarts only what the battery state still // warrants (charging/discharging wave, full pulse, or critical breathing). - final HorseshoeProgressBar gauge = findViewById(R.id.batteryPercentage); - gauge.resumeAnimations(); + batteryGauge.resumeAnimations(); } /** @@ -210,8 +214,7 @@ protected void onPause() { // Motion is only auto-stopped when the view is destroyed (onDetachedFromWindow), // not on backgrounding, so pause it here for the same reason we stop the timer. - final HorseshoeProgressBar gauge = findViewById(R.id.batteryPercentage); - gauge.pauseAnimations(); + batteryGauge.pauseAnimations(); } /** @@ -267,21 +270,16 @@ private void stopUpdateTimer() { * Runs on the main thread via {@link #handler}. */ private void refreshBatteryUi() { - final HorseshoeProgressBar gauge = findViewById(R.id.batteryPercentage); fillBatteryInfo(); - gauge.setLevel(batteryPercentage); - gauge.setTitle(batteryPercentageText); - gauge.setStatusText(subTitle); + batteryGauge.setLevel(batteryPercentage); + batteryGauge.setTitle(batteryPercentageText); + batteryGauge.setStatusText(subTitle); // Drive the gauge motion: charging wave, full-on-charger idle pulse, or discharge wave. if (nonNull(batteryDO)) { - gauge.setFlow(flowOf(batteryDO.getStatus())); + batteryGauge.setFlow(flowOf(batteryDO.getStatus())); } - final FragmentManager fragmentManager = getSupportFragmentManager(); - final BatteryDetailsFragment batteryDetailsFragment = - (BatteryDetailsFragment) fragmentManager.findFragmentById(R.id.detailsFragmentLayout); - if (nonNull(batteryDetailsFragment) && nonNull(batteryDO)) { batteryDetailsFragment.updateBatteryDetails(batteryDO); } @@ -309,8 +307,7 @@ private void initializeFirstValues() { fillBatteryInfo(); final SharedPreferences sharedPref = PreferenceManager.getDefaultSharedPreferences(this); - final HorseshoeProgressBar gauge = findViewById(R.id.batteryPercentage); - gauge.setThresholds(sharedPref.getInt(getString(R.string._pref_key_critical_battery_level), 20), + batteryGauge.setThresholds(sharedPref.getInt(getString(R.string._pref_key_critical_battery_level), 20), sharedPref.getInt(getString(R.string._pref_key_warn_battery_level), 40)); // Keep the in-fly slider in sync with values that may have changed in Settings. @@ -318,9 +315,9 @@ private void initializeFirstValues() { // The ring still animates whole levels; only the final title carries the decimals (#158). // Intermediate steps count up in whole percent, then the last step lands on the precise text. - gauge.animateLevelTo(batteryPercentage, progress -> { - gauge.setTitle(progress >= batteryPercentage ? batteryPercentageText : BatteryPercentFormatter.formatWhole(progress)); - gauge.setStatusText(subTitle); + batteryGauge.animateLevelTo(batteryPercentage, progress -> { + batteryGauge.setTitle(progress >= batteryPercentage ? batteryPercentageText : BatteryPercentFormatter.formatWhole(progress)); + batteryGauge.setStatusText(subTitle); }); } @@ -367,8 +364,7 @@ public void onStopTrackingTouch(@NonNull final RangeSlider slider) { .putInt(getString(R.string._pref_key_warn_battery_level), warning) .apply(); - final HorseshoeProgressBar gauge = findViewById(R.id.batteryPercentage); - gauge.setThresholds(critical, warning); + batteryGauge.setThresholds(critical, warning); } }); diff --git a/app/src/main/java/com/almothafar/simplebatterynotifier/ui/fragment/BatteryDetailsFragment.java b/app/src/main/java/com/almothafar/simplebatterynotifier/ui/fragment/BatteryDetailsFragment.java index 00d20e3..4229838 100644 --- a/app/src/main/java/com/almothafar/simplebatterynotifier/ui/fragment/BatteryDetailsFragment.java +++ b/app/src/main/java/com/almothafar/simplebatterynotifier/ui/fragment/BatteryDetailsFragment.java @@ -54,19 +54,30 @@ public class BatteryDetailsFragment extends Fragment { // #173: the avg line under the Current value renders at this fraction of the cell's text size. private static final float AVG_LINE_SIZE_RATIO = 0.8f; + // The value TextView is the third cell of every row (label, separator, value). + private static final int VALUE_CELL_INDEX = 2; + private BatteryDO batteryDO; private Map valuesMap; private View viewRef; + // The rows currently in the table, keyed by label (#161): each refresh updates the value cells in + // place and only adds/removes rows whose keys changed, instead of rebuilding the whole table — + // which lost accessibility focus every 3 s and churned allocations. Rebuilt with the view. + private final Map rowViews = new LinkedHashMap<>(); + private TableLayout tableLayout; + private int cellPadding; + private int cellPaddingTop; + // #94: when this device's charge counter can't be trusted, the Capacity row shows "Unknown" with a // tappable info icon instead of a misleading mAh figure. Computed in fillBatteryInfo, applied by - // createDetailsTable to just the capacity row. + // the row binder to just the capacity row. private boolean capacityUnreliable; private String capacityLabel; // #108: the drain-rate row's label, and the colour applied to its value cell (amber near the user's // limit, red at/above it, while discharging; 0 = no special colour). Both set in fillBatteryInfo and - // applied by createDetailsTable to just that row, mirroring the capacity special-case above. + // applied by the row binder to just that row, mirroring the capacity special-case above. private String rateLabel; private int rateValueColor; @@ -94,11 +105,13 @@ public View onCreateView(final LayoutInflater inflater, this.viewRef = view; batteryDO = SystemService.getBatteryInfo(view.getContext()); + setupTable(view); + // CRITICAL: Check for null batteryDO if (isNull(batteryDO)) { Log.w(TAG, "Unable to retrieve battery information"); } else { - createDetailsTable(view); + refreshDetailsTable(view); } maybeShowScrollHint(view); @@ -107,15 +120,14 @@ public View onCreateView(final LayoutInflater inflater, } /** - * Create and populate the battery details table + * One-time table setup for a freshly inflated view: column behaviour, cached paddings, and a clean + * row registry (the old view's rows are gone with it). * * @param view The fragment view containing the table */ - public void createDetailsTable(final View view) { - fillBatteryInfo(view); - - final TableLayout tableLayout = view.findViewById(R.id.batteryDetailsTable); - tableLayout.removeAllViews(); + private void setupTable(final View view) { + tableLayout = view.findViewById(R.id.batteryDetailsTable); + rowViews.clear(); // Colon-aligned rows with the divider near the horizontal centre (#96): stretch BOTH the label (0) // and value (2) columns so they share the width evenly, and the end-aligned labels' colons line up // around the middle in both LTR and RTL (rather than wherever the widest label happens to end). @@ -126,21 +138,96 @@ public void createDetailsTable(final View view) { tableLayout.setColumnStretchable(2, true); tableLayout.setColumnShrinkable(0, true); tableLayout.setColumnShrinkable(2, true); - final int cellPadding = getResources().getDimensionPixelSize(R.dimen.battery_details_cell_padding); - final int cellPaddingTop = getResources().getDimensionPixelSize(R.dimen.battery_details_cell_padding_top); - - int rowIndex = 0; - for (final String key : valuesMap.keySet()) { - // Only the capacity row gets the "unreliable reading" info affordance (#94). - final boolean unreliable = capacityUnreliable && key.equals(capacityLabel); - // Only the drain-rate row is coloured (amber/red near the limit while discharging) — #108. - final int valueColor = (rateValueColor != 0 && key.equals(rateLabel)) ? rateValueColor : 0; - final TableRow row = createTableRow(view, key, valuesMap.get(key), cellPadding, cellPaddingTop, unreliable, valueColor); - tableLayout.addView(row, rowIndex); - rowIndex++; + cellPadding = getResources().getDimensionPixelSize(R.dimen.battery_details_cell_padding); + cellPaddingTop = getResources().getDimensionPixelSize(R.dimen.battery_details_cell_padding_top); + } + + /** + * Refresh the battery details table with the current {@code batteryDO}. + *

+ * Steady-state refreshes (every 3 s while the screen is open) update the existing rows' value + * cells in place; rows are only created or removed when their key genuinely appears or vanishes + * (rate warm-up, time-to-full gating, capacity turning unknown). This keeps accessibility focus, + * scroll position and the view tree stable across refreshes (#161). + * + * @param view The fragment view containing the table + */ + private void refreshDetailsTable(final View view) { + fillBatteryInfo(view); + + syncRows(tableLayout, rowViews, valuesMap, new RowBinder() { + @Override + public TableRow createRow(final String label, final CharSequence value) { + return createTableRow(view, label, value, cellPadding, cellPaddingTop, isUnreliableRow(label), valueColorFor(label)); + } + + @Override + public void bindValue(final TableRow row, final String label, final CharSequence value) { + final TextView valueView = (TextView) row.getChildAt(VALUE_CELL_INDEX); + valueView.setText(value); + applyValueDecorations(valueView, isUnreliableRow(label), valueColorFor(label)); + } + }); + } + + /** Only the capacity row gets the "unreliable reading" info affordance (#94). */ + private boolean isUnreliableRow(final String label) { + return capacityUnreliable && label.equals(capacityLabel); + } + + /** Only the drain-rate row is coloured (amber/red near the limit while discharging) — #108. */ + private int valueColorFor(final String label) { + return (rateValueColor != 0 && label.equals(rateLabel)) ? rateValueColor : 0; + } + + /** + * Bring the table's rows in line with the desired label→value map: bind existing rows' value + * cells in place, remove rows whose label vanished, and insert new rows at their position. + * Relative order of surviving rows never changes (rows only appear/disappear), so inserting at + * the walk index keeps every row where it belongs. Static and binder-driven so the diffing is + * unit-testable without a fragment (#161). + * + * @param tableLayout the table being updated + * @param rowViews the rows currently in the table, keyed by label — updated in place + * @param values the desired rows for this refresh, in display order + * @param binder creates a full row for a new label / rebinds the value cell of an existing one + */ + static void syncRows(final TableLayout tableLayout, final Map rowViews, + final Map values, final RowBinder binder) { + // Drop rows whose label is gone this refresh. + rowViews.entrySet().removeIf(entry -> { + if (values.containsKey(entry.getKey())) { + return false; + } + tableLayout.removeView(entry.getValue()); + return true; + }); + + int index = 0; + for (final Map.Entry entry : values.entrySet()) { + TableRow row = rowViews.get(entry.getKey()); + if (isNull(row)) { + row = binder.createRow(entry.getKey(), entry.getValue()); + tableLayout.addView(row, index); + rowViews.put(entry.getKey(), row); + } else { + binder.bindValue(row, entry.getKey(), entry.getValue()); + } + index++; } } + /** + * How {@link #syncRows} materializes rows: create a complete row for a label that just appeared, + * or rebind the value cell of a row that persists across refreshes. + */ + interface RowBinder { + + TableRow createRow(String label, CharSequence value); + + void bindValue(TableRow row, String label, CharSequence value); + } + /** * Update the battery details with new data * @@ -149,7 +236,7 @@ public void createDetailsTable(final View view) { public void updateBatteryDetails(final BatteryDO batteryDO) { this.batteryDO = batteryDO; if (nonNull(viewRef) && nonNull(batteryDO)) { - this.createDetailsTable(viewRef); + refreshDetailsTable(viewRef); } } @@ -305,14 +392,28 @@ private TextView createValueTextView(final View view, final TextView textView = new TextView(view.getContext()); textView.setTextAppearance(R.style.DefaultTextStyle); textView.setText(text); - textView.setTextColor(valueColor != 0 - ? valueColor - : GeneralHelper.getColor(getResources(), R.color.battery_details_value_color)); // Start-align + relative padding + locale text direction so numeric (Latin) values sit right after // the colon like the Arabic text values do, instead of flying to the far edge in RTL (#96). textView.setGravity(Gravity.START); textView.setTextDirection(View.TEXT_DIRECTION_LOCALE); textView.setPaddingRelative(cellPadding, 0, 0, cellPaddingTop); + applyValueDecorations(textView, unreliable, valueColor); + return textView; + } + + /** + * (Re-)apply the per-refresh decorations of a value cell: the rate colour (#108) and the capacity + * row's tappable "unreliable reading" affordance (#94). Called at creation and on every in-place + * rebind (#161), so it also has to clear both when the row's state changes back. + * + * @param textView the value cell + * @param unreliable when true, decorate with the tappable amber info icon; when false, remove it + * @param valueColor colour for the value text, or 0 for the default value colour + */ + private void applyValueDecorations(final TextView textView, final boolean unreliable, final int valueColor) { + textView.setTextColor(valueColor != 0 + ? valueColor + : GeneralHelper.getColor(getResources(), R.color.battery_details_value_color)); if (unreliable) { // #94: amber warning affordance after the value; tap to learn why the reading can't be trusted. // Same mark as the insights health figure, so the two screens read consistently. @@ -320,8 +421,12 @@ private TextView createValueTextView(final View view, textView.setCompoundDrawablePadding((int) (6 * getResources().getDisplayMetrics().density)); textView.setContentDescription(getString(R.string.battery_reading_unreliable_cd)); textView.setOnClickListener(v -> showCapacityUnreliableDialog()); + } else { + textView.setCompoundDrawablesRelativeWithIntrinsicBounds(0, 0, 0, 0); + textView.setContentDescription(null); + textView.setOnClickListener(null); + textView.setClickable(false); } - return textView; } /** @@ -341,9 +446,10 @@ private void fillBatteryInfo(final View view) { // Capacity is an estimate from BatteryManager. Show "Unknown" rather than "0 mAh" when the device // doesn't report the charge counter, and also when the reported figure can't be trusted (#94) — - // in that case createDetailsTable adds a tappable info icon explaining why. + // in that case the row binder adds a tappable info icon explaining why. The snapshot's capacity + // is passed down so no second capacity estimate is read this tick (#161). final int capacity = batteryDO.getCapacity(); - capacityUnreliable = BatteryHealthTracker.isBatteryReadingUnreliable(view.getContext()); + capacityUnreliable = BatteryHealthTracker.isBatteryReadingUnreliable(view.getContext(), capacity); final String capacityText = (capacity > 0 && !capacityUnreliable) ? capacity + " mAh" : getResources().getString(R.string.unknown); @@ -358,8 +464,9 @@ private void fillBatteryInfo(final View view) { getResources().getString(R.string.design_capacity_value, String.valueOf(designCapacity))); } - // Add charge cycles from the battery health tracker - positioned right after capacity - final int chargeCycles = BatteryHealthTracker.getEffectiveCycleCount(view.getContext()); + // Add charge cycles from the battery health tracker - positioned right after capacity. The + // snapshot already carries the OS cycle count, so this triggers no extra sticky read (#161). + final int chargeCycles = BatteryHealthTracker.getEffectiveCycleCount(view.getContext(), batteryDO.getCycleCount()); valuesMap.put(getResources().getString(R.string.charge_cycles), String.valueOf(chargeCycles)); valuesMap.put(getResources().getString(R.string.voltage), batteryDO.getVoltage() + " mV"); diff --git a/app/src/test/java/com/almothafar/simplebatterynotifier/service/BatteryHealthTrackerStateTest.java b/app/src/test/java/com/almothafar/simplebatterynotifier/service/BatteryHealthTrackerStateTest.java index b82e864..910d2f9 100644 --- a/app/src/test/java/com/almothafar/simplebatterynotifier/service/BatteryHealthTrackerStateTest.java +++ b/app/src/test/java/com/almothafar/simplebatterynotifier/service/BatteryHealthTrackerStateTest.java @@ -104,8 +104,8 @@ public void designCapacity_persistsAndClears() { @Test public void measuredHealth_requiresDesignCapacity() { - // Without a design capacity there's nothing to measure against - assertEquals(-1, BatteryHealthTracker.getMeasuredHealthPercentage(context)); + // Without a design capacity there's nothing to measure against, whatever capacity was read + assertEquals(-1, BatteryHealthTracker.getMeasuredHealthPercentage(context, 4400)); } @Test diff --git a/app/src/test/java/com/almothafar/simplebatterynotifier/service/BatteryHealthTrackerTest.java b/app/src/test/java/com/almothafar/simplebatterynotifier/service/BatteryHealthTrackerTest.java index 1e61a05..96362b1 100644 --- a/app/src/test/java/com/almothafar/simplebatterynotifier/service/BatteryHealthTrackerTest.java +++ b/app/src/test/java/com/almothafar/simplebatterynotifier/service/BatteryHealthTrackerTest.java @@ -20,6 +20,68 @@ @RunWith(Enclosed.class) public class BatteryHealthTrackerTest { + /** + * {@link BatteryHealthTracker#estimatedHealthForCycles(int)} — the cycle-based degradation curve + * (#161 extracted it as a pure core so one cycle read serves every figure on a screen). + */ + @RunWith(Parameterized.class) + public static class EstimatedHealthForCycles { + + @Parameter(0) public String label; + @Parameter(1) public int cycles; + @Parameter(2) public int expected; + + @Parameters(name = "{0}") + public static Collection data() { + return Arrays.asList(new Object[][]{ + {"new battery -> 100%", 0, 100}, + {"mid-excellent 150 -> 98%", 150, 98}, + {"excellent boundary 300 -> 95%", 300, 95}, + {"mid-good 400 -> 90%", 400, 90}, + {"good boundary 500 -> 85%", 500, 85}, + {"mid-fair 650 -> 78%", 650, 78}, + {"fair boundary 800 -> 70%", 800, 70}, + {"deep poor 1300 -> 40%", 1300, 40}, + {"floor holds at 40%", 2000, 40}, + }); + } + + @Test + public void matchesExpected() { + assertEquals(label, expected, BatteryHealthTracker.estimatedHealthForCycles(cycles)); + } + } + + /** + * {@link BatteryHealthTracker#gradeForCycles(int)} — the cycle-count bucket boundaries, which must + * stay consistent with the {@link BatteryHealthTracker#estimatedHealthForCycles(int)} curve. + */ + @RunWith(Parameterized.class) + public static class GradeForCycles { + + @Parameter(0) public int cycles; + @Parameter(1) public BatteryHealthGrade expected; + + @Parameters(name = "{0} cycles -> {1}") + public static Collection data() { + return Arrays.asList(new Object[][]{ + {0, BatteryHealthGrade.EXCELLENT}, + {299, BatteryHealthGrade.EXCELLENT}, + {300, BatteryHealthGrade.GOOD}, + {499, BatteryHealthGrade.GOOD}, + {500, BatteryHealthGrade.FAIR}, + {799, BatteryHealthGrade.FAIR}, + {800, BatteryHealthGrade.POOR}, + {2000, BatteryHealthGrade.POOR}, + }); + } + + @Test + public void matchesExpected() { + assertEquals(expected, BatteryHealthTracker.gradeForCycles(cycles)); + } + } + /** * {@link BatteryHealthTracker#computeMeasuredHealth(int, int)} — measured capacity as a percentage * of design capacity, clamped to 1..100, or -1 when the inputs are unusable. diff --git a/app/src/test/java/com/almothafar/simplebatterynotifier/service/SystemServiceTest.java b/app/src/test/java/com/almothafar/simplebatterynotifier/service/SystemServiceTest.java index e32398e..fbf8e2a 100644 --- a/app/src/test/java/com/almothafar/simplebatterynotifier/service/SystemServiceTest.java +++ b/app/src/test/java/com/almothafar/simplebatterynotifier/service/SystemServiceTest.java @@ -1,11 +1,20 @@ package com.almothafar.simplebatterynotifier.service; +import android.content.Context; +import android.content.Intent; +import android.os.BatteryManager; + +import androidx.test.core.app.ApplicationProvider; + +import org.junit.Before; import org.junit.Test; import org.junit.experimental.runners.Enclosed; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameter; import org.junit.runners.Parameterized.Parameters; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; import java.util.Arrays; import java.util.Collection; @@ -13,11 +22,56 @@ import static org.junit.Assert.assertEquals; /** - * Unit tests for the pure capacity-estimation helpers in {@link SystemService}. + * Unit tests for the pure capacity-estimation helpers in {@link SystemService}, and the snapshot's + * cycle-count extraction (#161). */ @RunWith(Enclosed.class) public class SystemServiceTest { + /** + * The OS cycle count rides on the {@code BatteryDO} snapshot (#161), extracted from the same + * {@code ACTION_BATTERY_CHANGED} intent as everything else — normalized to -1 when absent or + * non-positive, matching {@link SystemService#getChargeCycleCount}, so per-tick consumers never + * need a second sticky-broadcast read. + */ + @RunWith(RobolectricTestRunner.class) + @Config(sdk = 34) + public static class SnapshotCycleCount { + + private Context context; + + @Before + public void setUp() { + context = ApplicationProvider.getApplicationContext(); + } + + @Test + public void reportedCycleCount_carriesOntoTheSnapshot() { + assertEquals(287, SystemService.getBatteryInfo(context, batteryIntent(287)).getCycleCount()); + } + + @Test + public void absentCycleCount_readsAsMinusOne() { + final Intent battery = new Intent(Intent.ACTION_BATTERY_CHANGED); + battery.putExtra(BatteryManager.EXTRA_LEVEL, 50); + battery.putExtra(BatteryManager.EXTRA_SCALE, 100); + assertEquals(-1, SystemService.getBatteryInfo(context, battery).getCycleCount()); + } + + @Test + public void nonPositiveCycleCount_normalizedToMinusOne() { + assertEquals(-1, SystemService.getBatteryInfo(context, batteryIntent(0)).getCycleCount()); + } + + private static Intent batteryIntent(final int cycleCount) { + final Intent battery = new Intent(Intent.ACTION_BATTERY_CHANGED); + battery.putExtra(BatteryManager.EXTRA_LEVEL, 50); + battery.putExtra(BatteryManager.EXTRA_SCALE, 100); + battery.putExtra(BatteryManager.EXTRA_CYCLE_COUNT, cycleCount); + return battery; + } + } + /** * {@link SystemService#estimateFullCapacityMah(int, int)} — derives full capacity (mAh) from a * CHARGE_COUNTER reading (µAh) at a given charge level, rejecting implausible results as 0. diff --git a/app/src/test/java/com/almothafar/simplebatterynotifier/ui/fragment/BatteryDetailsRowSyncTest.java b/app/src/test/java/com/almothafar/simplebatterynotifier/ui/fragment/BatteryDetailsRowSyncTest.java new file mode 100644 index 0000000..7922472 --- /dev/null +++ b/app/src/test/java/com/almothafar/simplebatterynotifier/ui/fragment/BatteryDetailsRowSyncTest.java @@ -0,0 +1,132 @@ +package com.almothafar.simplebatterynotifier.ui.fragment; + +import android.content.Context; +import android.widget.TableLayout; +import android.widget.TableRow; +import android.widget.TextView; + +import androidx.test.core.app.ApplicationProvider; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; + +import java.util.LinkedHashMap; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; + +/** + * Robolectric tests for {@link BatteryDetailsFragment#syncRows} (issue #161): steady-state + * refreshes must update existing rows in place — never recreate them, which reset accessibility + * focus and scroll every 3 s — while rows whose key appears or vanishes (rate warm-up, + * time-to-full gating) are inserted or removed at the right position. + */ +@RunWith(RobolectricTestRunner.class) +@Config(sdk = 34) +public class BatteryDetailsRowSyncTest { + + private Context context; + private TableLayout table; + private Map rowViews; + private BatteryDetailsFragment.RowBinder binder; + + @Before + public void setUp() { + context = ApplicationProvider.getApplicationContext(); + table = new TableLayout(context); + rowViews = new LinkedHashMap<>(); + // Minimal binder: one TextView per row carrying the value, updated in place on rebind. + binder = new BatteryDetailsFragment.RowBinder() { + @Override + public TableRow createRow(final String label, final CharSequence value) { + final TableRow row = new TableRow(context); + final TextView valueView = new TextView(context); + valueView.setText(value); + row.addView(valueView); + return row; + } + + @Override + public void bindValue(final TableRow row, final String label, final CharSequence value) { + ((TextView) row.getChildAt(0)).setText(value); + } + }; + } + + @Test + public void steadyState_updatesValuesWithoutRecreatingRows() { + BatteryDetailsFragment.syncRows(table, rowViews, values("Voltage", "3900 mV", "Temperature", "25 °C"), binder); + final TableRow voltageRow = rowViews.get("Voltage"); + final TableRow temperatureRow = rowViews.get("Temperature"); + + BatteryDetailsFragment.syncRows(table, rowViews, values("Voltage", "3850 mV", "Temperature", "26 °C"), binder); + + // Same view instances (a11y focus survives), fresh values, no row growth. + assertSame(voltageRow, table.getChildAt(0)); + assertSame(temperatureRow, table.getChildAt(1)); + assertEquals(2, table.getChildCount()); + assertEquals("3850 mV", valueAt(0)); + assertEquals("26 °C", valueAt(1)); + } + + @Test + public void vanishedKey_removesOnlyItsRow() { + BatteryDetailsFragment.syncRows(table, rowViews, values("Rate", "9%/h", "Voltage", "3900 mV", "Temperature", "25 °C"), binder); + final TableRow voltageRow = rowViews.get("Voltage"); + final TableRow temperatureRow = rowViews.get("Temperature"); + + // The rate row hides (e.g. static level); its neighbours must survive untouched. + BatteryDetailsFragment.syncRows(table, rowViews, values("Voltage", "3900 mV", "Temperature", "25 °C"), binder); + + assertEquals(2, table.getChildCount()); + assertSame(voltageRow, table.getChildAt(0)); + assertSame(temperatureRow, table.getChildAt(1)); + } + + @Test + public void appearingKey_isInsertedAtItsPosition() { + BatteryDetailsFragment.syncRows(table, rowViews, values("Voltage", "3900 mV", "Temperature", "25 °C"), binder); + final TableRow voltageRow = rowViews.get("Voltage"); + final TableRow temperatureRow = rowViews.get("Temperature"); + + // The rate row appears at the TOP after warm-up; existing rows shift down but stay the same views. + BatteryDetailsFragment.syncRows(table, rowViews, values("Rate", "9%/h", "Voltage", "3900 mV", "Temperature", "25 °C"), binder); + + assertEquals(3, table.getChildCount()); + assertSame(rowViews.get("Rate"), table.getChildAt(0)); + assertSame(voltageRow, table.getChildAt(1)); + assertSame(temperatureRow, table.getChildAt(2)); + assertEquals("9%/h", valueAt(0)); + } + + @Test + public void midTableInsertion_landsBetweenItsNeighbours() { + BatteryDetailsFragment.syncRows(table, rowViews, values("Rate", "9%/h", "Temperature", "25 °C"), binder); + + // Time-to-full gates open mid-table (#124): it must land between rate and temperature. + BatteryDetailsFragment.syncRows(table, rowViews, values("Rate", "9%/h", "Time to full", "~1h 20m", "Temperature", "25 °C"), binder); + + assertEquals(3, table.getChildCount()); + assertSame(rowViews.get("Time to full"), table.getChildAt(1)); + assertEquals("~1h 20m", valueAt(1)); + } + + // --- helpers ------------------------------------------------------------- + + private static Map values(final CharSequence... labelValuePairs) { + final Map map = new LinkedHashMap<>(); + for (int i = 0; i < labelValuePairs.length; i += 2) { + map.put(labelValuePairs[i].toString(), labelValuePairs[i + 1]); + } + return map; + } + + private String valueAt(final int rowIndex) { + final TableRow row = (TableRow) table.getChildAt(rowIndex); + return ((TextView) row.getChildAt(0)).getText().toString(); + } +}