feat: Budget Duration #556

ianshade · 2021-08-17T14:12:46Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
A feature called Budget Duration
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Parts have a new optional property called budgetDuration.
In a Segment that has any Part with budgetDuration set, an Editorial Marker will be displayed on the segment timeline, as a dashed vertical line, indicating the budget duration of the entire Segment (sum of budgetDurations of its Parts).
In said Segment, the SegmentDuration component will display the segment's budget duration, and count down from it when the segment becomes live. The duration will go negative when the segment is playing longer than its budget.
If a segment with budget duration is on air, Part Counters on the next Segments are based on budget duration of the current segment, instead of expected durations of the parts that are left.
Other information:

Status

Code documentation for the relevant parts in the code have been added/updated by the PR author
The functionality has been tested by the PR author
The functionality has been tested by NRK

When a segment is past the budget, the planned start of the next segment should be now

…get-duration

codecov-commenter · 2021-08-17T14:14:20Z

Codecov Report

Merging #556 (7dd3330) into release38 (124af32) will increase coverage by 15.06%.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           release38     #556       +/-   ##
==============================================
+ Coverage      67.39%   82.46%   +15.06%     
==============================================
  Files            264       12      -252     
  Lines          18374     1015    -17359     
  Branches        4083      210     -3873     
==============================================
- Hits           12384      837    -11547     
+ Misses          5949      177     -5772     
+ Partials          41        1       -40

Impacted Files	Coverage Δ
meteor/lib/rundown/rundownTiming.ts
meteor/lib/collections/Parts.ts
meteor/server/api/blueprints/context/lib.ts
meteor/lib/collections/AdLibPieces.ts
meteor/lib/collections/PackageInfos.ts
meteor/server/security/organization.ts
meteor/lib/collections/RundownPlaylists.ts
...client/ui/RundownList/DisplayFormattedTimeInner.ts
meteor/server/api/ingest/commit.ts
meteor/lib/check.ts
... and 242 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 124af32...7dd3330. Read the comment docs.

prevents issus in a future merge with tv2:feat/rundown-view-customization

meteor/lib/collections/PartInstances.ts

nytamin · 2021-09-08T06:48:38Z

It would be good if another unit test be added into meteor/lib/rundown/__tests__/rundownTiming.test.ts, to support this functionality.

probably a bad merge from times when `findPartInstanceInMapOrWrapToTemporary` did not exist

ianshade · 2021-09-08T11:22:29Z

I added a test case. Note: this feature currently affects only the partCountdown. It does not affect remainingPlaylistDuration, totalPlaylistDuration, etc. but it's subject to change in future PRs

jstarpl

I'm really confused by some of the implementation & some parts can be definitely implemented in a more straightforward fashion & reducing the amount of inter-dependency between the components.

jstarpl · 2021-09-17T05:44:21Z

meteor/client/ui/RundownView/RundownTiming/SegmentDuration.tsx

