feat(user feedback): display a button in a window that presents a vc

[armcknight]

#skip-changelog; for #4270

Deliver a complete styled button like we have in the javascript SDK docs that opens a modal view where the form will go in a subsequent PR.

Check the linked issue for updates on what it looks like with various config settings and device settings like for accessibility and dark mode.

[brustolin]

I think we should detect TabBar to arrange the button according.

[brustolin]

h: We can debate it later, but currently we're organising Swift code under Swift folder, so

This	Should be here

You can keep the Obj-C integration here, but the file itself is in the wrong directory. The header should be in the include path, and the .m file in the Sentry folder. This organization relates to how we distribute via CocoaPods and, in the future, SPM, when we move back to sharing code instead of a pre-compiled framework.

Edit: I realized these files were included in a previous PR. We can either fix it in this one or in another one.

[armcknight]

I think we should detect TabBar to arrange the button according.

This should be on the consumer to use the layoutUIOffset and later, enabling draggability of the button will obviate this. I don't think trying to predict all the possible combinations of view hierarchy, or spelunking through them, is wise. That being said I'm not sure which sample app or device you're testing, because I thought I already fixed this with the sample apps' usage of said layoutUIOffset property 🤔

[brustolin]

That being said I'm not sure which sample app or device you're testing, because I thought I already fixed this with the sample apps' usage of said layoutUIOffset property 🤔

I just downloaded this branch.

[armcknight]

That being said I'm not sure which sample app or device you're testing, because I thought I already fixed this with the sample apps' usage of said layoutUIOffset property 🤔

I just downloaded this branch.

which sample app or device

[brustolin]

which sample app or device

iOS-Swift sample iPhone 16 Pro

[github-actions[bot]]

	Messages
:book:	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by :no_entry_sign: dangerJS against 4254b596930628006ed78fea9ef9ca4d67d06109

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 8.46561% with 346 lines in your changes missing coverage. Please review.

Project coverage is 91.434%. Comparing base (149877e) to head (4254b59). Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
...rFeedback/SentryUserFeedbackWidgetButtonView.swift	0.000%	154 Missing :warning:
...ations/UserFeedback/SentryUserFeedbackWidget.swift	0.000%	65 Missing :warning:
...tryUserFeedbackWidgetButtonMegaphoneIconView.swift	0.000%	50 Missing :warning:
...s/UserFeedback/SentryUserFeedbackIntegration.swift	0.000%	34 Missing :warning:
...onfiguration/SentryUserFeedbackConfiguration.swift	0.000%	18 Missing :warning:
...uration/SentryUserFeedbackThemeConfiguration.swift	0.000%	6 Missing :warning:
.../UserFeedback/SentryUserFeedbackIntegrationShell.m	0.000%	5 Missing :warning:
Sources/Sentry/NSLocale+Sentry.m	0.000%	4 Missing :warning:
Sources/Sentry/SentryOptions.m	85.185%	4 Missing :warning:
...ration/SentryUserFeedbackWidgetConfiguration.swift	0.000%	4 Missing :warning:
... and 1 more

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #4364       +/-   ##
=============================================
+ Coverage   91.318%   91.434%   +0.115%     
=============================================
  Files          609       614        +5     
  Lines        49855     68495    +18640     
  Branches     17932     24585     +6653     
=============================================
+ Hits         45527     62628    +17101     
- Misses        4236      5774     +1538     
- Partials        92        93        +1     

Files with missing lines	Coverage Δ
...guration/SentryUserFeedbackFormConfiguration.swift	0.000% <ø> (ø)
Sources/Sentry/SentrySDK.m	87.654% <81.818%> (-0.744%)	:arrow_down:
Sources/Sentry/NSLocale+Sentry.m	71.428% <0.000%> (-28.572%)	:arrow_down:
Sources/Sentry/SentryOptions.m	98.691% <85.185%> (-0.561%)	:arrow_down:
...ration/SentryUserFeedbackWidgetConfiguration.swift	0.000% <0.000%> (ø)
.../UserFeedback/SentryUserFeedbackIntegrationShell.m	0.000% <0.000%> (ø)
...uration/SentryUserFeedbackThemeConfiguration.swift	0.000% <0.000%> (ø)
...onfiguration/SentryUserFeedbackConfiguration.swift	0.000% <0.000%> (ø)
...s/UserFeedback/SentryUserFeedbackIntegration.swift	0.000% <0.000%> (ø)
...tryUserFeedbackWidgetButtonMegaphoneIconView.swift	0.000% <0.000%> (ø)
... and 2 more

