-
Notifications
You must be signed in to change notification settings - Fork 87
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
Remaining time in ad break duration #640
Conversation
src/ts/stringutils.ts
Outdated
let scheduledAds = player.ads.getActiveAdBreak().ads; | ||
let durations: number[] = []; | ||
let activeAd = player.ads.getActiveAd(); | ||
let indexOfActiveAd: number = 0; | ||
|
||
for(let i=0; i<scheduledAds.length; i++) { | ||
let iAd = scheduledAds[i]; | ||
if(iAd.isLinear) { | ||
durations.push((iAd as LinearAd).duration); | ||
|
||
if(iAd.id == activeAd.id) { | ||
indexOfActiveAd = i; | ||
} | ||
} | ||
} | ||
// Compute duration of ads to be played (incl active ad) | ||
let duration = durations.slice(indexOfActiveAd).reduce((prev, current) => prev + current, 0); |
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 could be simplified like this (unless I'm missing something):
let scheduledAds = player.ads.getActiveAdBreak().ads; | |
let durations: number[] = []; | |
let activeAd = player.ads.getActiveAd(); | |
let indexOfActiveAd: number = 0; | |
for(let i=0; i<scheduledAds.length; i++) { | |
let iAd = scheduledAds[i]; | |
if(iAd.isLinear) { | |
durations.push((iAd as LinearAd).duration); | |
if(iAd.id == activeAd.id) { | |
indexOfActiveAd = i; | |
} | |
} | |
} | |
// Compute duration of ads to be played (incl active ad) | |
let duration = durations.slice(indexOfActiveAd).reduce((prev, current) => prev + current, 0); | |
const duration = player.ads.getActiveAdBreak().ads | |
.map(ad => ad.duration) | |
.reduce((prev, current) => prev + current, 0); |
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.
thanks @dweinber . I've simplified the code per your guidance.
@saravanans-github please do not alter the PR Checklist - it should stay as-is, because it contains the formal PR requirements. Your changes should be described separately. Please re-add it, and add a Changelog entry :) |
thank you @felix-hoc; I've made the change to the PR checklist. Kindly see if that suffices please? |
@saravanans-github the CI is still failing. Also, your branch is quite a bit behind Also, you're still missing the changelog entry |
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 code looks good, just some nit picks from my side.
Please ensure:
- your branch is up-to-date with
main
- there are no conflicts
- the CI passes and the code is properly formatted (your IDE should highlight formatting errors that are identified by TSLint and offer to auto-fix them)
- you add a changelog entry
src/ts/stringutils.ts
Outdated
const duration = player.ads.getActiveAdBreak().ads.slice(indexOfActiveAd) | ||
.map(ad => { | ||
if(ad.isLinear) { | ||
return (ad as LinearAd).duration | ||
} else { | ||
return 0 | ||
} | ||
}) | ||
.reduce((prev, current) => prev + current, 0); |
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.
nit: this could be simplified to use a single reduce
:
const duration = player.ads.getActiveAdBreak().ads.slice(indexOfActiveAd) | |
.map(ad => { | |
if(ad.isLinear) { | |
return (ad as LinearAd).duration | |
} else { | |
return 0 | |
} | |
}) | |
.reduce((prev, current) => prev + current, 0); | |
const duration = player.ads.getActiveAdBreak().ads | |
.slice(indexOfActiveAd) | |
.reduce((total, ad) => total + (ad.isLinear ? (ad as LinearAd).duration : 0), 0); |
src/ts/stringutils.ts
Outdated
@@ -112,7 +114,28 @@ export namespace StringUtils { | |||
time = player.getCurrentTime(); | |||
} else if (formatString.indexOf('adDuration') > -1) { | |||
time = player.getDuration(); | |||
} else if (formatString.indexOf('adBreakRemainingTime') > -1) { // To display the remaining time in the ad bread as opposed to in the ad |
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.
typo
} else if (formatString.indexOf('adBreakRemainingTime') > -1) { // To display the remaining time in the ad bread as opposed to in the ad | |
} else if (formatString.indexOf('adBreakRemainingTime') > -1) { // To display the remaining time in the ad break as opposed to in the ad |
…reakDuration ensuring branch is up-to-date with develop
Co-authored-by: Felix Hochgruber <felix.hochgruber@bitmovin.com>
Co-authored-by: Felix Hochgruber <felix.hochgruber@bitmovin.com>
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.
Very nice! 😃
Description
added support for a new adMessagePlaceholder -
adBreakRemainingTime
. This placeholder displays the remaining time in an ad break.Checklist (for PR submitter and reviewers)
CHANGELOG
entry