Refactor: split NotificationService (channels / quiet-hours / sound / dispatch) + dedupe (#166)#199
Merged
Conversation
…ispatch + dedupe (#166) NotificationService had grown to ~1,480 lines doing four unrelated jobs (SRP): channel registry, quiet-hours/silent-mode policy, sound/vibration playback, and notification building/dispatch, plus dead static-lifecycle plumbing. All the prerequisite fixes this refactor was sequenced behind (#153/#154/#155/#160/#165) are merged, so this is a pure move — no behaviour change. Split into focused, package-private collaborators in the service package: - NotificationChannels — channel registry, versioned-ID scheme (#153), create/update/refresh, channelFor. - QuietHours — the (mostly pure) quiet-hours/silent-mode policy: isWithinNotificationWindow / isWithinTimeRange / boundOrDefaultMinutes / alertsAllowedNow / shouldIgnoreSilentMode. - AlertSounds — playAlarm + its soundExecutor + the DND check. - OngoingStatusContent — the ongoing notification's title/detail/expanded text and small-icon selection (#192/#194). - AlertSpec (now a shared top-level record) + NotificationConfig moved to their own files; NotificationService stays the public dispatch facade. DRY items folded in: 1. One builder chain — the setTicker/…/BigTextStyle sequence written three times now lives in one alertBuilder(context, channelId, AlertSpec); the level, temperature, fast-drain, slow-charge and charge-connected alerts all flow through it. 2. One charge-style resolver — deliverPerChargeStyle(context, content, notificationDelivery) is shared by notifyChargeConnected and sendSlowChargeWarning (read pref → resolve → none/toast/notification). 3. One vibration pattern — VIBRATION_PATTERN now lives in SystemService and is referenced by channel creation, so channel vibration and the manual silent-mode buzz can't drift. 4. Dead lifecycle API removed — shutdown()/releaseCachedBitmap() had no callers and there is no Application class to wire them to; the executor and the WeakReference'd icon are reclaimed on process death. Also consolidated the "Vibrate" preference read (previously an inline true default in three spots) into AppPrefs.vibrateEnabled (the #162 facade). Tests split to mirror the new classes: QuietHoursTest, NotificationChannelsTest, OngoingStatusContentTest; NotificationServiceTest trimmed to the charge-connected wiring + charge-style normalization; a vibrateEnabled case added to AppPrefsTest. 446 tests + lintDebug + assembleDebug green; installed on the Mate 10 Pro. No user-facing strings. Closes #166 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The DND check swallowed Settings.SettingNotFoundException silently (a pre-existing catch moved verbatim from NotificationService), violating the no-silent-exception standard. zen_mode has existed since API 21 and minSdk is 26, so its absence is genuinely unexpected — not an expected-validation case — so the narrow catch now logs (Log.w with the throwable) while keeping the safe "not in DND" fallback that leaves the alert on its normal audible path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…licated prologue (#166) Follow-up to the two-axis review of PR #199. Three Standards findings: 1. Hard: AppPrefs.vibrateEnabled had `final` on its Context param, against the new-code convention (no final on params; final on locals). Dropped it. (AppPrefs' legacy methods keep final — a full sweep is a separate change.) 2. Feature Envy / Data Clumps: sendNotification reached into six of NotificationConfig's display fields to re-pack them into an AlertSpec. NotificationConfig now emits its own AlertSpec via toAlertSpec(id), so the dispatcher no longer reaches in. 3. Duplicated Code: postChargeNotification repeated sendQuietHoursAwareAlert's prologue verbatim (permission guard -> ensureChannels -> prefs -> window -> channelFor). Extracted routeAlert(context, spec) -> AlertRouting(channelId, withinWindow); each caller now differs only in its setOnlyAlertOnce and whether it sounds. No boolean flag args introduced. Minor, non-observable deltas from folding charge-connected through routeAlert: its permission-denied log line is now the specific "charge-connected alert not sent" (was "notification not sent"), and its title/ticker strings are built before the permission check (pure getString). No functional change. 446 tests + lintDebug + assembleDebug green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 tasks
almothafar
added a commit
that referenced
this pull request
Jul 21, 2026
The guideline docs said "use final for all method parameters" while the actual convention is final on locals, NOT on parameters (new/edited code; legacy migrates incrementally). Surfaced by the #166 / PR #199 review, where AppPrefs.vibrateEnabled's final param was a hard violation the docs endorsed. - .claude/guidelines.md + CODE_REVIEW_GUIDELINES.md: state the convention (final on locals, not parameters; legacy migrates as touched), add a bad/good example, drop `final` from every parameter in the doc examples, and update the review checklist item. - AppPrefs: drop `final` from all remaining legacy method parameters so the file is internally consistent (it went mixed in #199). `final` on locals unchanged; behaviour unchanged (final on a param is a no-op modifier). testDebugUnitTest + lintDebug + assembleDebug green. Closes #200 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Merged
3 tasks
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
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 #166.
A pure move — no behaviour change. All the fixes this refactor was sequenced behind (#153/#154/#155/#160/#165) are merged, so
NotificationService(which had grown to ~1,480 lines doing four unrelated jobs) is now reshaped without changing what it does.The split
NotificationServicestays the public dispatch façade (unchanged public API → no caller changes) and delegates to focused, package-private collaborators inservice:NotificationChannelschannelForQuietHoursisWithinNotificationWindow/isWithinTimeRange/boundOrDefaultMinutes/alertsAllowedNow/shouldIgnoreSilentModeAlertSoundsplayAlarm+ itssoundExecutor+ the DND checkOngoingStatusContentAlertSpec(now top-level) +NotificationConfigRough line counts after:
NotificationService633 (≈300 code lines; the rest is public-API javadoc),OngoingStatusContent285,NotificationChannels222,QuietHours140,NotificationConfig107,AlertSounds82,AlertSpec21.DRY items folded in
setTicker/…/BigTextStylesequence written three times is now onealertBuilder(context, channelId, AlertSpec). The level, temperature, fast-drain, slow-charge and charge-connected alerts all flow through it (the issue's "everything becomes anAlertSpec").deliverPerChargeStyle(context, content, notificationDelivery)is shared bynotifyChargeConnectedandsendSlowChargeWarning(read pref → resolve → none/toast/notification).VIBRATION_PATTERNnow lives inSystemService(which does the actual vibrating), referenced by channel creation, so channel vibration and the manual silent-mode buzz can't drift.shutdown()/releaseCachedBitmap()had no callers and there's noApplicationclass to wire them to; the executor and theWeakReference'd icon are reclaimed on process death. (Decision recorded per the issue: delete.)Also consolidated the "Vibrate" preference read (previously an inline
truedefault in three spots) into a singleAppPrefs.vibrateEnabled— the #162 facade pattern, one typed getter.Tests
Split to mirror the new classes:
QuietHoursTest,NotificationChannelsTest,OngoingStatusContentTest;NotificationServiceTesttrimmed to the charge-connected wiring + charge-style normalization; avibrateEnabledcase added toAppPrefsTest. Scenarios/assertions are unchanged — only the class under test is repointed.Verification
:app:testDebugUnitTest(446 tests, 0 failures) +:app:lintDebug(MissingTranslation= error, so a clean pass confirms string parity) +:app:assembleDebug— all green.values-ar.Acceptance criteria
shutdown/releaseCachedBitmapremovedNotificationServiceis 633 total but ~300 code lines; the remainder is thorough public-API javadoc, matching repo style)🤖 Generated with Claude Code