Conversation
…nt cannot be resolved from Push.
| Result.success( | ||
| requests.associateWith { Result.failure(ResolvingException("Unable to resolve event")) } | ||
| ) | ||
| }, |
There was a problem hiding this comment.
The existing test was not really testing the notification resolver failure (the type is a Result of a Map of Result, so it's a bit confusing), but this change fixes it.
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4845 +/- ##
========================================
Coverage 80.29% 80.30%
========================================
Files 2140 2148 +8
Lines 56902 56986 +84
Branches 7158 7172 +14
========================================
+ Hits 45688 45761 +73
- Misses 8781 8787 +6
- Partials 2433 2438 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
...h/impl/src/main/kotlin/io/element/android/libraries/push/impl/battery/BatteryOptimization.kt
Show resolved
Hide resolved
...sh/impl/src/main/kotlin/io/element/android/libraries/push/impl/store/DefaultPushDataStore.kt
Show resolved
Hide resolved
...c/main/kotlin/io/element/android/libraries/push/impl/push/MutableBatteryOptimizationStore.kt
Show resolved
Hide resolved
...c/main/kotlin/io/element/android/libraries/push/impl/battery/BatteryOptimizationPresenter.kt
Outdated
Show resolved
Hide resolved
...pi/src/main/kotlin/io/element/android/libraries/push/api/battery/BatteryOptimizationState.kt
Outdated
Show resolved
Hide resolved
|
A couple of suggestions on the wording:
Notifications not arriving?
Disable battery optimization for this app, to make sure all notifications are received.
Disable Optimization Am I right that we do not directly check if the battery optimization is enabled, we derive this from the fact that there was an unresolved event? So in theory it could be that the battery optimization actually is already disabled and there was another reason why the event was unresolved? |
|
Hello, @mxandreas , thanks for the feedback.
No, Element X also checks if the battery optimisation is already disabled. If battery optimization is already disabled, we do not show the banner. (code) |
jmartinesp
left a comment
There was a problem hiding this comment.
LGTM, thanks. Just a question about naming.
|
|
||
| sealed interface BatteryOptimizationEvents { | ||
| data object Dismiss : BatteryOptimizationEvents | ||
| data object DoAction : BatteryOptimizationEvents |
There was a problem hiding this comment.
DoAction sounds way too generic IMO 😅 . Why not just DisableOptimizations? Even if we end up opening the app settings as a fallback for this I think it's fine.
There was a problem hiding this comment.
Ah yes, renamed to RequestDisableOptimizations. I wanted to use a common Event interface for the other banner, but I changed my mind and forgot to rename the data object.
|



Content
When an Event cannot be resolved from Push, let the application render a banner in the room list to propose the user to disable battery optimization.
When battery optimization is disabled, it should help to resolve more events when a Push is received. The cost is a higher battery consumption. User always has the ability to revert the change by navigating to the system settings.
Draft as we're waiting for product decision and the wording of the banner may be updated (and must be added to Localazy).
Motivation and context
Improve Notification resolution and limit the risk of not shown notification because not resolve. In a separate PR we will ensure that a notification is always displayed when a not resolved Push is happening.
Closes #4611
Screenshots / GIFs
BatteryOptimization_0.mp4
BatteryOptimization_1.mp4
Tests
Tested devices
Checklist