Skip to content
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

session replay: use new apis for screen capture, add support for screenshot capture #93

Merged
merged 23 commits into from
Nov 4, 2024

Conversation

Augustyniak
Copy link
Contributor

@Augustyniak Augustyniak commented Nov 1, 2024

Fixes BIT-4005

  1. Expose callbacks that are used by shared-core to inform the platform layer about a need to take a screenshot.
  2. Remove session replay runtime variables from platform layer - the feature is controlled by the rRust layer now.
  3. Breaking API change - remove an option to specify the rate at which session replay screens are captured. This is controlled with the use of runtime variables now. Another change is that customers can no longer disable session replay by passing null as SessionReplayConfiguration.

@Augustyniak Augustyniak requested review from murki and Reflejo November 1, 2024 14:12
@Augustyniak Augustyniak changed the title use new session replay apis session replay: use new apis for screen capture, add support for screenshot capture Nov 1, 2024
Copy link
Contributor

@Reflejo Reflejo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ios part lgtm

override fun captureScreenshot() {
// This implementation is required for the SDK to support screenshot capture on Android.
// As currently implemented, the function must emit a session replay screenshot log.
// Without this emission, the SDK is blocked from requesting additional screenshots.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing a //TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

processLifecycleOwner.lifecycle.removeObserver(this)
}
override fun captureScreen() {
val skipReplayComposeViews = runtime?.isEnabled(RuntimeFeature.SESSION_REPLAY_COMPOSE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is runtime nullable? shouldn't it just be passed at construction time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a TODO to the property. TL;DR; is that it cannot be nonnullable since runtime can be created only after the Rust logger is created and SessionReplayTarget is one of the arguments of Rust logger creation method call.

There is another TODO for making SessionReplayTarget an argument of Rust logger start method call as opposed to Rust logger creation method call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That 2nd TODO seems more relevant now that none of the recurrent loggers are managed at the platform level (resource utilization and now replay). Hopefully we can get to that todo next.

* @param replayMapperConfiguration Map used to matching third party Android views to Bitdrift view types.
*/
data class SessionReplayConfiguration @JvmOverloads constructor(
val captureIntervalMs: Long = 3000,
val replayMapperConfiguration: ReplayMapperConfiguration? = null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 apparently this is the only part of the configuration left, which makes the entire nested structure unnecessary complicated. not much to do now but leaving this message for posterity: we should simplify this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, I almost removed it but then realize that we actually depend on the replayMapperConfiguration for some of the customers.

when (event) {
Lifecycle.Event.ON_CREATE -> replayModule.create(context)
Lifecycle.Event.ON_START -> replayModule.start()
Lifecycle.Event.ON_STOP -> replayModule.stop()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where did we land on not capturing replay wireframes when the app is in the background? you just went ahead and removed it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified that capturing works when you tap home button or tap back button. I would need your help to verify that it works (doesn't crash) when the application launches in the background.

For cases when the app is backgrounded the session replay doesn't see any views and that makes us NOT to emit session replay log so effectively speaking the end behavior is just as it used to be.

@Augustyniak Augustyniak enabled auto-merge (squash) November 4, 2024 19:25
@Augustyniak Augustyniak merged commit 30539ad into main Nov 4, 2024
15 checks passed
@Augustyniak Augustyniak deleted the update-to-new-session-replay-target branch November 4, 2024 19:37
@github-actions github-actions bot locked and limited conversation to collaborators Nov 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants