Splitting notifications into separate creation and displaying passes #4185
Conversation
| private val autoAcceptInvites: AutoAcceptInvites | ||
| ) { | ||
|
|
||
| fun modifyAndProcess(eventList: MutableList<NotifiableEvent>, currentRoomId: String?): ProcessedNotificationEvents { |
There was a problem hiding this comment.
this loop extracted as class
https://github.com/vector-im/element-android/blob/b71e2de9acf611d4a50703aed458d1e022d2b105/vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt#L248
it's processing the events by filtering out of date/already read or auto accepted invites
| ) { | ||
|
|
||
| fun createRoomMessage(events: List<NotifiableMessageEvent>, roomId: String, userDisplayName: String, userAvatarUrl: String?): RoomNotification.Message { | ||
| val firstKnownRoomEvent = events[0] |
There was a problem hiding this comment.
extracted the notification creation from
https://github.com/vector-im/element-android/blob/b71e2de9acf611d4a50703aed458d1e022d2b105/vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt#L299
this class only handles creating room message notifications
| invitationNotifications: List<OneShotNotification.Append.Meta>, | ||
| simpleNotifications: List<OneShotNotification.Append.Meta>, | ||
| useCompleteNotificationFormat: Boolean): Notification { | ||
| val summaryInboxStyle = NotificationCompat.InboxStyle().also { style -> |
There was a problem hiding this comment.
this the all the summary group logic collected into a single function/class https://github.com/vector-im/element-android/blob/b71e2de9acf611d4a50703aed458d1e022d2b105/vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt#L465
rather than spread across all other types of notification creation
| eventList: MutableList<NotifiableEvent>) { | ||
| Timber.v("Render notification events - count: ${eventList.size}") | ||
| val notificationEvents = notifiableEventProcessor.modifyAndProcess(eventList, currentRoomId) | ||
| if (lastKnownEventList == notificationEvents.hashCode()) { |
There was a problem hiding this comment.
with the notifiable events becoming immutable models #4182 we can check for changes and skip redundant render passes where the notifications haven't changed or aren't relevant, eg a room marking a message as read
| private fun processEvents(notificationEvents: ProcessedNotificationEvents, myUserId: String, myUserDisplayName: String, myUserAvatarUrl: String?, useCompleteNotificationFormat: Boolean) { | ||
| val (roomEvents, simpleEvents, invitationEvents) = notificationEvents | ||
| with(notificationFactory) { | ||
| val roomNotifications = roomEvents.toNotifications(myUserDisplayName, myUserAvatarUrl) |
There was a problem hiding this comment.
processEvents is where the all the notifications are created and then afterwards all notifications are rendered
ouchadam
force-pushed
the
feature/adm/notifications-creation-display-split
branch
from
91f35f2 to
b715e9f
Compare
| val invitationEvents: MutableMap<String, InviteNotifiableEvent?> = LinkedHashMap() | ||
|
|
||
| val eventIterator = eventList.listIterator() | ||
| while (eventIterator.hasNext()) { |
There was a problem hiding this comment.
we can further improve this by only calculating the diff between what we've rendered and the new eventList state
ouchadam
force-pushed
the
feature/adm/immutable-notifiable-models
branch
from
55681b1 to
d5e73cc
Compare
…e handled - also puts in the basis for a separate notification refreshing implementation
- updates the logic to track when events are removed as a way for the notifications to remove themselves, null events mean they've been removed
- also handles when the event diff means the notifications should be removed
…nto a NotificationRender - extract the displaying into its own class to avoid leaking the entire notificationutils - cancel/display notification actions are completely driven by the event or abscense of event from the eventList - attempts to avoid redundant render passes by checking if the eventList has changed since the last render
- the renderer's responsibility it handling events
…r source of truth - when events have finished being displayed they should be removed from the eventList via notification delete actions
…o empty due to only containing removals
…l notifications - adds some comments to explain the positioning
ouchadam
force-pushed
the
feature/adm/notifications-creation-display-split
branch
from
b715e9f to
1c35109
Compare
|
closing to avoid ordering confusion |
Well this ballooned in size...
This entire change is the result of extracting, breaking down and unit testing 1 function NotificationDrawerManager.refreshNotificationDrawerBg
There are 3 main objectives/changes to the notification design
system UI message queue, which in turn should address our double notification problem 🤞 Doubled notifications #4152
eventListis the sole source of truth, we now simply render it as android notifications, when a notification is dismissed it triggers a delete action which causes the event to be reflected in theeventList, which in turn renders the newly missing event as a notification removal.cancel -> showflows, we only show or cancel event changesRoomGroupMessageCreator&SummaryGroupMessageCreatorare not particularly friendly to unit tests due to calling into android builders (although we could abstract that and test...)