Add screenshot capturing for Mac/iOS #849
Conversation
|
# Conflicts: # scripts/packaging/package-github.snapshot # scripts/packaging/package-marketplace.snapshot
plugin-dev/Source/Sentry/Private/Apple/AppleSentrySubsystem.cpp
Outdated
Show resolved
Hide resolved
| FString backupFilePath = FString::Printf(TEXT("%s%s%s.%s"), *name, TEXT("-backup-"), *originalTime.ToString(), *extension); | ||
| if (!fileManager.Move(*backupFilePath, *screenshotFilePath, true)) | ||
| { | ||
| UE_LOG(LogSentrySdk, Error, TEXT("Failed to backup screenshot.")); |
There was a problem hiding this comment.
Why do we need to make a backup of the file?
There was a problem hiding this comment.
No more backups - now we delete screenshot right after it was sent to Sentry.
There was a problem hiding this comment.
We're certain those screenshots were taken by us, right? No file deletion of other types of attachment where we could be deleting someone's files?
There was a problem hiding this comment.
Screenshots are saved separately in Saved/SentryScreenshots dir so deleting them after sending to Sentry should be safe.
plugin-dev/Source/Sentry/Private/Apple/AppleSentrySubsystem.cpp
Outdated
Show resolved
Hide resolved
|
|
||
| FString FIOSSentrySubsystem::GetScreenshotPath() const | ||
| { | ||
| return FPaths::Combine(FPaths::ProjectSavedDir(), TEXT("screenshot.png")); |
There was a problem hiding this comment.
Looks like we'll create conflicting file names.
Should we use something pseudo random here on the file name? We can return the file name around (I mean we are, here doing that already)
There was a problem hiding this comment.
Fair point - I've updated it so that we now append the event ID to the file name when one is already available (e.g. in the case of ensures).
For asserts/crashes we still fall back to using just screenshot.png so that the file can be easily found on the next app launch and attached to the corresponding event.
There was a problem hiding this comment.
We could consider any file that startWith("screenshot_") for example, and use a timestamp after _.
This way we are sure to never override anything accidently.
|
|
||
| if (isScreenshotAttachmentEnabled) | ||
| { | ||
| TryCaptureScreenshot(); |
There was a problem hiding this comment.
Should Try.. return bool to indicate whether it was successful? We can skip the next method call altogether if we know it failed
There was a problem hiding this comment.
Depending on the platform TryCaptureScreenshot may run asynchronously (i.e. on iOS screenshot capturing happens on the main thread) so returning a bool immediately won't always work as expected.
While we could wait for the operation to complete that's probably not ideal during crash handling. In that context, it's better to make a best-effort attempt to capture the screen without blocking.
There was a problem hiding this comment.
But we call UploadScreenshotForEvent without synchronizing at all.
is it likely the screenshot will be ready by the time we call UploadScreenshotForEvent ?
We just check for the file path existing after but I imagine it's likely not as the code will run much faster than a screenshot can be taken + file I/O
There was a problem hiding this comment.
Alright, now I see where the confusion is coming from - TryCaptureScreenshot() is a virtual void method defined in parent AppleSentrySubsystem which behaves synchronously on Mac and asynchronously on iOS.
Probably makes it worth considering separating the two implementations given their different nature.
|
|
||
| FString FIOSSentrySubsystem::GetScreenshotPath() const | ||
| { | ||
| return FPaths::Combine(FPaths::ProjectSavedDir(), TEXT("screenshot.png")); |
There was a problem hiding this comment.
We could consider any file that startWith("screenshot_") for example, and use a timestamp after _.
This way we are sure to never override anything accidently.
|
|
||
| if (isScreenshotAttachmentEnabled) | ||
| { | ||
| TryCaptureScreenshot(); |
There was a problem hiding this comment.
But we call UploadScreenshotForEvent without synchronizing at all.
is it likely the screenshot will be ready by the time we call UploadScreenshotForEvent ?
We just check for the file path existing after but I imagine it's likely not as the code will run much faster than a screenshot can be taken + file I/O
This PR adds automatic screenshot capturing for crash events on Mac and iOS.
Due to current Cocoa SDK limitations the underlying native library can capture screenshots only for certain event types depending on the platforms:
sentry-cocoafunctionality and for assertions/crashes using custom implementation.The actual screenshot attachment is sent to Sentry in a separate envelope which's constructed upon the next app launch after it crashed. This may change in future when at least one of the following Cocoa SDK tickets will be resolved:
hintssentry-cocoa#2325Related to #567