-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve thread DB access handling #1290
Conversation
5332055
to
37ba46d
Compare
e1f091e
to
83560d7
Compare
* OneSignalCacheCleaner shouldn't have access to the outcome table when we have a class responsible for managing the outcome database table
* IAM database access and all regarding IAM should live in OSInAppMessageController
* OSNotificationDataController will be the new class responsible for handling notification table access
* Move clearOneSignalNotifications from OneSignal.java to OSNotificationDataController * Move removeGroupedNotifications from OneSignal.java to OSNotificationDataController * Move removeNotification from OneSignal.java to OSNotificationDataController * Move isDuplicateNotification from OneSignal.java to OSNotificationDataController * isDuplicateNotification no longer returns a boolean, all methods using isDuplicateNotification wait for callback result
83560d7
to
47ae123
Compare
01b1877
to
a2b9fd4
Compare
a2b9fd4
to
b5b1792
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 21 of 21 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Jeasmine)
OneSignalSDK/onesignal/src/main/java/com/onesignal/ADMMessageHandlerJob.kt, line 17 at r1 (raw file):
// FCMBroadcastReceiver.completeWakefulIntent(intent); processedResult?.let {
Not sure what the best practices in general are for Kotlin but I think it would be cleaner like this.
if (processedResult?.processed() == true) return
OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationBundleProcessor.java, line 374 at r1 (raw file):
maximizeButtonsFromBundle(bundle); if (inAppMessagePreviewHandled(bundleResult, bundle)) {
👍 Nice clean up on moving the preview logic to it's own method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Lots of good clean up in this PR!
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Jeasmine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Jeasmine)
OneSignalSDK/onesignal/src/main/java/com/onesignal/ADMMessageHandlerJob.kt, line 17 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Not sure what the best practices in general are for Kotlin but I think it would be cleaner like this.
if (processedResult?.processed() == true) return
This is really more of a nit, not a blocker.
The Problem
isDuplicateNotification methods sometimes ended called on the main thread, this can en on an ANR is Notification table is being blocked by other thread.
Solution
Move all database access from OneSignal.java to the new class OSNotificationDataController. In the future, we will want all notification table access to be on the same class to have a single responsible class, also to have few threads handling notification table access.
Move outcome table access to OSOutcomeEventsCache
Move IAM cache cleaning to OSInAppMessageController
Rename OneSignalCacheCleaner to OSNotificationDataController
Move OneSignal db access to OSNotificationDataController
Fix #1284
This change is