Immutable NotifiableEvent models #4182

ouchadam · 2021-10-07T11:28:55Z

A little bit of clean up to attempt to reduce the notification redesign PR size...

Updates the NotifiableEvent models to favour immutable state, this allows us to the simplify the mutation flows

Gives the NotifiableEventResolver full control over the creation of the events (when I get time I'll revisit and add tests)
isPushGatewayEvent -> canBeReplaced
isNoisy is calculated at the point of creation rather than appended later on
Ensures events can't mutate whilst we're iterating over them*
Removes unused handleNotificationWithoutSyncingMode and lockScreenVisibility

Notice how the summary sneaks through in the reading gif, this will also be fixed by the redesign.

RECEIVING	READING

*this doesn't include updating hasBeenDisplayed as this will be removed entirely
*against a feature branch

ouchadam · 2021-10-07T11:35:00Z

vector/src/gplay/java/im/vector/app/gplay/push/fcm/VectorFirebaseMessagingService.kt

@@ -238,87 +232,4 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() {
        }
        return false
    }
-
-    private fun handleNotificationWithoutSyncingMode(data: Map<String, String>, session: Session?) {


this was unused, if we want it back we can retrieve it from the source control history

Thanks for the cleanup. In practice I think this code will be lost forever :), but anyway, this is not a big loss

ouchadam · 2021-10-07T11:36:24Z

vector/src/main/java/im/vector/app/features/notifications/NotifiableEventResolver.kt

                    roomId = roomId,
                    timestamp = event.originServerTs ?: 0,
-                    noisy = false, // will be set later
+                    noisy = isNoisy,
                    title = stringProvider.getString(R.string.notification_new_invitation),
                    description = body.toString(),
                    soundName = null, // will be set later


this comment is a lie! the soundName is in fact never updated

ouchadam · 2021-10-07T11:38:36Z

vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt

