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

Bugfix: Making it possible to set extra properties for AV/Media heartbeats #3

Merged

Conversation

felix-ulmer
Copy link
Contributor

We had the problem with the SDK before that all our heartbeat events were missing all of the av insights standard + custom properties like the av_content_x - Properties.

This PR is fixing this by:

  • Adding a field to the Media - class to store the extraProps - map + making it accessible.
  • Using that field in the runnables for the heartbeats in AVRunnable to pass the extraProps instead of null as it was before.

@DeKaN
Copy link
Contributor

DeKaN commented May 29, 2023

Hi @felix-ulmer, we're planning to release develop branch as the next version. I recommend using this branch as target for the PR

@felix-ulmer felix-ulmer changed the base branch from main to develop May 30, 2023 06:21
@DeKaN
Copy link
Contributor

DeKaN commented May 31, 2023

@felix-ulmer Due to global changes in the develop branch simple merging is not enough for this PR

@felix-ulmer felix-ulmer force-pushed the bugfix/extra_props_for_heartbeats branch from c5255bc to 5df7efe Compare May 31, 2023 13:32
…ps_for_heartbeats

# Conflicts:
#	build.gradle.kts
#	piano-analytics/build.gradle
#	piano-analytics/build.gradle.kts
#	piano-analytics/src/main/java/io/piano/analytics/ConfigurationStep.java
#	piano-analytics/src/main/java/io/piano/analytics/InternalContextPropertiesStep.java
#	piano-analytics/src/main/java/io/piano/analytics/PrivacyStep.java
#	piano-analytics/src/main/java/io/piano/analytics/avinsights/AVRunnable.java
#	piano-analytics/src/main/java/io/piano/analytics/avinsights/Media.java
#	piano-analytics/src/test/java/io/piano/analytics/BuildStepTest.java
#	piano-analytics/src/test/java/io/piano/analytics/InternalContextPropertiesStepAPI16Test.java
#	piano-analytics/src/test/java/io/piano/analytics/TestResources.java
#	settings.gradle.kts
@felix-ulmer
Copy link
Contributor Author

@felix-ulmer Due to global changes in the develop branch simple merging is not enough for this PR

Yes, I saw that today unfortunately! :( I'm currently redoing the changes based on the new architecture of the develop branch + Kotlin.

*/
@Suppress("unused", "MemberVisibilityCanBePrivate") // Public API.
fun setExtraProps(extraProps: Array<out Property>) = apply {
this.extraProps = extraProps
Copy link
Contributor

Choose a reason for hiding this comment

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

Array values can be modified outside, it's better to make a copy here via extraProps.copyOf()

* @return this [MediaHelper] instance
*/
@Suppress("unused", "MemberVisibilityCanBePrivate") // Public API.
fun setExtraProps(extraProps: Array<out Property>) = apply {
Copy link
Contributor

Choose a reason for hiding this comment

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

May be we should add fun setExtraProps(vararg extraProps: Property) = setExtraProps(extraProps) also. What do you think?

@DeKaN DeKaN merged commit 7ffcba3 into at-internet:develop Jun 5, 2023
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.

3 participants