Skip to content

Conversation

@ouchadam
Copy link
Contributor

  • Fixes notification related ANR [ANR] NotificationDrawerManager.setCurrentRoom  #4214

    • Splits the event list processing from the notification rendering, this allows the synchronised block to lock for less time (only whilst we process the events)
  • Fixes the serialised event list becoming out of sync with the currently rendered notifications, eg receiving multiple room invites

    • Adds event list and currently rendered events diffing, this allows us to remove out of sync notifications
BEFORE AFTER
before-accept-multiple after-multiple-joins

@ouchadam ouchadam force-pushed the feature/adm/clear-message-via-event-list branch from ea97e6b to 401d0cc Compare October 11, 2021 16:08
@github-actions
Copy link

github-actions bot commented Oct 11, 2021

Unit Test Results

  54 files  ±0    54 suites  ±0   44s ⏱️ -6s
107 tests +1  107 ✔️ +1  0 💤 ±0  0 ±0 
316 runs  +4  316 ✔️ +4  0 💤 ±0  0 ±0 

Results for commit ea8629e. ± Comparison against base commit 1c35109.

♻️ This comment has been updated with latest results.

@ouchadam ouchadam force-pushed the feature/adm/notifications-creation-display-split branch from b715e9f to 1c35109 Compare October 11, 2021 16:59
- 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
@ouchadam ouchadam force-pushed the feature/adm/clear-message-via-event-list branch from 401d0cc to ea8629e Compare October 11, 2021 17:01
@bmarty
Copy link
Member

bmarty commented Oct 12, 2021

There is a lot PR to be reviewed, I'm wondering if it's worth creating sub PR like that. Also I'm afraid I will have to review the same code when reviewing the PR which want to be merged on develop.
Maybe a big single PR with all the changes regarding the notification management would be better, to be revied and tested only once, but I'm not sure.
WDYT?

@ouchadam
Copy link
Contributor Author

ouchadam commented Oct 12, 2021

@bmarty that was my plan 😅 I can close the pending PRs if it's makes the review chain simpler

*all PRs merge into feature/adm/notification-redesign

In order
#4207 duplicate invitation notifications (already merged)
#4182 immutable event data
#4185 splitting creation and displaying logic
#4220 diffing the serialised events vs rendered

finally the feature PR for feature/adm/notification-redesign, which would be for testing or a code review if people want to review all the changes in a single PR instead of broken down

@ouchadam
Copy link
Contributor Author

closing to avoid ordering confusion

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.

3 participants