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

Feat: Screenshots on reports #576

Closed
wants to merge 6 commits into from
Closed

Feat: Screenshots on reports #576

wants to merge 6 commits into from

Conversation

ueman
Copy link
Collaborator

@ueman ueman commented Aug 31, 2021

📜 Description

💡 Motivation and Context

Closes #556

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@ueman
Copy link
Collaborator Author

ueman commented Aug 31, 2021

@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2021

Codecov Report

Base: 89.90% // Head: 89.90% // No change to project coverage 👍

Coverage data is based on head (8169028) compared to base (870f5eb).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #576   +/-   ##
=======================================
  Coverage   89.90%   89.90%           
=======================================
  Files         106      106           
  Lines        3388     3388           
=======================================
  Hits         3046     3046           
  Misses        342      342           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@marandaneto
Copy link
Contributor

@ueman
Copy link
Collaborator Author

ueman commented Aug 31, 2021

looks like the macOS hang again https://github.com/getsentry/sentry-dart/pull/576/checks?check_run_id=3472320682

Ahhh, I really dislike that error :(

void dispose() {
hub.configureScope((scope) {
// The following doesn't work
// scope.attachements.remove(ScreenshotAttachment());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How are Attachments supposed to be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Scope#clearAttachments()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But that clears all Attachments, I just want to remove the Screenshot attachement.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, that's how it works for all the list, either you clean them all or just add a new one, and all of them are sent.
a new idea came up before which is a parameter of adding an attachment, like addAttachment(attachment, onlyOnce: true)
as soon as its sent and onlyOnce is true, the attachment remove itself from the list

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines +81 to +96
FutureOr<Uint8List> get bytes async {
//return await createScreenshot() ?? Uint8List.fromList([]);
final instance = SchedulerBinding.instance;
if (instance == null) {
return Uint8List.fromList([]);
}

final _completer = Completer<Uint8List?>();
// We add an post frame callback because we aren't able to take a screenshot
// if there's currently a draw in process.
instance.addPostFrameCallback((timeStamp) async {
final image = await createScreenshot();
_completer.complete(image);
});
return await _completer.future ?? Uint8List.fromList([]);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm pretty sure this is needed, though in a test it works without this. Maybe it also works on a device without this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say that we should keep the callback then, since the chances of an error happening during the draw process is quite high

final bytes = await image.toByteData(format: ui.ImageByteFormat.png);
return bytes?.buffer.asUint8List();
}
} catch (_) {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be logged somehow

/// Remarks:
/// - Depending on the place where it's used, you might have a transparent
/// background.
/// - Platform Views currently can't be captured.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's this issue: flutter/flutter#25306

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

is flutter/flutter#83856 a blocker?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wouldn't say so but it's definitly not nice. Most Flutter apps I've seen don't have a lot of platform views, so it should be fine for most users.

final renderObject = _gloablKey.currentContext?.findRenderObject();

if (renderObject is RenderRepaintBoundary) {
final image = await renderObject.toImage(pixelRatio: 1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add an option for this

if (instance == null) {
_completer.complete(null);
return await _completer.future ?? Uint8List.fromList([]);
return Uint8List.fromList([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we allow returning null then? it does not make sense to return an array of nothing, unless we filter out before adding the attachment if the array is empty or not

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would require all Attachments to allow returning null. Is that acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah not nice, but we should filter attachments with 0 bytes before adding to the envelope

@ueman ueman added the flutter flutter label Oct 6, 2021
@marandaneto
Copy link
Contributor

draft of the spec btw https://github.com/getsentry/develop/pull/422/files

@marandaneto marandaneto mentioned this pull request Oct 20, 2021
5 tasks
Copy link

@eshopekstore eshopekstore left a comment

Choose a reason for hiding this comment

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

@

# Conflicts:
#	flutter/example/lib/main.dart
#	flutter/lib/sentry_flutter.dart
@github-actions
Copy link
Contributor

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Screenshots on reports ([#576](https://github.com/getsentry/sentry-dart/pull/576))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 8169028

@github-actions
Copy link
Contributor

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 288.82 ms 332.56 ms 43.74 ms
Size 5.94 MiB 6.92 MiB 1005.82 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
eb1a7c1 332.98 ms 381.55 ms 48.57 ms
322aa66 284.98 ms 341.76 ms 56.78 ms
633cf2e 289.36 ms 340.38 ms 51.02 ms
9c5aec6 287.33 ms 320.94 ms 33.61 ms
870f5eb 329.45 ms 369.29 ms 39.84 ms
6d317ea 303.46 ms 356.06 ms 52.60 ms
72dfc83 298.62 ms 340.14 ms 41.52 ms
4efee31 308.92 ms 368.68 ms 59.76 ms
49a149b 301.96 ms 344.51 ms 42.55 ms
1c6eb5b 350.69 ms 393.86 ms 43.17 ms

App size

Revision Plain With Sentry Diff
eb1a7c1 5.94 MiB 6.92 MiB 1005.76 KiB
322aa66 5.94 MiB 6.92 MiB 1005.75 KiB
633cf2e 5.94 MiB 6.92 MiB 1001.53 KiB
9c5aec6 5.94 MiB 6.92 MiB 1001.61 KiB
870f5eb 5.94 MiB 6.92 MiB 1005.77 KiB
6d317ea 5.94 MiB 6.92 MiB 1001.74 KiB
72dfc83 5.94 MiB 6.92 MiB 1001.71 KiB
4efee31 5.94 MiB 6.92 MiB 1003.76 KiB
49a149b 5.94 MiB 6.92 MiB 1001.54 KiB
1c6eb5b 5.94 MiB 6.92 MiB 1001.53 KiB

@github-actions
Copy link
Contributor

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1276.96 ms 1306.33 ms 29.37 ms
Size 8.15 MiB 9.13 MiB 1000.14 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
1c6eb5b 1277.85 ms 1285.71 ms 7.86 ms
322aa66 1251.68 ms 1275.52 ms 23.84 ms
9c5aec6 1266.51 ms 1274.65 ms 8.14 ms
21845e2 1279.37 ms 1298.81 ms 19.45 ms
4efee31 1270.33 ms 1285.75 ms 15.42 ms
49a149b 1296.47 ms 1320.20 ms 23.73 ms
56810ff 1267.59 ms 1293.48 ms 25.89 ms
870f5eb 1267.78 ms 1286.86 ms 19.08 ms
633cf2e 1257.96 ms 1275.73 ms 17.77 ms
3e5ee37 1248.25 ms 1265.38 ms 17.13 ms

App size

Revision Plain With Sentry Diff
1c6eb5b 8.15 MiB 9.12 MiB 986.27 KiB
322aa66 8.15 MiB 9.12 MiB 992.53 KiB
9c5aec6 8.15 MiB 9.12 MiB 986.23 KiB
21845e2 8.15 MiB 9.12 MiB 991.34 KiB
4efee31 8.15 MiB 9.12 MiB 991.35 KiB
49a149b 8.15 MiB 9.12 MiB 986.26 KiB
56810ff 8.15 MiB 9.12 MiB 987.35 KiB
870f5eb 8.15 MiB 9.13 MiB 1000.08 KiB
633cf2e 8.15 MiB 9.12 MiB 986.26 KiB
3e5ee37 8.15 MiB 9.12 MiB 986.23 KiB

@ueman
Copy link
Collaborator Author

ueman commented Oct 24, 2022

Closing in favor of #1088

@ueman ueman closed this Oct 24, 2022
@denrase denrase mentioned this pull request Oct 25, 2022
5 tasks
@marandaneto marandaneto deleted the feat/screenshot branch January 13, 2023 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flutter flutter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support screenshot feature (via attachments)
5 participants