Skip to content

Conversation

@mmustafa-tse
Copy link
Contributor

@mmustafa-tse mmustafa-tse commented May 14, 2025

Summary

Added the following to Roku SDK's Media API

  • initializing and calculating the mediaSessionStartTime and mediaSessionEndTime
  • added storedPlaybackTime and currentPlaybackStartTimestamp
  • update the mediaSessionEndTime on every media session event logged (similar to Web, Android and iOS media SDKs)
  • update mediaContentComplete to true when logMediaContentEnd is called
  • calculating the mediaContentTimeSpent when logPause and logMediaContentEnd are called

Testing Plan

  • Was this tested locally? If not, explain why.
  • E2E tested
  • Before fix:
Screenshot 2025-05-16 at 10 49 57 AM - After fix: Screenshot 2025-05-16 at 10 50 14 AM

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

@BrandonStalnaker BrandonStalnaker self-requested a review May 15, 2025 18:32
Copy link
Contributor

@BrandonStalnaker BrandonStalnaker left a 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()
Copy link
Contributor

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?

Copy link
Contributor Author

@mmustafa-tse mmustafa-tse May 16, 2025

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

Copy link
Contributor

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?

Copy link
Member

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?

Copy link
Contributor

@BrandonStalnaker BrandonStalnaker left a comment

Choose a reason for hiding this comment

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

LGTM

@rmi22186 rmi22186 merged commit 7e03abf into mParticle:master May 21, 2025
1 check passed
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