Notifications redesign #4274
Notifications redesign #4274
Conversation
ouchadam
force-pushed
the
feature/adm/notification-redesign
branch
from
d8919e8 to
7eaf0d8
Compare
|
Doing some tests.
|
|
ahhh good catch, this is because the redaction logic is only removing the message when all the messages are redacted here's a failing test for the @Test
fun `given a room with redacted and non redacted message events when mapping to notification then redacted events are removed`() = testWith(notificationFactory) {
val roomWithRedactedMessage = mapOf(A_ROOM_ID to listOf(
ProcessedEvent(Type.KEEP, A_MESSAGE_EVENT.copy(isRedacted = true)),
ProcessedEvent(Type.KEEP, A_MESSAGE_EVENT.copy(eventId = "not-redacted"))
))
val withRedactedRemoved = listOf(A_MESSAGE_EVENT.copy(eventId = "not-redacted"))
val expectedNotification = roomGroupMessageCreator.givenCreatesRoomMessageFor(withRedactedRemoved, A_ROOM_ID, MY_USER_ID, MY_AVATAR_URL)
val result = roomWithRedactedMessage.toNotifications(MY_USER_ID, MY_AVATAR_URL)
result shouldBeEqualTo listOf(expectedNotification)
}the fix is to add a redacted filter to the messages val messageEvents = events.onlyKeptEvents().filterNot { it.isRedacted } |
|
Another remark, I think |
Thanks 😊
TDD FTW! |
that's a very good point that I completely missed it's not ideal but at least it will only happen once for this upgrade for the fix above I'm waiting for some test devices to charge so I can take a screenshot/gif of the redaction in action |
|
|
||
| private typealias ProcessedMessageEvents = List<ProcessedEvent<NotifiableMessageEvent>> | ||
|
|
||
| class NotificationFactory @Inject constructor( |
There was a problem hiding this comment.
The name is a bit confusing, this factory does not create Notifications (class from the Android SDK) but rather Notifications bound to actions (as far as I understand).
However, I do not know what would be a better name :/
There was a problem hiding this comment.
it's creating delegates/wrappers of android notification + element metadata, could be a ElementNotification + ElementNotificationFactory but it doesn't add much, totally open to ideas
| when (summaryNotification) { | ||
| SummaryNotification.Removed -> { | ||
| Timber.d("Removing summary notification") | ||
| notificationDisplayer.cancelNotificationMessage(null, SUMMARY_NOTIFICATION_ID) |
There was a problem hiding this comment.
At first sight, I was wondering if it was worth continuing with the rest of the fun (so add a return statement), but reading more carefully, I think the answer is yes, we have to remove the other notifications
There was a problem hiding this comment.
yep exactly ^^^ we also need to update the summary is a specific order to avoiding flickering
| } | ||
| } | ||
|
|
||
| fun <T> List<ProcessedEvent<T>>.onlyKeptEvents() = filter { it.type == ProcessedEvent.Type.KEEP }.map { it.event } |
There was a problem hiding this comment.
Nit: to avoid multiple iterations on list, reduce to
fun <T> List<ProcessedEvent<T>>.onlyKeptEvents() = mapNotNull { processedEvent ->
processedEvent.event.takeIf { processedEvent.type == ProcessedEvent.Type.KEEP }
}(not tested)
There was a problem hiding this comment.
good point 0d81fc9
is working as expected
| } | ||
| } | ||
|
|
||
| sealed interface RoomNotification { |
There was a problem hiding this comment.
kudos for the usage of sealed interface
|
(I've pushed a small commit about formatting. I think I will not push other commits on this branch) |
|
I have seen that too, not sure where it comes from, and this is maybe not new...
|
|
redacted events fixed by #4286 |
ouchadam
force-pushed
the
feature/adm/notification-redesign
branch
from
de62a33 to
5b43e5e
Compare
|
🎉 I did rebase this morning but will do another one and a quick test before merging |
- we only want to notify users when they receive an invititation, not when they've accepted it
…an avoid eager notification cancels and rely on the main notification refresh flow
…chain to avoid another list creation
- also makes the notifiable events sealed interfaces so that we can copy the data classes with new redacted values when it changes
…ed or made immutable by the notification redesign
…can be replaced - makes the property immutable as only the creation of the event knows if it can be replace eg it came from a push or the /sync event stream
…l notifications - adds some comments to explain the positioning
- this allows us to only synchronise of the event list modifications rather than the entire notification creation/rendering which should in turn reduce some of our ANRs #4214
…ed event id - this state will be used to diff the currently rendered events against the new ones
… allow us to dismiss notifications from removed events
…cting the processed type when creating the notifications
…as our notification source of truth
…ngleton annotation
…assing around the process notificatiable events - also includes @JvmName on all conflicting extensions for consistency
…ing the notifications
ouchadam
force-pushed
the
feature/adm/notification-redesign
branch
from
5b43e5e to
124061e
Compare
|
CI happy, local testing is also still working as expected, going to merge! |
Notification redesign feature PR
Addresses an assortment of issues
The notifications logic has been redesigned in order to split the creation of the notifications and the displaying/rendering of them.
This aims to eliminate any race conditions between updating the child and group summary notifications which can cause duplicated notifications, sounds, vibration.
Another benefit of the redesign is the simplification of the flow allowing us to diff new events and rendered events which means we're able to stay better in sync with which notifications to display
Included reviewed PRs