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

Add a waitForFullDisplay parameter to the sentryTrace() SwiftUI modifier #4787

Closed
matthewreimer opened this issue Feb 3, 2025 · 6 comments · Fixed by #4797
Closed

Add a waitForFullDisplay parameter to the sentryTrace() SwiftUI modifier #4787

matthewreimer opened this issue Feb 3, 2025 · 6 comments · Fixed by #4797

Comments

@matthewreimer
Copy link
Contributor

matthewreimer commented Feb 3, 2025

Problem Statement

For control and for symmetry with SentryTracedView it would be nice for the sentryTrace() SwiftUI view modifier to have a waitForFullDisplay parameter.

Solution Brainstorm

Basically this:

--- SentryTracedView.swift.orig	2025-02-03 10:10:26
+++ SentryTracedView.swift	2025-02-03 10:10:57
@@ -1,7 +1,7 @@
 @available(iOS 13, macOS 10.15, tvOS 13, watchOS 6.0, *)
 public extension View {
-    func sentryTrace(_ viewName: String? = nil) -> some View {
-        return SentryTracedView(viewName) {
+    func sentryTrace(_ viewName: String? = nil, waitForFullDisplay: Bool? = nil) -> some View {
+        return SentryTracedView(viewName, waitForFullDisplay: waitForFullDisplay) {
             return self
         }
     }

Are you willing to submit a PR?

Yes

@kahest
Copy link
Member

kahest commented Feb 3, 2025

This should be covered in https://github.com/getsentry/sentry-cocoa/pull/4596/files :)
Please note that work on the feature #4162 is still in progress.

Closing this, feel free to comment if I misunderstood, or follow up on the other issue

@kahest kahest closed this as completed Feb 3, 2025
@github-project-automation github-project-automation bot moved this from Needs Discussion to Done in Mobile SDKs Feb 3, 2025
@kahest
Copy link
Member

kahest commented Feb 3, 2025

I realize I misread - this is requesting waitForFullDisplay for sentryTrace. Before I reopen, can you @matthewreimer clarify what your use case is for this, other than symmetry?

@matthewreimer
Copy link
Contributor Author

@kahest Our use case is that in order to keep our code easier to read, we use .sentryTrace() instead of SentryTracedView(), because we can tuck .sentryTrace("ourviewname") down near the end of the view body rather than it having to be right at the top of the view body. Also, doing so reduces the indentation level a bit.

In other words, instead of this:

SentryTracedView("My Awesome Screen") {
    VStack {
        // The part of your content you want to measure
    }
}

we prefer to do this:

VStack {
    // The part of your content you want to measure
    // This makes the substance of the view more prominent than if it's nested
}
.sentryTrace("some view name")

@kahest kahest reopened this Feb 3, 2025
@kahest kahest moved this from Done to Needs Discussion in Mobile SDKs Feb 3, 2025
@kahest
Copy link
Member

kahest commented Feb 3, 2025

@matthewreimer got it, we'll discuss if we can add this

@philprime
Copy link
Contributor

@matthewreimer after discussing this internally, we believe your solution brainstorm should work. Can you please open up a PR with the proposed changes? We are happy to review it.

@philprime philprime moved this from Needs Discussion to Todo in Mobile SDKs Feb 5, 2025
matthewreimer added a commit to matthewreimer/sentry-cocoa that referenced this issue Feb 5, 2025
Add a waitForFullDisplay parameter to the sentryTrace view modifier
on platforms that support it, for parity with SentryTracedView.

Fixes getsentry#4787.
@matthewreimer
Copy link
Contributor Author

@philprime Here you go: #4797

@github-project-automation github-project-automation bot moved this from Todo to Done in Mobile SDKs Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants