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

chore: add support to set screen name to all event #181

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

thisames
Copy link
Contributor

💡 Motivation and Context

#119

💚 How did you test it?

The tests have been updated to include checks for the new $screen_name property in any event.
I tested it manually and analyse the props in dashboard.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

@@ -68,6 +68,8 @@ public class PostHog private constructor(
private var isIdentifiedLoaded: Boolean = false
private var isPersonProcessingLoaded: Boolean = false

private lateinit var currentScreen: String
Copy link
Member

Choose a reason for hiding this comment

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

I think this approach leaves room for too many false positives.
eg I call screen(x) for screen X, but I don't call screen(y) for screen Y, when events are captured in the screen y, the screen name is x.

Copy link
Member

@marandaneto marandaneto Sep 23, 2024

Choose a reason for hiding this comment

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

I think a better approach would be using https://github.com/PostHog/posthog-android/blob/main/posthog-android/src/main/java/com/posthog/android/internal/PostHogActivityLifecycleCallbackIntegration.kt
So we can cache the current Activity based on the screen's lifecycle and infer the name from it.
If screen is called either manually, it has precedence over the current activity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, you are correct. The problem with using PostHogActivityLifecycleCallbackIntegration.kt is that we would have to add method signature in PostHogInterface, and i dont know if it make sense.

see theses changes
thisames@7d4a6f5

do you think this make sense?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should add a new concept to the SDK that allows enriching the event after being captured, for example, an event processor.
I will make an example and you can expand from there

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, you made it much easier for me. I will update this PR with the changes and then you check and give me the ok, if it works I will do the unit tests

Copy link
Member

Choose a reason for hiding this comment

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

Another option is to use the super properties for the screen name

public override fun register(
key: String,
value: Any,
) {
if (!isEnabled()) {
return
}
if (ALL_INTERNAL_KEYS.contains(key)) {
config?.logger?.log("Key: $key is reserved for internal use.")
return
}
getPreferences().setValue(key, value)
}
public override fun unregister(key: String) {
if (!isEnabled()) {
return
}
getPreferences().remove(key)
}

both would work the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer the solution with processors, it's working well

@marandaneto marandaneto marked this pull request as draft September 25, 2024 13:07
@thisames thisames marked this pull request as ready for review November 28, 2024 10:06
@thisames
Copy link
Contributor Author

thisames commented Dec 3, 2024

@marandaneto ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants