fix: SOF-769 make `direkte` idents behave like other idents #139

ianshade · 2022-08-30T11:39:53Z

Removes some sticky ident logic because they are supposed to be brought back only when recalling Last Live, which being an adlib action, is capable of finding the associated ident as well - no need for extra timelineObject enable & class rules. Also stops within-part graphics in the CutToCamera action to make it work and look more like a Part would.

LindvedKrvang

Lots of deleted code. I can only approve of that ;)

Nice to see the refactor into smaller functions :)

src/tv2-common/actions/executeAction.ts

src/tv2-common/helpers/graphics/InternalGraphic.ts

src/tv2-common/helpers/graphics/internal/index.ts

LindvedKrvang · 2022-08-31T05:28:16Z

src/tv2-common/helpers/graphics/timing.ts

@@ -68,46 +69,27 @@ export function FindInfiniteModeFromConfig(

 export function GetGraphicDuration(
 	config: TV2BlueprintConfig,
-	cue: CueDefinitionGraphic<GraphicInternalOrPilot>,
-	defaultTime: boolean
+	cue: CueDefinitionGraphic<GraphicInternalOrPilot>
 ): number | undefined {
 	if (config.showStyle.GFXTemplates) {


Maybe invert the if, so you get something like this to avoid the nesting?

if (!config.showStyle.GFXTemplates) { return GetDefaultOut(config) } const template = findGFXTemplate(config, cue) return template && template.OutType && !template.OutType.toString().match(/default/i)) ? undefined : GetDefaultOut(config)

src/tv2-common/onTimelineGenerate.ts

LindvedKrvang · 2022-08-31T05:42:26Z

src/tv2-common/updatePolicies/shouldRemoveOrphanedPartInstance.ts

@@ -5,5 +5,5 @@ export function shouldRemoveOrphanedPartInstance(
 	_context: IRundownUserContext,
 	partInstance: BlueprintRemoveOrphanedPartInstance
 ): boolean {
-	return !(partInstance.partInstance.part.metaData as PartMetaData).dirty
+	return !(partInstance.partInstance.part.metaData as PartMetaData | undefined)?.dirty


We should strive to not let our types be undefined as it forces us to make undefined check later in the code.
In this case the 'later in the code' part is two spaces right after the declaration of undefined.

But since Sofie is Sofie I assume that this metaData is able to already be undefined when it gets here and that's why you added the undefined type.

metaData is transparent to Core (it is whatever the blueprints put in there, if they want, because it is optional). So it should be only in the functions exported through the blueprints manifest, that get that data back, where we have to cast to the type we know we (might have) put there. Optional values are useful, you just have to be careful when casting 'early in the code'.

LindvedKrvang · 2022-08-31T05:44:44Z

src/tv2_afvd_showstyle/migrations/index.ts

@@ -119,7 +119,7 @@ export const showStyleMigrations: MigrationStepShowStyle[] = literal<MigrationSt
 	 */
 	// OVERLAY group
 	SetSourceLayerName('1.6.9', SourceLayer.PgmGraphicsIdent, 'GFX Ident'),
-	SetSourceLayerName('1.6.9', SourceLayer.PgmGraphicsIdentPersistent, 'GFX Ident Persistent (hidden)'),
+	SetSourceLayerName('1.6.9', 'studio0_graphicsIdent_persistent', 'GFX Ident Persistent (hidden)'),


We should consider making this change for the rest of the enum dependent migrations in here

… types and other small refactors

ianshade · 2022-09-06T07:40:28Z

I refactored a bit more than expected, but I wanted to have a bit better types on pieces and their metadata, remove/replace some literal calls and remove some sketchy assertions etc.

LindvedKrvang

Big fan of getting rid of unknown types etc :)

Not a fan of the refactor from const variable: T[] to const variable: Array<T>.
Unless there is a technical difference between the two (as far as I am aware, there is not), then it comes down to personal preference/team agreement. If we make a tream agreement, then I vote for const variable: T[] (even if we don't make a team agreement I vote for this ;) )

You removed literal<T> a lot of places and left them other places. Out of curiosity, what determines when to use them and when not to?

LindvedKrvang · 2022-09-07T10:11:58Z

src/__mocks__/context.ts

 	public currentPart: IBlueprintPartInstance
-	public currentPieceInstances: IBlueprintPieceInstance[]
+	public currentPieceInstances: Array<IBlueprintPieceInstance<PieceMetaData>>


Why not use the [] syntax?
I would prefer that we used that syntax :)

I would prefer them as well, but the current linting rules in this package enforce Array<...<...>> instead. We should start using the Sofie preset here - I think it allows ...<...>[].

There are some things in the preset I'm not that big of a fan off. That being said I won't make too big of a fuss if we use it.
But maybe just start with chaning our linting rules to allow ...<...>[]?

LindvedKrvang · 2022-09-07T10:14:58Z

src/__mocks__/context.ts


 	public takeAfterExecute: boolean = false
+	public isTV2Context: true = true


I think either change the return type to boolean or delete this. If it can only be true then there is never a need to check for it :)

Also, I don't seem to be able to spot where it is used, so I half assumed this is leftover from some testing? Or is there a hidden dependency I am missing?

I think this was used to make IActionExecutionContext not pass as ITV2ActionExecutionContext, maybe, but I think all our functions should just use TV2ActionExecutionContext instead of ITV2ActionExecutionContext, because it's not like we have anything else implementing ITV2ActionExecutionContext.

LindvedKrvang · 2022-09-07T10:19:45Z

src/inews-mixins/playlist.ts

+		let result: BlueprintResultRundownPlaylist = (getRundownPlaylistInfo
+			? getRundownPlaylistInfo(context, rundowns, '')
+			: undefined) ?? {
+			playlist: literal<IBlueprintResultRundownPlaylist>({


Carefull with the nested ternaries and coalescing operators. This is starting to be messy to look at.
Also, I think the indentation is slightly off here. It should be:

: undefined) ?? { playlist: {...}

not

: undefined) ?? { playlist: {...}

ianshade · 2022-09-07T11:47:09Z

@LindvedKrvang when considering literal<T>, if you're providing object literals of the exact type declared by whatever takes them, it is fine to omit it. For example when:

a function declares some return type, you don't have to use literal to repeat that type in the return statement
a function takes an argument of some type, you don't have to explicitly state that type when passing the argument
initializing variables, it is shorter to put the type directly on the variable declaration instead of getting it inferred from the literal

If you're return type / argument / variable is a Discriminated Union, TypeScript will also just figure the right type out, but it might be argued that using literal in this case makes it easier for the reader to figure it out. And it might make sense in some other cases, like picking the type from unions that are not discriminated or interfaces that extend others, like with our piece metadata types (which is perhaps not ideal and would make more sense to be discriminated and have stronger checking based on that, instead of assuming a DVE piece certainly has DVEPieceMetaData, but that's probably for another Friday evening refactoring session).

LindvedKrvang · 2022-09-09T07:58:46Z

@LindvedKrvang when considering literal<T>, if you're providing object literals of the exact type declared by whatever takes them, it is fine to omit it. For example when:

a function declares some return type, you don't have to use literal to repeat that type in the return statement

a function takes an argument of some type, you don't have to explicitly state that type when passing the argument

initializing variables, it is shorter to put the type directly on the variable declaration instead of getting it inferred from the literal

If you're return type / argument / variable is a Discriminated Union, TypeScript will also just figure the right type out, but it might be argued that using literal in this case makes it easier for the reader to figure it out. And it might make sense in some other cases, like picking the type from unions that are not discriminated or interfaces that extend others, like with our piece metadata types (which is perhaps not ideal and would make more sense to be discriminated and have stronger checking based on that, instead of assuming a DVE piece certainly has DVEPieceMetaData, but that's probably for another Friday evening refactoring session).

Coming from Java, I am fond of having the type stated close to declaration of variables etc, so when literal<T> is the only thing telling me what type it is in the file I'm looking at, I think it's fine for them to be there.
But I don't really have any strong opinions about them being there or not. I just want to be able to quickly go to the interface declaration :)

ianshade · 2022-09-14T07:28:03Z

@LindvedKrvang there is a new operator coming to TypeScript, and it will let us replace some (if not all) remaining literal<T>, and microsoft/TypeScript#46827 😃

fix: SOF-769 make direkte idents behave like other idents

2eb9f06

ianshade requested review from KvelaGorrrrnio and LindvedKrvang August 30, 2022 11:39

LindvedKrvang approved these changes Aug 31, 2022

View reviewed changes

KvelaGorrrrnio approved these changes Sep 1, 2022

View reviewed changes

refactor: SOF-769 remove some assertions and literals; improve return…

21fd0aa

… types and other small refactors

ianshade requested a review from LindvedKrvang September 6, 2022 07:37

LindvedKrvang approved these changes Sep 7, 2022

View reviewed changes

ianshade merged commit d91aea7 into staging Sep 14, 2022

ianshade deleted the SOF-769/fixDirekteInAllParts branch October 25, 2022 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: SOF-769 make `direkte` idents behave like other idents #139

fix: SOF-769 make `direkte` idents behave like other idents #139

ianshade commented Aug 30, 2022

LindvedKrvang left a comment

LindvedKrvang Aug 31, 2022

LindvedKrvang Aug 31, 2022

ianshade Sep 2, 2022 •

edited

LindvedKrvang Aug 31, 2022

ianshade commented Sep 6, 2022

LindvedKrvang left a comment

LindvedKrvang Sep 7, 2022

ianshade Sep 7, 2022

LindvedKrvang Sep 9, 2022

LindvedKrvang Sep 7, 2022

ianshade Sep 7, 2022

LindvedKrvang Sep 9, 2022

LindvedKrvang Sep 7, 2022

ianshade commented Sep 7, 2022 •

edited

LindvedKrvang commented Sep 9, 2022

ianshade commented Sep 14, 2022


		public takeAfterExecute: boolean = false
		public isTV2Context: true = true

fix: SOF-769 make direkte idents behave like other idents #139

fix: SOF-769 make direkte idents behave like other idents #139

Conversation

ianshade commented Aug 30, 2022

LindvedKrvang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianshade Sep 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianshade commented Sep 6, 2022

LindvedKrvang 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

ianshade commented Sep 7, 2022 • edited

LindvedKrvang commented Sep 9, 2022

ianshade commented Sep 14, 2022

fix: SOF-769 make `direkte` idents behave like other idents #139

fix: SOF-769 make `direkte` idents behave like other idents #139

ianshade Sep 2, 2022 •

edited

ianshade commented Sep 7, 2022 •

edited