DRY: typed AppPrefs facade — one owner for the critical/warning key + default (#162)#198
Merged
Conversation
) Introduce AppPrefs as the single owner of the critical/warning battery-level key + default. The 20/40 literals previously restated in NotificationService, BatteryLevelReceiver, MainActivity and BatteryRangeSliderHelper now live only in AppPrefs.DEFAULT_CRITICAL_LEVEL / DEFAULT_WARNING_LEVEL, so the alert engine, receiver and both sliders can no longer disagree. - Migrate the read sites to AppPrefs.criticalLevel/warningLevel(context). - Route the two slider write paths' shared pair through AppPrefs.setBatteryLevels. - Remove BatteryRangeSliderHelper.DEFAULT_CRITICAL/DEFAULT_WARNING; the slider preference now references the facade constants, and pref_alerts.xml carries a comment tying its XML-declared defaults to them. - Drop MainActivity's now-unused SharedPreferences/PreferenceManager imports. - Add Robolectric coverage for the facade's defaults, read-back and write. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lt (#162) Code review flagged the class Javadoc's "the literals now live only ... here" as overstating it: pref_alerts.xml still declares the slider's XML default (20/40), which the framework cannot share as a constant. Reword to say every Java read derives its default from the constants, and name the XML restatement as the one remaining (already comment-tied) exception, so the doc matches reality. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ade, XML drift guard (#162) Three items the two-axis review raised as deferrable, now done: 1. Data Clumps — introduce model.LevelThresholds (record) so the critical/warning pair travels as one value instead of a loose (int, int) that could be swapped. AppPrefs.batteryLevels()/setBatteryLevels() and BatteryRangeSliderHelper.clampPair() now take/return it; the sliders and MainActivity unwrap only at the widget edge (HorseshoeProgressBar stays int-based to remain a reusable, app-agnostic widget). 2. Fold the drain limit into the facade — move DEFAULT/MIN/MAX_DRAIN_LIMIT_PPH and the clamp from BatteryRateTracker into AppPrefs as drainLimitPph()/clampDrainLimit(), the "single owner of key + default + clamp" the issue asks for. This also avoids a util->service dependency. Repoint FastDrainDetector and BatteryDetailsFragment; the pure clamp stays unit-tested (test moved alongside it). 3. Drift guard — AppPrefsTest now parses pref_alerts.xml and asserts the slider's XML-declared criticalDefault/warningDefault equal the AppPrefs constants, so the one framework-forced restatement can no longer silently diverge. Full suite: 445 tests, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #162.
Problem
The critical/warning battery-level defaults (
20/40) were restated as inline literals at every read site —NotificationService,BatteryLevelReceiver,MainActivity— while named constants lived in a fourth place (BatteryRangeSliderHelper). If the default ever changed, the alert engine and the UI could silently disagree.Fix
Introduce
AppPrefs, a small typed facade inutil/that is the single owner of the key + default for the critical/warning levels:AppPrefs.criticalLevel(ctx)/warningLevel(ctx)— key + default, replacing the inlinegetInt(getString(...), 20|40)reads.AppPrefs.setBatteryLevels(ctx, critical, warning)— the two slider write paths shared this pair; the write now lives beside the reads.AppPrefs.DEFAULT_CRITICAL_LEVEL/DEFAULT_WARNING_LEVEL— the only place20/40live now.Migrated sites:
BatteryLevelReceiver,NotificationService,MainActivity(3 read/write sites). RemovedBatteryRangeSliderHelper.DEFAULT_CRITICAL/DEFAULT_WARNING; the slider preference now references the facade constants, andpref_alerts.xmlcarries a comment tying its XML-declared defaults to them (the framework instantiates from XML, so the value can't literally share a constant there). Also dropped MainActivity's now-unusedSharedPreferences/PreferenceManagerimports.Per the issue's "migrate incrementally — a full sweep can trail", this PR covers the duplicated-default offenders only. The already-single-owner clamps (
clampDrainLimit,clampMinutesToMs) and other settings are left for later increments; they stay untouched and unit-tested.Acceptance criteria
BatteryRangeSliderHelper.DEFAULT_*moved to the facade (slider preference delegates to it)Tests
AppPrefsTest(Robolectric): defaults fall back to the single owner, stored values read back,setBatteryLevelsround-trips under the shared keys.:app:testDebugUnitTestsuite green.Not covered here (deferrable per #162)
LevelThresholdsvalue type (Data Clumps) — better as a follow-up.drainLimitPph/clamps and the remaining settings into the facade — later increments.🤖 Generated with Claude Code