-
Notifications
You must be signed in to change notification settings - Fork 371
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
Move start call of unused services off the main thread #2127
Conversation
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt
Outdated
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt
Outdated
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt
Outdated
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt
Outdated
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt
Outdated
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt
Outdated
Show resolved
Hide resolved
a5e935f
to
f2559c3
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.
Left another 2 comments.
Also this existing comment is still unanswered and not addressed:
https://github.com/OneSignal/OneSignal-Android-SDK/pull/2127/files#r1644805601
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt
Outdated
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/services/ServiceProvider.kt
Outdated
Show resolved
Hide resolved
8f95d63
to
5469f76
Compare
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt
Outdated
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt
Outdated
Show resolved
Hide resolved
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.
Tests are failing on CI, made a comment about one test that will not pass often.
...lSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt
Show resolved
Hide resolved
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.
Changes look good now, but probably want to rebase to clean up a few a few of the fixup commits.
…ice start in background
afdfdc4
to
1346a32
Compare
Revert "Merge pull request #2127 from OneSignal/get-service-by-getter"
Description
One Line Summary
Move start call of unused services off the main thread to make the initWithContext more efficient.
Details
Motivation
Our SDK initialization is currently taking longer to finish compare to some other SDKs. One of the reason is we construct service components like Location Manager or IAM Manager during the initialization phrase when they are not really needed until later. This PR aims to move the initialization of these services to background thread so they don't slow down the overall SDK initialization.
Scope
The retrieval of services that were changed to initialize in background is not longer from a saved instance in OneSignalImp.kt. Instead, each of these components has a getter that retrieve its instance from the service provider.
Testing
Manual testing
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is