-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
[SR] Handle orientation change #3335
[SR] Handle orientation change #3335
Conversation
…sion-replay-orientation-change
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
03382fb | 320.57 ms | 374.67 ms | 54.10 ms |
ff9aecd | 359.28 ms | 450.31 ms | 91.03 ms |
891e156 | 346.82 ms | 407.51 ms | 60.69 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
03382fb | 1.70 MiB | 2.30 MiB | 617.80 KiB |
ff9aecd | 1.70 MiB | 2.30 MiB | 617.80 KiB |
891e156 | 1.70 MiB | 2.30 MiB | 617.80 KiB |
return | ||
} | ||
|
||
recorder?.stopRecording() |
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'm wondering if onConfigurationChanged
could be triggered without the width/height actually changing? Would it make sense to guard this the following way ~ if (oldConfig == newConfig) { return }
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 it can be triggered in the following cases
we're mostly interested in the first two, but I guess it's fine to stop and restart the recording for the others as well, just to make sure we don't try to encode different configurations into the same video segment, wdyt? Otherwise, we'd probably have to check what actually changed in the configuration, and if it's not window/screen size, then we do nothing.
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 it makes sense to me to create a new segment, let's keep it simple and check later if too many config changes are causing an issue 👍 (e.g. keyboard show/hide, network provider changing, ...)
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.
sounds good! I guess we'd need some kind of internal tracking for that, but that's for later
a0c2678
into
rz/feat/session-replay-redact-options
#skip-changelog
📜 Description
session
mode for now, to support this inbuffer
mode we'd have to split the buffered video into multiple segments (as many as the number of orientation changes), but this will come laterreplayId
insession
mode💡 Motivation and Context
Part of getsentry/sentry#63255