-
Notifications
You must be signed in to change notification settings - Fork 5
feat: Add Media Session Timestamps and Media Sessions time tracking #40
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
feat: Add Media Session Timestamps and Media Sessions time tracking #40
Conversation
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.
Just one logic question
| date = CreateObject("roDateTime") | ||
| currentTime = CreateObject("roLongInteger") | ||
| currentTime.SetLongInt(date.asSeconds()) | ||
| return (currentTime * 1000) + date.getMilliseconds() |
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.
Can you explain to me the logic here? It seems like your getting the timestamp of the current date in ms two ways and then adding it together? Is that right or am I misunderstanding?
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.
the reason is because with Brightscript there is no way to get the unix timestamp with milliseconds included, only app to seconds, I wanted to match the other media SDKs (and mP in general when dealing with timestamps) to include the unix timestamps including the milliseconds
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.
oh so the (currentTime * 1000) is like the beginning of the day (like midnight) in ms while date.getMilliseconds() gives you like thats days ms?
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.
@mmustafa-tse - you are caling the same code 3x throughout this commit. can you make a helper function for getCurrentTimeInMillis which contains this logic to DRY it up?
BrandonStalnaker
left a comment
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.
LGTM
Summary
Added the following to Roku SDK's Media API
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)