Fix notification sensor settings initialization#7206
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60857a98-004b-4e8b-b344-bea34c70e8e8
There was a problem hiding this comment.
Pull request overview
This pull request fixes initialization of the notification sensor allow-list settings so they are persisted during normal sensor refresh (not only after notification posted/removed callbacks), ensuring settings are immediately configurable after enabling the sensor.
Changes:
- Initialize allow-list settings for “last notification” and “last removed notification” during
requestSensorUpdate()when those sensors are enabled. - Refactor duplicated allow-list retrieval logic into a shared
getNotificationSettings(...)helper. - Add Robolectric regression tests covering both sensors’ initialization and preservation of an existing custom allow-list value; add a changelog entry.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/kotlin/io/homeassistant/companion/android/sensors/NotificationListenerSensorManager.kt | Initializes notification allow-list settings during refresh and centralizes settings retrieval logic. |
| app/src/test/kotlin/io/homeassistant/companion/android/sensors/NotificationListenerSensorManagerTest.kt | Adds regression coverage for settings initialization and preservation of existing allow-list values. |
| app/src/main/res/xml/changelog_master.xml | Documents the user-visible fix in the main changelog. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60857a98-004b-4e8b-b344-bea34c70e8e8
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60857a98-004b-4e8b-b344-bea34c70e8e8
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60857a98-004b-4e8b-b344-bea34c70e8e8
jpelgrom
left a comment
There was a problem hiding this comment.
Thanks for changing this, nice improvement to the user experience!
| } | ||
|
|
||
| private suspend fun getNotificationSettings(sensor: SensorManager.BasicSensor): NotificationSettings { | ||
| val allowPackages = getSetting( |
There was a problem hiding this comment.
I really don't like the pattern this fix has to lean on: a get that initializes and persists the value as a side effect. This PR even makes it explicit, requestSensorUpdate now calls a getter and discards the result purely so the write happens. To be clear, that side effect is not introduced here,
getSetting has always worked like this, and given the current architecture this is the correct fix for the bug.
I recently removed exactly this behavior for the sensors themselves in #7040: the repository returns the default when nothing is stored, and only an actual change persists to the DB. I didn't look at the settings back then, but they should follow the same
model. The reason it can't be a drop-in change is that the detail screen currently discovers which settings exist from the stored rows (getSettingsFlow), so the write-on-read is what makes settings show up at all. Moving to the #7040 model means sensor managers declare their available settings and the UI
merges that declaration with the stored values, at which point this initialization path, and the need for this fix, disappears entirely.
That refactor is clearly out of scope here, so nothing to change in this PR. See #7225
| val settings = getNotificationSettings(lastNotification) | ||
|
|
||
| if (sbn.packageName == applicationContext.packageName || | ||
| (allowPackages.isNotEmpty() && sbn.packageName !in allowPackages) || | ||
| (!disableAllowListRequirement && allowPackages.isEmpty()) | ||
| (settings.allowPackages.isNotEmpty() && sbn.packageName !in settings.allowPackages) || | ||
| (!settings.disableAllowListRequirement && settings.allowPackages.isEmpty()) |
There was a problem hiding this comment.
This logic is duplicated line 204-208 and should be merged into one method.
TimoPtr
left a comment
There was a problem hiding this comment.
Let's just avoid the duplication and we can merge it in this current state.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Summary
Notification allow-list settings were only persisted after a notification callback. As a result, a newly enabled notification sensor could show no configurable settings until a notification was posted or removed.
Initialize the settings during the normal sensor refresh for enabled posted and removed notification sensors. The callback paths use the same settings helper, preserving existing values and filtering behavior. This also adds regression coverage for both sensors and custom allow-list preservation.
Fixes #6773
Checklist
Screenshots
Not applicable; this fixes when existing settings become available and does not change their appearance.
Link to pull request in documentation repositories
No documentation changes are required.
Any other notes
None.