[go: up one dir, main page]

Skip to content
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: 'overrides' SUPERFLY-2 #5

Open
wants to merge 158 commits into
base: upstream-release51
Choose a base branch
from

Conversation

Julusian
Copy link
Collaborator
@Julusian Julusian commented Apr 16, 2024

About the Contributor

Type of Contribution

This is a Feature

New Behavior

This lays the groundwork for 'overrides'.

As far as I am aware, the backend portion of this is implemented, and works.
Large portions of the ingest flow have been rewritten/refactored to achieve this, which has resulted in this being a very large change.

Currently, this exposes a user action for the user facing operations, but no ui to trigger it


Note: naming of the feature and methods is not final, and might still be changed.

The core of this feature is that blueprints have a new method that they can optionally implement called processIngestData.
This method serves a few purposes:

  1. It lets the blueprints perform some pre-processing of the IngestRundown. It can split/merge/invent Parts or Segments, or copy data between them without worrying about impacting ingest performance. Changes are tracked to minimise what needs to be run through getSegment.
    As part of this, it is now the responsibility of this method to ensure that ingest changes are pulled into the IngestRundown correctly. There is a method context.defaultApplyIngestChanges which can do this for you, or you can customise the behaviour by doing something different.
  2. It lets the blueprint apply some 'user operation'/'user edit' onto the IngestRundown. How this gets done and persisted is entirely up to the blueprint. It needs to figure that out, and figure out persistence when other ingest changes are made (if persistence beyond that is even wanted).

As a side effect of this, the grouping of MOS stories into segments has been rewritten to be another method that can be used inside of processIngestData. If implementing the method, the mos grouping will have to be applied manually when desired.

If processIngestData is not implemented, core uses the default implementation of, which will match previous behaviour:

export async function processIngestData(
	context: IProcessIngestDataContext,
	mutableIngestRundown: MutableIngestRundown<any, any, any>,
	nrcsIngestRundown: IngestRundown,
	previousNrcsIngestRundown: IngestRundown | undefined,
	ingestChanges: NrcsIngestChangeDetails | UserOperationChange<any>
): Promise<void> {
	if (ingestChanges.source === 'ingest') {
		// Match default grouping, this can be customized to group parts differently
		const groupedIngestRundown = context.groupMosPartsInRundownAndChangesWithSeparator(
			nrcsIngestRundown,
			previousNrcsIngestRundown,
			ingestChanges,
			';' // Match the default separator
		)

		// Standard apply ingest changes
		context.defaultApplyIngestChanges(
			mutableIngestRundown,
			groupedIngestRundown.nrcsIngestRundown,
			groupedIngestRundown.ingestChanges
		)
	} else {
		// TODO: handle user operation
	}
}

The user operations are triggered by a new userAction executeUserChangeOperation. Where this is used and the exact payload is not very well defined yet, that will be done as part of figuring out the ui design.

Testing Instructions

Time Frame

Other Information

This should be squashed when merging.
When contributing upstream, it will need to be broken up into multiple smaller PRs so that each chunk can be reviewed and merged independently. I have a rough plan on how it can be broken up for that

There is an example crude simple ui prototyped at feat/blueprint-processIngestData...bbc:sofie-core:wip/user-overrides-ui
This adds some context menu options for parts and segments:
image

And a form driven one:
Screenshot from 2024-04-19 14-19-42
Screenshot from 2024-04-19 14-19-55

Status

  • PR is ready to be reviewed.
  • The functionality has been tested by the author.
  • Relevant unit tests has been added / updated.
  • Relevant documentation (code comments, system documentation) has been added / updated.

@Julusian Julusian changed the base branch from release51 to upstream-release51 September 20, 2024 10:44
Copy link
Collaborator Author
@Julusian Julusian left a comment

Choose a reason for hiding this comment

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

I am very unsure about what the intended flow is for these userEditStates properties which are everywhere.
This is likely because it looks like the ui isn't complete in this, so I don't have a good grasp on how they are intended to be used
Should it be more strongly typed? I see that the mute segment feature has the ui looking for a specific key. Or will this be used with some 'looser' matching that will allow any string to be used?
Perhaps we should talk through this to help me understand.

A bunch of places are using CoreUserEditingDefinitionAction instead of CoreUserEditingDefinition, which has resulted in needing a lot of casting to make the types 'match', even though those CoreUserEditingDefinitionAction could also be CoreUserEditingDefinitionForm which now isnt reflected in the types.

@@ -385,6 +385,8 @@ export class PartAndPieceInstanceActionService {
invalidReason: undefined,
floated: false,
expectedDurationWithTransition: undefined, // Filled in later
userEditStates: {},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should this be removing whatever the blueprints tried to set when queuing this part?
I suspect this might be intentional, if so a comment to justify it would be good so that someone doesnt change it thinking this is a bug later on.

packages/corelib/src/dataModel/IngestDataCache.ts Outdated Show resolved Hide resolved
packages/corelib/src/snapshots.ts Outdated Show resolved Hide resolved
packages/corelib/src/dataModel/Piece.ts Outdated Show resolved Hide resolved
packages/job-worker/src/blueprints/postProcess.ts Outdated Show resolved Hide resolved
packages/job-worker/src/ingest/generationSegment.ts Outdated Show resolved Hide resolved
packages/job-worker/src/ingest/generationSegment.ts Outdated Show resolved Hide resolved
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.

5 participants