@@ -6,6 +6,8 @@ import { PartUi } from '../../SegmentTimeline/SegmentTimelineContainer'

 interface ISegmentDurationProps {
 	parts: PartUi[]
+	budgetDuration?: number
+	playedOutDuration: number


What this is used for definitely needs a comment. I would expect SegmentDuration component to figure out the playedOutDuration of given parts on it's own, so maybe this should be called playedOutDurationOverride or something like that? Same for budgetDuration, since calculating the budget was also something that used to be only done internally.

Removed those properties in favor of timingDurations

jstarpl · 2021-09-17T05:54:14Z

meteor/client/ui/SegmentTimeline/SegmentTimeline.tsx

+						isLastInSegment={false}
+						isAfterLastValidInSegmentAndItsLive={false}
+						isBudgetGap={true}
+						part={{


As is, this will create a new part object for every render of the component, even though this object is essentially constant through the lifetime of the component. This needs to be converted to a constant that gets reused on every render.

Done in a7a9c6c

jstarpl · 2021-09-17T06:01:00Z

meteor/client/ui/SegmentTimeline/SegmentTimelinePart.tsx

 	onContextMenu?: (contextMenuContext: IContextMenuContext) => void
 	isLastInSegment: boolean
 	isAfterLastValidInSegmentAndItsLive: boolean
 	isLastSegment: boolean
+	isBudgetGap: boolean


I think this can be made optional. isBudgetGap is specified as true in just one single line. Makes no sense to keep writing isBudgetGap=false all the time.

Done in b46f8ae

jstarpl · 2021-09-17T06:01:53Z

meteor/client/ui/SegmentTimeline/SourceLayerItem.tsx

@@ -421,11 +423,11 @@ export const SourceLayerItem = withTranslation()(
 		}

 		private mountResizeObserver() {
-			if (this.props.isLiveLine && !this._resizeObserver && this.state.itemElement) {
+			if (this.props.isLiveLine && !this._resizeObserver && this.itemElement) {


Why isn't this using the state anymore? Using state is definitely safer.

Storing refs in state doesn't play well with componentDidMount, where we're using this ref

jstarpl · 2021-09-17T07:40:28Z

meteor/client/ui/SegmentTimeline/SegmentTimelineContainer.tsx

@@ -615,6 +622,21 @@ export const SegmentTimelineContainer = translateWithTracker<IProps, IState, ITr
 			window.removeEventListener('resize', this.onWindowResize)
 		}

+		private getSegmentBudgetDuration(): number | undefined {


I'm somewhat confused. So RundownTimingCalculator seems to already be doing almost the same thing, and yet then the Segment component seems to be doing it again? And then it feels like it only does that to pass that information forward to components that have access to the timing context anyway, so they could just get it from the Calculator. Or am I missing something?

SegmentTimelineContainer's tracker seems to be a better place to do it, which adresses #556 (comment) as well.
Why can't the value come from RundownTimingCalculator? It could but we would need to do an extra comparison every time SegmentTimelineClass receives its HR time event.

jstarpl · 2021-09-17T07:42:12Z

meteor/client/ui/SegmentTimeline/SegmentTimelineContainer.tsx

@@ -794,11 +816,14 @@ export const SegmentTimelineContainer = translateWithTracker<IProps, IState, ITr
 						? partOffset + e.detail.currentTime - virtualStartedPlayback + lastTakeOffset
 						: partOffset + lastTakeOffset

+				const budgetDuration = this.getSegmentBudgetDuration()


Why do this in the onAirLineRefresh()? This is going to happen on every tick, yet getSegmentBudgetDuration() only depends on semi-constant budgetDuration on the PartInstance. Wouldn't it make more sense to calculate that reactively in the tracker, which already depends on those PartInstances, so it's going to refresh if/when they change?

Moved to the tracker

jstarpl · 2021-09-29T12:09:11Z

@ianshade Can you look into the PR review? I guess you would like this merged into Release 38.

ianshade · 2021-09-30T12:25:28Z

We found an issue causing this to break the fit-to-viewport zoom feature. We're still working on a fix

`segment-timeline__timeline` has now a min-width to make the gap part self-adjust using flex; take measurements from `segment-timeline__timeline-container` instead

…get-duration

jstarpl

I think this looks OK.

ianshade added 5 commits January 26, 2021 17:19

feat: Budget duration

053de3c

fix: Display budget duration in SegmentDuration component

7326cc9

fix: Revert the correct behaviour of PartCountdown

4d4a3dd

When a segment is past the budget, the planned start of the next segment should be now

Merge branch 'release36' into contribute/budget-duration

4826764

Merge remote-tracking branch 'upstream/release37' into contribute/bud…

5f0db5a

…get-duration

nytamin added Contribution from TV 2 Danmark Contributions sponsored by TV 2 Danmark (tv2.dk) ✨ enhancement New feature or request labels Aug 27, 2021

nytamin requested a review from jstarpl August 27, 2021 11:32

chore: move variable initializations

9bc5d36

prevents issus in a future merge with tv2:feat/rundown-view-customization

nytamin reviewed Sep 8, 2021

View reviewed changes

meteor/lib/collections/PartInstances.ts Outdated Show resolved Hide resolved

nytamin added the 👍 Accepted This PR is accepted by the Product Owner label Sep 8, 2021

ianshade added 2 commits September 8, 2021 12:18

chore: fix wrong type

7c125c0

probably a bad merge from times when `findPartInstanceInMapOrWrapToTemporary` did not exist

chore: add budget duration test case

0064632

jstarpl requested changes Sep 17, 2021

View reviewed changes

ianshade added 6 commits October 12, 2021 14:26

fix: show-all using wrong div for measurement

1a54e65

`segment-timeline__timeline` has now a min-width to make the gap part self-adjust using flex; take measurements from `segment-timeline__timeline-container` instead

refactor: move budgetDuration calculation to tracked props

ffd550a

refactor: make isBudgetGap optional

b46f8ae

refactor: SegmentDuration to use timingDurations

37dcf2a

refactor: make budget gap part const

a7a9c6c

Merge remote-tracking branch 'upstream/release37' into contribute/bud…

e7763ae

…get-duration

jstarpl self-requested a review October 13, 2021 08:32

jstarpl approved these changes Oct 13, 2021

View reviewed changes

jstarpl changed the base branch from release37 to release38 October 18, 2021 13:15

jstarpl added 2 commits October 19, 2021 12:28

Merge branch 'release38' into contribute/budget-duration

0e674c9

fix(SegmentTimingPanel): props mismatch

75a7ebd

chore: lint

7dd3330

jstarpl merged commit 4b7627d into nrkno:release38 Oct 19, 2021

mint-dewit mentioned this pull request Dec 17, 2021

Release 38 nrkno/Sofie-TV-automation#39

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Budget Duration #556

feat: Budget Duration #556

ianshade commented Aug 17, 2021 •

edited

codecov-commenter commented Aug 17, 2021 •

edited

nytamin commented Sep 8, 2021

ianshade commented Sep 8, 2021

jstarpl left a comment

jstarpl Sep 17, 2021

ianshade Oct 12, 2021

jstarpl Sep 17, 2021

ianshade Oct 12, 2021

jstarpl Sep 17, 2021

ianshade Oct 12, 2021

jstarpl Sep 17, 2021

ianshade Oct 12, 2021

jstarpl Sep 17, 2021

ianshade Oct 12, 2021

jstarpl Sep 17, 2021 •

edited

ianshade Oct 12, 2021

jstarpl commented Sep 29, 2021

ianshade commented Sep 30, 2021

jstarpl left a comment

feat: Budget Duration #556

feat: Budget Duration #556

Conversation

ianshade commented Aug 17, 2021 • edited

codecov-commenter commented Aug 17, 2021 • edited

Codecov Report

nytamin commented Sep 8, 2021

ianshade commented Sep 8, 2021

jstarpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarpl Sep 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarpl commented Sep 29, 2021

ianshade commented Sep 30, 2021

jstarpl left a comment

Choose a reason for hiding this comment

ianshade commented Aug 17, 2021 •

edited

codecov-commenter commented Aug 17, 2021 •

edited

jstarpl Sep 17, 2021 •

edited