... and 572 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 149877e...4254b59. Read the comment docs.

[armcknight]

The diff is currently a little over 500 lines added, note that half of that is just the UIBezier drawing the megaphone that I got from our SVG design asset :D

[github-actions[bot]]

Performance metrics :rocket:

	Plain	With Sentry	Diff
Startup time	1241.37 ms	1257.02 ms	15.65 ms
Size	21.90 KiB	723.37 KiB	701.47 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
60bfc9193451826534224411e4aba5520374bc85	1251.29 ms	1261.67 ms	10.39 ms
24e07440d85b8d07ae4a98885d0f8f440ede339e	1237.60 ms	1264.69 ms	27.09 ms
e070f8a8a655a997c08db8a86ad82280b348e668	1236.47 ms	1250.50 ms	14.03 ms
a4ee85bde26a2e61f9c2b65df6bcbd2b3f73eaf7	1227.57 ms	1232.32 ms	4.75 ms
42ef6baf82435c9e60f92974097657417ca4839a	1220.59 ms	1245.24 ms	24.65 ms
4f848d0fb5363bc5db7cc7c1ce8c053a1a52292d	1233.79 ms	1258.49 ms	24.70 ms
dd4145f06df6a1b565366b306cfe34f89c27c39e	1233.48 ms	1254.35 ms	20.88 ms
50bb751cfe50189673d3c0ec5b0d664659218ade	1296.52 ms	1323.73 ms	27.21 ms
5b14b06f568de94636a906774c944e7fae54ae68	1196.33 ms	1227.00 ms	30.67 ms
2deb275cd052bc453253e9e3f2f7b39d7b45a9ca	1213.47 ms	1243.63 ms	30.16 ms

App size

Revision	Plain	With Sentry	Diff
60bfc9193451826534224411e4aba5520374bc85	20.76 KiB	434.94 KiB	414.18 KiB
24e07440d85b8d07ae4a98885d0f8f440ede339e	21.58 KiB	709.06 KiB	687.48 KiB
e070f8a8a655a997c08db8a86ad82280b348e668	21.58 KiB	546.20 KiB	524.62 KiB
a4ee85bde26a2e61f9c2b65df6bcbd2b3f73eaf7	20.76 KiB	434.92 KiB	414.16 KiB
42ef6baf82435c9e60f92974097657417ca4839a	21.58 KiB	417.86 KiB	396.28 KiB
4f848d0fb5363bc5db7cc7c1ce8c053a1a52292d	21.58 KiB	713.91 KiB	692.33 KiB
dd4145f06df6a1b565366b306cfe34f89c27c39e	21.58 KiB	540.09 KiB	518.51 KiB
50bb751cfe50189673d3c0ec5b0d664659218ade	21.58 KiB	417.85 KiB	396.27 KiB
5b14b06f568de94636a906774c944e7fae54ae68	22.84 KiB	402.56 KiB	379.72 KiB
2deb275cd052bc453253e9e3f2f7b39d7b45a9ca	21.58 KiB	681.72 KiB	660.14 KiB

Previous results on branch: armcknight/feat(user-feedback)/widget

Startup times

Revision	Plain	With Sentry	Diff
b7f2f001c47ee59ea6edd4da5d2f650dbe92ea05	1230.78 ms	1252.25 ms	21.47 ms
04583b12ca0235a773be16e278b4ad93603255d2	1217.31 ms	1234.60 ms	17.28 ms
7f8951b03260dde3aaa52f19d21ebe4d7ddece05	1228.18 ms	1244.61 ms	16.43 ms
77b46c1e6fffb81a6b245ca87e29ede258b1de48	1239.90 ms	1259.65 ms	19.76 ms
9a094afcf74cd084132295889cdbd61b7b6f61f7	1236.98 ms	1260.67 ms	23.70 ms
27fdb5fa367205b510788aecc0d7f827c20f9b70	1224.61 ms	1247.08 ms	22.47 ms
57bfd949394abdbbbef578b133b7b206ce83a982	1225.29 ms	1242.58 ms	17.29 ms

