-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: add support for automatic activity screen tracking #334
Conversation
Sample app builds 📱Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/cdp #334 +/- ##
=================================================
- Coverage 54.45% 53.64% -0.81%
+ Complexity 282 276 -6
=================================================
Files 109 105 -4
Lines 2534 2466 -68
Branches 355 345 -10
=================================================
- Hits 1380 1323 -57
+ Misses 1032 1024 -8
+ Partials 122 119 -3 ☔ View full report in Codecov by Sentry. |
Build available to test |
📏 SDK Binary Size Comparison ReportNo changes detected in SDK binary size ✅ |
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.
Small comments, rest looks good
override val type: Plugin.Type = Plugin.Type.Utility | ||
override lateinit var analytics: Analytics | ||
|
||
override fun onActivityStarted(activity: Activity?) { |
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.
I think the activity is marked with NonNull
? 🤔
} | ||
} | ||
// If screen name is null, we do not track the screen | ||
screenName?.let { |
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.
Should we check for blank here too?
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.
it might not be needed but doesnt hurt to add it
} catch (e: Exception) { | ||
// Should we log exceptions that happen? Ignore them? How rare is an exception happen in this function? | ||
// ActionUtils.getErrorAction(ErrorResult(error = ErrorDetail(message = "Unable to track, $activity"))) | ||
} |
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.
Are these comments intended?
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.
yeah we should rather just log them
"ActivityHomeActivity".getScreenNameFromActivity() shouldBeEqualTo "Home" | ||
"ItemsListActivity".getScreenNameFromActivity() shouldBeEqualTo "Items" | ||
"ItemsDialogActivity".getScreenNameFromActivity() shouldBeEqualTo "Items" | ||
"MapFragmentActivity".getScreenNameFromActivity() shouldBeEqualTo "Map" |
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.
Would like to see some entries here that remain the same, e.g. SplashScreen
## [4.0.0](3.11.1...4.0.0) (2024-07-24) ### ⚠ BREAKING CHANGES * android as CDP source (#412) ### Features * add support for automatic activity screen tracking ([#334](#334)) ([9d239c8](9d239c8)) * added track methods ([#328](#328)) ([81d825f](81d825f)) * android as CDP source ([#412](#412)) ([772b489](772b489)) * expose public APIs to identify and clear profiles ([#322](#322)) ([09dea32](09dea32)) * track device with attributes ([#341](#341)) ([7cb17c1](7cb17c1)) * track metric event ([#335](#335)) ([f7af4e6](f7af4e6))
closes: https://linear.app/customerio/issue/MBL-286/expose-plugin-for-automatic-screen-tracking
Includes: