-
Notifications
You must be signed in to change notification settings - Fork 4
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
Support additional standard device and content tags #56
Support additional standard device and content tags #56
Conversation
metadata.setAdditionalStandardTags(newValue); | ||
} | ||
|
||
public Map<String, Object> getAdditionalStandardTags() { |
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.
This function is is basically the same as getCustom()
customInternTags.put("contentType", "Episode"); | ||
metadata.setCustom(customInternTags); | ||
|
||
Map<String, Object> standardTags = new HashMap<>(); |
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.
Shouldn't this be Map<String, String>
as well?
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.
If the setContentInfo
API accepts Map<String, Object>
I would rather consider changing customTags
as well to not take away a feature from the Conviva SDK 🤔
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.
This is the data type that setContentInfo accepts, so I didn't want to limit the customer: https://pulse.conviva.com/learning-center/content/sensor_developer_center/sensor_integration/android/android_stream_sensor.htm#:~:text=To%20update%20or%20amend%20pre%2Ddefined,metadata%20tags%20for%20video%20content.
For the custom tags, I left the type as is, because the ContentMetadata
class (that I assume MetadataOverrides is based on) has it defined as Map<String, String>. We can change that if needed in the future IMO.
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, didn't realize Conviva's reportPlaybackRequested
and setContentInfo
APIs takes Map<String, Object>
. In this case I agree, we should change the handling of custom tags to also take an object. But no need to do it within this PR.
@@ -14,6 +14,7 @@ public class MetadataOverrides { | |||
private String applicationName; | |||
private Map<String, String> custom; | |||
private Integer duration; | |||
private Map<String, Object> additionalStandardTags; |
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.
Shouldn't this be Map<String, String>
? Does Conviva even support us sending objects?
Conviva has a list of predefined standard device and content tags that this integration currently doesn't support reporting.
Changes
MetadataOverrides.setAdditionalStandardTags
that allows to set additional standard tags for the session.