App size

Revision	Plain	With Sentry	Diff
b7f2f001c47ee59ea6edd4da5d2f650dbe92ea05	21.90 KiB	722.76 KiB	700.86 KiB
04583b12ca0235a773be16e278b4ad93603255d2	21.90 KiB	724.61 KiB	702.71 KiB
7f8951b03260dde3aaa52f19d21ebe4d7ddece05	21.90 KiB	725.85 KiB	703.95 KiB
77b46c1e6fffb81a6b245ca87e29ede258b1de48	21.90 KiB	723.83 KiB	701.93 KiB
9a094afcf74cd084132295889cdbd61b7b6f61f7	21.90 KiB	724.61 KiB	702.71 KiB
27fdb5fa367205b510788aecc0d7f827c20f9b70	21.90 KiB	724.61 KiB	702.71 KiB
57bfd949394abdbbbef578b133b7b206ce83a982	21.90 KiB	724.52 KiB	702.62 KiB

[brustolin]

For default values, we should respect iOS assessibility values:

This:

Should be:

In this configuration

[armcknight]

@brustolin accessibility sizes and right-to-left languages are known tasks: https://github.com/getsentry/sentry-cocoa/issues/4270#issuecomment-2408210459

I already have right-to-left support going after i linked the PR last night, see the latest update in the issue: https://github.com/getsentry/sentry-cocoa/issues/4270#issuecomment-2417077633 with the implementation work in https://github.com/getsentry/sentry-cocoa/pull/4364/commits/abc51191c823c86b6cdf09317f14746e728b3e9d

and added dynamic type support in https://github.com/getsentry/sentry-cocoa/pull/4364/commits/be4eb19e1b2758bfaecbab9281fae1b3787eb564: https://github.com/getsentry/sentry-cocoa/issues/4270#issuecomment-2417693926

[armcknight]

I think configureTheme, should be inside configureWidget, or at least the properties that configure the widget, it took me an extra second to find this out, when it would be much easier to set the widget theme inside the configureWidget callback

This can be debated separately from this PR if people want to. I'll just note here that theme options will also be needed in the form, so I tried creating a composition hierarchy that indicates they are used by more than just the widget (button):

Since the widget and form will use a lot of the same options I don't think it makes sense to make separate theme objects for each to own separately.

[armcknight]

This need to be addressed before merging the PR.

@brustolin Would you mind if we do it in a separate PR along with https://github.com/getsentry/sentry-cocoa/pull/4364#discussion_r1787018390? I just don't want to mess the diff up here. Nothing is being shipped yet so there's no danger of a file not being delivered where it should be.

ETA: unless it's required for CI to pass anyways, that is... ETA2: doesn't seem so, so i'd prefer to defer this

[brustolin]

@brustolin Would you mind if we do it in a separate PR along with https://github.com/getsentry/sentry-cocoa/pull/4364#discussion_r1787018390? I just don't want to mess the diff up here. Nothing is being shipped yet so there's no danger of a file not being delivered where it should be.

Are you planning to merge this PR back to main without the form ready? If we make another release in the meantime, users will be able to show the button but it won't be working? What is the release plan/timeline for this?

I would happily agree to have a feature branch where we can merge this and any other partial work of user feedback.

[armcknight]

@brustolin Would you mind if we do it in a separate PR along with #4364 (comment)? I just don't want to mess the diff up here. Nothing is being shipped yet so there's no danger of a file not being delivered where it should be.

Are you planning to merge this PR back to main without the form ready? If we make another release in the meantime, users will be able to show the button but it won't be working? What is the release plan/timeline for this?

I would happily agree to have a feature branch where we can merge this and any other partial work of user feedback.

@brustolin yes, then plan is to merge to main for each PR. The option is currently in SentryOptions+Private so they should not be able to get a widget showing in their app.

[brustolin]

@brustolin yes, then plan is to merge to main for each PR. The option is currently in SentryOptions+Private so they should not be able to get a widget showing in their app.

Ok, good enough for me. Thanks!