User Details
- User Since
- Dec 29 2021, 2:54 AM (90 w, 3 d)
Tue, Sep 19
Rebase before landing
Rebase before landing
Rebase before landing
- Don't wait forever
- Rebase before landing
Some last changes and rebase before landing
I am leaving for vacation so I don't want to block this diff. I am adding two reviewers:
- @kamil for changes related to data clearance. I didn't add him as a blocking reviewer since he might be absent for a longer time. But if he is back quickly I would appreciate his review on the data clearance.
- @bartek for changes related to putting entire authentication payload in Secure Store at once.
- Take advantage of builder mutability.
- Use streams instead of iteration.
- Don't iterate twice over notifications id list when removing a notification id
THos should cover all comments from Tomek last review
Move isStaffRelease checks to better places
Use local notifications for collapsible notifications to achieve previous UX experience.
Mon, Sep 18
Set auto cancel property to false only for staff releases
Refactor using isStaffRelease utility.
Fri, Sep 15
Rebase before landing
Rebase before landing
Enhance variable naming
- Update typing
- Save device token to hash mapping instead of plain hash list.
Thu, Sep 14
Wed, Sep 13
Tue, Sep 12
If the UX change originating from this diff is not acceptable the alternative approach we can experiment with is to silence every collapsible notifications and schedule local notifications from the NSE. This way we will be able to set request.identifier property (since we are the ones creating the UNNotificationRequest instance) and let the OS do the coalescing.
Update coalescing condition.
As mentioned in the description coalescing mechanism changes slightly and so does user experience. I recommend that reviewers watch recordings attached below:
Current coalescing:
NSE coalescing:
Mon, Sep 11
Remove unnecessary Kotlin WorkManager dependency
I checked our app .apk file size twice: first time on a parent commit to this one and second time on this commit. In between I uninstalled the app and executed ./gradlew clean. My results are below:
Parent commit:
This commit:
So we probably have already had guava in some other dependency since the size in MB didn't change at all. However please correct my if my method of checking the size was wrong.
One last question: Is it guaranteed that we call the notif's completionHandler only after the deletion tasks are finished?
It is not guaranteed and it is impossible to guarentee that since contentHandler is called quickly in the NSE while blob deletion takes place on the background thread in the main app so the system decides when it is best to call it. Additionally main app might not be running when NSE processes notification. However why would such guarantee be necessary in the first place? Blob download and persistence is synchronous and writing blob metadata to the storage which main app reads during deletion takes place afterwards. So the deletion process won't start until notification payload is persisted on the device so when user opens the app the message will be loaded and immediately visible in the app regardless on when delete requests are issued.
I understand that I am not a reviewer but @kamil wanted me to see this diff and I would like to note one thing. This cron job will only fire if keyserver's very first prekey's were marked as published somewhere else. In practice keyserver prekeys are published for the first time here: https://github.com/CommE2E/comm/blob/master/keyserver/src/responders/keys-responders.js#L59. However the only reason prekeys are marked as published there is that at the time this code was implemented we wanted to launch e2e notifs ASAP but the identity service was not ready, so we agreed on a temporary solution to mark prekey's as published when user fetches them to initialise olm notifications session with the keyserver. That said as of this differential this line should be deleted and we should find a place to publish keyserver's prekeys for the very first time after olm accounts for the keyserver are created.
Fri, Sep 8
- Delete blobs even if app is running in the background.
- Fix type in ongoing.
Shouldn't we also send a delete request when a notif is received while the app is running?
This is possible using the darwin notification mechanism. We already use it so that NSE can inform the backgrounded running app that it can fetch messages from the temporary storage. One thing that I need to check however is that whether darwin notifications are delivered instantly even if the app is in the background or they are queued until the app enters the foreground. In our case we would prefer to second since I don't want to execute networking code when the app is backgrounded (the reason for this was explained in one of my first comments on this diff: https://phab.comm.dev/D9091#inline-57952).
Thu, Sep 7
Rebase before landing
Rebase before landing
Rebase before landing
Rebase before landing
Wed, Sep 6
Rebase
Rebase
Rebase
Share blob client code with the main app.
Tue, Sep 5
- Primary keyword is not valid when talking about the "main" olm account - the convention is to use Content keyword.
- Identity keyword refers to curve25519. ed25519 is signing key.
Fri, Sep 1
Implement WIP of comm access token authentication on iOS