Skip to content

Refactor: split NotificationService (channels / quiet-hours / sound / dispatch) + dedupe (#166)#199

Merged
almothafar merged 3 commits into
masterfrom
refactor/166-split-notification-service
Jul 21, 2026
Merged

Refactor: split NotificationService (channels / quiet-hours / sound / dispatch) + dedupe (#166)#199
almothafar merged 3 commits into
masterfrom
refactor/166-split-notification-service

Conversation

@almothafar

Copy link
Copy Markdown
Owner

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

NotificationService stays the public dispatch façade (unchanged public API → no caller changes) and delegates to focused, package-private collaborators in service:

Class Job
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 + small-icon choice (#192/#194)
AlertSpec (now top-level) + NotificationConfig moved to their own files

Rough line counts after: NotificationService 633 (≈300 code lines; the rest is public-API javadoc), OngoingStatusContent 285, NotificationChannels 222, QuietHours 140, NotificationConfig 107, AlertSounds 82, AlertSpec 21.

DRY items folded in

  1. One builder chain — the setTicker/…/BigTextStyle sequence written three times is now one alertBuilder(context, channelId, AlertSpec). The level, temperature, fast-drain, slow-charge and charge-connected alerts all flow through it (the issue's "everything becomes an AlertSpec").
  2. One charge-style resolverdeliverPerChargeStyle(context, content, notificationDelivery) is shared by notifyChargeConnected and sendSlowChargeWarning (read pref → resolve → none/toast/notification).
  3. One vibration patternVIBRATION_PATTERN now lives in SystemService (which does the actual vibrating), referenced by channel creation, so channel vibration and the manual silent-mode buzz can't drift.
  4. Dead lifecycle API removedshutdown() / releaseCachedBitmap() had no callers and there's no Application class to wire them to; the executor and the WeakReference'd icon are reclaimed on process death. (Decision recorded per the issue: delete.)

Also consolidated the "Vibrate" preference read (previously an inline true default in three spots) into a single AppPrefs.vibrateEnabled — the #162 facade pattern, one typed getter.

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. 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.
  • Installed on the Mate 10 Pro (BLA-L29).
  • No user-facing strings → nothing for values-ar.

Acceptance criteria

  • No behaviour change (full suite green; on-device smoke pending — maintainer's step)
  • One builder chain, one charge-style resolver, one vibration pattern
  • shutdown/releaseCachedBitmap removed
  • No resulting class above ~400 lines (soft target: NotificationService is 633 total but ~300 code lines; the remainder is thorough public-API javadoc, matching repo style)

🤖 Generated with Claude Code

almothafar and others added 3 commits July 19, 2026 17:59
…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>
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>
@almothafar
almothafar merged commit fcd1c2e into master Jul 21, 2026
2 checks passed
@almothafar
almothafar deleted the refactor/166-split-notification-service branch July 21, 2026 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor: split NotificationService (channels / quiet-hours policy / dispatch / sound) and dedupe its repeated blocks

1 participant