-
-
Notifications
You must be signed in to change notification settings - Fork 452
New User Feedback form #4384
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
New User Feedback form #4384
Conversation
Added Scopes.captureFeedback API Changed the MainActivity sample app to use the new API
captureFeedback does not update scope lastEventId anymore
added SentryOptions.beforeSendFeedback implemented SentryClient.captureFeedback
# Conflicts: # CHANGELOG.md # sentry/api/sentry.api # sentry/src/main/java/io/sentry/SentryClient.java
added first draft of SentryUserFeedbackDialog
…idget # Conflicts: # CHANGELOG.md
added feedbackOptions to SentryOptions added callbacks to SentryUserFeedbackDialog
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
21335f4 | 451.73 ms | 476.46 ms | 24.73 ms |
c3914aa | 401.70 ms | 437.38 ms | 35.68 ms |
75996c3 | 414.94 ms | 454.02 ms | 39.08 ms |
9f19891 | 402.73 ms | 525.00 ms | 122.27 ms |
b79d17c | 431.40 ms | 488.06 ms | 56.66 ms |
799de03 | 403.33 ms | 460.19 ms | 56.86 ms |
1792f53 | 430.40 ms | 486.48 ms | 56.08 ms |
f362a97 | 414.44 ms | 463.76 ms | 49.32 ms |
560a588 | 466.82 ms | 493.07 ms | 26.25 ms |
a6cc553 | 414.43 ms | 463.46 ms | 49.03 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
21335f4 | 1.58 MiB | 2.12 MiB | 548.14 KiB |
c3914aa | 1.58 MiB | 2.12 MiB | 548.00 KiB |
75996c3 | 1.58 MiB | 2.12 MiB | 547.61 KiB |
9f19891 | 1.58 MiB | 2.09 MiB | 515.84 KiB |
b79d17c | 1.58 MiB | 2.09 MiB | 517.60 KiB |
799de03 | 1.58 MiB | 2.12 MiB | 548.40 KiB |
1792f53 | 1.58 MiB | 2.12 MiB | 547.60 KiB |
f362a97 | 1.58 MiB | 2.12 MiB | 548.01 KiB |
560a588 | 1.58 MiB | 2.12 MiB | 548.00 KiB |
a6cc553 | 1.58 MiB | 2.12 MiB | 548.39 KiB |
added feedback message truncation SentryClient doesn't check for backfillable feedbacks anymore
…idget # Conflicts: # CHANGELOG.md
added dialog theme to sample app MainActivity "send user feedback" button now opens feedback dialog instead of sending feedback
# Conflicts: # sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.java # sentry/api/sentry.api # sentry/src/main/java/io/sentry/HubAdapter.java # sentry/src/main/java/io/sentry/HubScopesWrapper.java # sentry/src/main/java/io/sentry/IScopes.java # sentry/src/main/java/io/sentry/NoOpHub.java # sentry/src/main/java/io/sentry/NoOpScopes.java # sentry/src/main/java/io/sentry/Scopes.java # sentry/src/main/java/io/sentry/ScopesAdapter.java # sentry/src/main/java/io/sentry/SentryClient.java # sentry/src/main/java/io/sentry/transport/RateLimiter.java
fixed feedback form behaviour added manifest options
updated changelog
updated changelog
added delegate listener to avoid user overwriting our OnDismissListener for the feedback dialog
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
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.
Looks good to me, I left a few comments around the API, let's sort those out before merging. On top of that: Do you know if other SDKs provide default i18n for the widget? Or is it really up the the user to provide their own?
sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackDialog.java
Outdated
Show resolved
Hide resolved
android:id="@+id/sentry_dialog_user_feedback_title" | ||
android:layout_width="match_parent" | ||
android:layout_height="wrap_content" | ||
android:text="Report a Bug" |
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.
You could use "tools:text"
instead for some nice preview, as it all will be overwritten anyway right?
You're right, it would bloat the APK, especially if we support languages the consuming app doesn't. Let's stick to EN then. |
added SentryUserFeedbackDialog.Builder, allowing a configuration being passed for the current dialog options added SentryFeedbackOptions copy constructor
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.
Nice!
* | ||
* @param submitButtonLabel the label of the submit button | ||
*/ | ||
public void setSubmitButtonLabel(final @NotNull String submitButtonLabel) { |
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 thinking it'd be a nice DevX to have overloads that accept @StringRes int resId
like this.
Also this one (and other Strings) should probably be changed to a CharSequence
to make it more versatile
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.
ah I see it's in core sentry... still thinking we could expose those just without the annotation and have something like SentryOptions.ResourceProvider
which on android would return context.getResources()
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.
That's the reason we added the SentryUserFeedback.Builder
, which has the OptionsConfiguration
with void configure(final @NotNull Context context, final @NotNull SentryFeedbackOptions options);
I agree on the int
overload, though, and if you think it makes sense, i can add it in a followup PR
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 will change the options to CharSequence
📜 Description
Continuation of #4286
Add the new User Feedback form (an AlertDialog)
Add all relevant options for customizations and callbacks
Here is the result.
How the form is shown in the example app with light theme and no customization

How the form is shown in the example app with dark theme and no customization

How the form is shown in the example app with (pretty bad) customization

💡 Motivation and Context
Implements first part of #3613
💚 How did you test it?
ui test (
UserFeedbackUiTest
)📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps
Next PR will have the widget (the button that actually show the form)
After that, we'll have screenshots and whatever