-                hasBeenDisplayed = false
+            eventList.replace(eventId) {
+                when (it) {
+                    is InviteNotifiableEvent  -> it.copy(isRedacted = true).apply { hasBeenDisplayed = false }


updating hasBeenDisplayed is temporary

in the redesign simply mutating the contents of the eventList will allow the notifications to be rerendered, no need to use the hasBeenDisplayed state

the eventList will be a 1 to 1 mapping of the currently displaying notifications

the eventList will be a 1 to 1 mapping of the currently displaying notifications

Could be nice to have this comment above eventList declaration

will add 👍

github-actions · 2021-10-07T11:57:30Z

Unit Test Results

  42 files   42 suites 47s ⏱️
  83 tests   83 ✔️ 0 💤 0 ❌
220 runs 220 ✔️ 0 💤 0 ❌

Results for commit 2a328c6.

♻️ This comment has been updated with latest results.

… the constructor

- 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

… available

…tation Including room name in the invitation notification

ouchadam · 2021-10-13T07:40:31Z

vector/src/main/java/im/vector/app/features/notifications/NotifiableEventResolver.kt

        val content = event.content?.toModel<RoomMemberContent>() ?: return null
        val roomId = event.roomId ?: return null
        val dName = event.senderId?.let { session.getRoomMember(it, roomId)?.displayName }
        if (Membership.INVITE == content.membership) {
-            val body = noticeEventFormatter.format(event, dName, isDm = session.getRoomSummary(roomId)?.isDirect.orFalse())
+            val roomSummary = session.getRoomSummary(roomId)


the room name change is from #4230 (for next time I'll be sure to better highlight the merge ordering to avoid growing diffs)

bmarty

LGTM, can be merged to the other branch.
I have to admit I made a fast review, will read (and test) more carefully the PR which will land to develop.

ouchadam · 2021-10-13T09:47:26Z

vector/src/main/java/im/vector/app/features/notifications/NotifiableEventResolver.kt

-
-            val notifiableEvent = resolveMessageEvent(timelineEvent, session)
-
-            if (notifiableEvent == null) {


resolveMessageEvent no longer returns null 🎉

ouchadam · 2021-10-13T09:50:01Z

@bmarty thanks! this PR is probably the lowest risk compared to the big creation/display split and the diffing changes

I could create a separate PR will all the changes merged purely for testing if you think that would help?

bmarty · 2021-10-13T09:54:38Z

I could create a separate PR will all the changes merged purely for testing if you think that would help?

Yes, it will be a PR from branch feature/adm/notification-redesign, no? I was thinking you will create one, but it was not the plan? Maybe because feature/adm/notification-redesign only contains approved PRs content?

ouchadam · 2021-10-13T10:04:30Z

yep exactly ^^^ feature/adm/notification-redesign only contains approved PRs

although now I think about it, it might be better to PR the main logic change and the diffing into the same PR, that way there's only 1 PR remaining. then we don't need to the separate test branch (in this case)

it's a bit easier to review new code by itself than new code + changes

I'll still raise the full feature PR against develop for PR/commit tracking purposes but it will only contain already reviewed PRs

ouchadam commented Oct 7, 2021

View reviewed changes

ouchadam mentioned this pull request Oct 7, 2021

Splitting notifications into separate creation and displaying passes #4185

Closed

ouchadam added 9 commits October 11, 2021 17:57

removing unused commented code

44f2320

removing non common properties form the base event

6ddb153

making non overriden properties immutable by passing the values intro…

6080024

… the constructor

removing unused property (written to but never read)

1acf4da

making the isRedacted event property immutable

012df13

- also makes the notifiable events sealed interfaces so that we can copy the data classes with new redacted values when it changes

making the noisy property immutable

45d30e3

separating the mutable vars from the immutable ones, they'll be remov…

e3f2161

…ed or made immutable by the notification redesign

updating the push gateway property to reflect that it mean the event …

dcae177

…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

formatting

d5e73cc

ouchadam force-pushed the feature/adm/immutable-notifiable-models branch from 55681b1 to d5e73cc Compare October 11, 2021 16:57

ouchadam mentioned this pull request Oct 12, 2021

Notification diffing #4220

Closed

including the room name in the invitation event if the room sumary is…

ae2dadf

… available

ouchadam mentioned this pull request Oct 12, 2021

Including room name in the invitation notification #4230

Merged

ouchadam and others added 2 commits October 12, 2021 17:33

adding changelog entry

ecaac69

Merge pull request #4230 from vector-im/feature/adm/room-name-in-invi…

2a328c6

…tation Including room name in the invitation notification

ouchadam commented Oct 13, 2021

View reviewed changes

ouchadam requested a review from bmarty October 13, 2021 07:40

bmarty approved these changes Oct 13, 2021

View reviewed changes

ouchadam commented Oct 13, 2021

View reviewed changes

ouchadam merged commit ba6e019 into feature/adm/notification-redesign Oct 13, 2021

ouchadam deleted the feature/adm/immutable-notifiable-models branch October 13, 2021 09:50

ouchadam mentioned this pull request Oct 20, 2021

Notifications redesign #4274

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Immutable NotifiableEvent models #4182

Immutable NotifiableEvent models #4182

ouchadam commented Oct 7, 2021

ouchadam Oct 7, 2021

bmarty Oct 13, 2021

ouchadam Oct 7, 2021

ouchadam Oct 7, 2021 •

edited

Loading

bmarty Oct 13, 2021

ouchadam Oct 13, 2021

github-actions bot commented Oct 7, 2021 •

edited

Loading

ouchadam Oct 13, 2021

bmarty left a comment

ouchadam Oct 13, 2021

ouchadam commented Oct 13, 2021

bmarty commented Oct 13, 2021

ouchadam commented Oct 13, 2021


		val notifiableEvent = resolveMessageEvent(timelineEvent, session)

		if (notifiableEvent == null) {

Immutable NotifiableEvent models #4182

Immutable NotifiableEvent models #4182

Conversation

ouchadam commented Oct 7, 2021

ouchadam Oct 7, 2021

Choose a reason for hiding this comment

bmarty Oct 13, 2021

Choose a reason for hiding this comment

ouchadam Oct 7, 2021

Choose a reason for hiding this comment

ouchadam Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

bmarty Oct 13, 2021

Choose a reason for hiding this comment

ouchadam Oct 13, 2021

Choose a reason for hiding this comment

github-actions bot commented Oct 7, 2021 • edited Loading

Unit Test Results

ouchadam Oct 13, 2021

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

ouchadam Oct 13, 2021

Choose a reason for hiding this comment

ouchadam commented Oct 13, 2021

bmarty commented Oct 13, 2021

ouchadam commented Oct 13, 2021

ouchadam Oct 7, 2021 •

edited

Loading

github-actions bot commented Oct 7, 2021 •

edited

Loading