-
Notifications
You must be signed in to change notification settings - Fork 32
Integrations Logic fix + Copy mechanism for payloads #59
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
Conversation
core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt
Outdated
Show resolved
Hide resolved
core/src/main/java/com/segment/analytics/kotlin/core/platform/Plugin.kt
Outdated
Show resolved
Hide resolved
core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt
Outdated
Show resolved
Hide resolved
core/src/main/java/com/segment/analytics/kotlin/core/platform/plugins/SegmentDestination.kt
Outdated
Show resolved
Hide resolved
f7d7a18
to
3709603
Compare
Codecov Report
@@ Coverage Diff @@
## main #59 +/- ##
=======================================
Coverage ? 57.71%
Complexity ? 418
=======================================
Files ? 66
Lines ? 7137
Branches ? 690
=======================================
Hits ? 4119
Misses ? 2506
Partials ? 512 Continue to review full report at Codecov.
|
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.
LGTM
* in coroutines. | ||
*/ | ||
@BlockingApi | ||
fun settings(): Settings? = runBlocking { |
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.
need to add a settings
function to JavaAnalytics
for compatibility. also unit tests for them
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.
ah good catch. will add those
@@ -79,6 +104,7 @@ suspend fun Analytics.checkSettings() { | |||
log("Dispatching update settings on ${Thread.currentThread().name}") | |||
store.dispatch(System.UpdateSettingsAction(settingsObj), System::class) | |||
update(settingsObj, updateType) | |||
store.dispatch(System.ToggleSettingsDispatch(dispatched = true), System::class) |
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.
just to confirm: we dispatch twice here because we want to ensure settings being successfully propagated to subscribers and then toggle the state?
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.
yup i didnt want to preemptively toggle the dispatch
} | ||
// Differs from swift, bcos kotlin can store `enabled` state. ref: https://git.io/J1bhJ | ||
// finding it in timeline rather than using the ref that is provided to cover our bases | ||
find(plugin::class)?.enabled = true |
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.
❤️ nice and clean
Uh oh!
There was an error while loading. Please reload this page.