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

pop-out-clock proof of concept #1247

Merged
merged 2 commits into from
Oct 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions apps/client/src/AppRouter.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';

Check failure on line 1 in apps/client/src/AppRouter.tsx

View workflow job for this annotation

GitHub Actions / unit-test

Run autofix to sort these imports!
import {
createRoutesFromChildren,
matchRoutes,
Expand All @@ -16,13 +16,15 @@
import withData from './features/viewers/ViewWrapper';
import { ONTIME_VERSION } from './ONTIME_VERSION';
import { sentryDsn, sentryRecommendedIgnore } from './sentry.config';
import { Playback, TimerPhase, TimerType } from 'ontime-types';

const Editor = React.lazy(() => import('./features/editors/ProtectedEditor'));
const Cuesheet = React.lazy(() => import('./features/cuesheet/ProtectedCuesheet'));
const Operator = React.lazy(() => import('./features/operator/OperatorExport'));

const TimerView = React.lazy(() => import('./features/viewers/timer/Timer'));
const MinimalTimerView = React.lazy(() => import('./features/viewers/minimal-timer/MinimalTimer'));
const PopOutTimer = React.lazy(() => import('./features/viewers/pop-out-clock/PopOutTimer'));
const ClockView = React.lazy(() => import('./features/viewers/clock/Clock'));
const Countdown = React.lazy(() => import('./features/viewers/countdown/Countdown'));

Expand Down Expand Up @@ -73,6 +75,30 @@

return (
<React.Suspense fallback={null}>
<PopOutTimer
isMirrored={false}
time={{
addedTime: 0,
current: null,
duration: null,
elapsed: null,
expectedFinish: null,
finishedAt: null,
phase: TimerPhase.Default,
playback: Playback.Play,
secondaryTimer: null,
startedAt: null,
clock: 0,
timerType: TimerType.CountDown
}}
viewSettings={{
dangerColor: '',
endMessage: '',
freezeEnd: false,
normalColor: '',
overrideStyles: false,
warningColor: ''
}} />
Comment on lines +78 to +101
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refactoring hardcoded values in PopOutTimer props.

While the placement of the PopOutTimer component is correct, there are a few points to consider:

  1. The time prop contains many hardcoded values. This could lead to maintenance issues and might not reflect the actual state of the application.
  2. The viewSettings prop has empty strings for colors and messages.

Given that this is a proof of concept, these issues are understandable. However, for the next iteration, consider:

  • Connecting the time prop to your application's state management system (e.g., Redux, Context API) to reflect real-time values.
  • Implementing a configuration file or environment variables for the viewSettings to make them easily customizable.
  • Adding PropTypes or TypeScript interfaces to define the expected prop types and structures.

Would you like assistance in setting up a more dynamic prop structure for the PopOutTimer component?

<SentryRoutes>
<Route path='/' element={<Navigate to='/timer' />} />
<Route path='/timer' element={<STimer />} />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { hideTimerSeconds } from '../../../common/components/view-params-editor/constants';
import { ViewOption } from '../../../common/components/view-params-editor/types';

export const MINIMAL_TIMER_OPTIONS: ViewOption[] = [
{ section: 'Timer Options' },
hideTimerSeconds,
{ section: 'Element visibility' },
{
id: 'hideovertime',
title: 'Hide Overtime',
description: 'Whether to suppress overtime styles (red borders and red text)',
type: 'boolean',
defaultValue: false,
},
{
id: 'hideendmessage',
title: 'Hide End Message',
description: 'Whether to hide end message and continue showing the clock if timer is in overtime',
type: 'boolean',
defaultValue: false,
},
{ section: 'View style override' },
{
id: 'key',
title: 'Key Colour',
description: 'Background colour in hexadecimal',
prefix: '#',
type: 'string',
placeholder: '00000000 (default)',
},
{
id: 'text',
title: 'Text Colour',
description: 'Text colour in hexadecimal',
prefix: '#',
type: 'string',
placeholder: 'fffff (default)',
},
{
id: 'textbg',
title: 'Text Background',
description: 'Colour of text background in hexadecimal',
prefix: '#',
type: 'string',
placeholder: '00000000 (default)',
},
{
id: 'font',
title: 'Font',
description: 'Font family, will use the fonts available in the system',
type: 'string',
placeholder: 'Arial Black (default)',
},
Comment on lines +22 to +53
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix inconsistency in text color placeholder.

The "View style override" section is well-structured, providing comprehensive customization options. However, there's an inconsistency in the text color placeholder.

Apply this change to fix the inconsistency:

-    placeholder: 'fffff (default)',
+    placeholder: 'ffffff (default)',

This change ensures that the placeholder represents a valid 6-digit hexadecimal color code, consistent with the other color options.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{ section: 'View style override' },
{
id: 'key',
title: 'Key Colour',
description: 'Background colour in hexadecimal',
prefix: '#',
type: 'string',
placeholder: '00000000 (default)',
},
{
id: 'text',
title: 'Text Colour',
description: 'Text colour in hexadecimal',
prefix: '#',
type: 'string',
placeholder: 'fffff (default)',
},
{
id: 'textbg',
title: 'Text Background',
description: 'Colour of text background in hexadecimal',
prefix: '#',
type: 'string',
placeholder: '00000000 (default)',
},
{
id: 'font',
title: 'Font',
description: 'Font family, will use the fonts available in the system',
type: 'string',
placeholder: 'Arial Black (default)',
},
{ section: 'View style override' },
{
id: 'key',
title: 'Key Colour',
description: 'Background colour in hexadecimal',
prefix: '#',
type: 'string',
placeholder: '00000000 (default)',
},
{
id: 'text',
title: 'Text Colour',
description: 'Text colour in hexadecimal',
prefix: '#',
type: 'string',
placeholder: 'ffffff (default)',
},
{
id: 'textbg',
title: 'Text Background',
description: 'Colour of text background in hexadecimal',
prefix: '#',
type: 'string',
placeholder: '00000000 (default)',
},
{
id: 'font',
title: 'Font',
description: 'Font family, will use the fonts available in the system',
type: 'string',
placeholder: 'Arial Black (default)',
},

{
id: 'size',
title: 'Text Size',
description: 'Scales the current style (0.5 = 50% 1 = 100% 2 = 200%)',
type: 'number',
placeholder: '1 (default)',
},
{
id: 'alignx',
title: 'Align Horizontal',
description: 'Moves the horizontally in page to start = left | center | end = right',
type: 'option',
values: { start: 'Start', center: 'Center', end: 'End' },
defaultValue: 'center',
},
{
id: 'offsetx',
title: 'Offset Horizontal',
description: 'Offsets the timer horizontal position by a given amount in pixels',
type: 'number',
placeholder: '0 (default)',
},
{
id: 'aligny',
title: 'Align Vertical',
description: 'Moves the vertically in page to start = left | center | end = right',
type: 'option',
values: { start: 'Start', center: 'Center', end: 'End' },
defaultValue: 'center',
},
{
id: 'offsety',
title: 'Offset Vertical',
description: 'Offsets the timer vertical position by a given amount in pixels',
type: 'number',
placeholder: '0 (default)',
},
];
53 changes: 53 additions & 0 deletions apps/client/src/features/viewers/pop-out-clock/PopOutTimer.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
@use '../../../theme/viewerDefs' as *;

.minimal-timer {
margin: 0;
box-sizing: border-box; /* reset */
overflow: hidden;
width: 100%; /* restrict the page width to viewport */
height: 100vh;
transition: opacity 0.5s ease-in-out;

background: var(--background-color-override, $viewer-background-color);
color: var(--color-override, $viewer-color);
display: grid;
place-content: center;

&--finished {
outline: clamp(4px, 1vw, 16px) solid $timer-finished-color;
outline-offset: calc(clamp(4px, 1vw, 16px) * -1);
transition: $viewer-transition-time;
}

.timer {
opacity: 1;
font-family: var(--font-family-bold-override, $timer-bold-font-family) ;
font-size: 20vw;
position: relative;
color: var(--timer-color-override, var(--phase-color));
transition: $viewer-transition-time;
transition-property: opacity;
background-color: transparent;
letter-spacing: 0.05em;

&--paused {
opacity: $viewer-opacity-disabled;
transition: $viewer-transition-time;
}

&--finished {
color: $timer-finished-color;
}
}

/* =================== OVERLAY ===================*/

.end-message {
text-align: center;
font-size: 12vw;
line-height: 0.9em;
font-weight: 600;
color: $timer-finished-color;
padding: 0;
}
}
160 changes: 160 additions & 0 deletions apps/client/src/features/viewers/pop-out-clock/PopOutTimer.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
import { useEffect, useRef, useState } from 'react';

Check failure on line 1 in apps/client/src/features/viewers/pop-out-clock/PopOutTimer.tsx

View workflow job for this annotation

GitHub Actions / unit-test

Run autofix to sort these imports!
import { getFormattedTimer, getTimerByType, isStringBoolean } from '../common/viewUtils';

Check failure on line 2 in apps/client/src/features/viewers/pop-out-clock/PopOutTimer.tsx

View workflow job for this annotation

GitHub Actions / unit-test

'isStringBoolean' is defined but never used. Allowed unused vars must match /^_/u
import { Playback, TimerPhase, TimerType, ViewSettings } from 'ontime-types';

Check failure on line 3 in apps/client/src/features/viewers/pop-out-clock/PopOutTimer.tsx

View workflow job for this annotation

GitHub Actions / unit-test

'Playback' is defined but never used. Allowed unused vars must match /^_/u

Check failure on line 3 in apps/client/src/features/viewers/pop-out-clock/PopOutTimer.tsx

View workflow job for this annotation

GitHub Actions / unit-test

'TimerPhase' is defined but never used. Allowed unused vars must match /^_/u

Check failure on line 3 in apps/client/src/features/viewers/pop-out-clock/PopOutTimer.tsx

View workflow job for this annotation

GitHub Actions / unit-test

'TimerType' is defined but never used. Allowed unused vars must match /^_/u
import { ViewExtendedTimer } from '../../../common/models/TimeManager.type';
import { useTranslation } from '../../../translation/TranslationProvider';


import './PopOutTimer.scss';

interface MinimalTimerProps {
isMirrored: boolean;
time: ViewExtendedTimer;
viewSettings: ViewSettings;

}

export default function PopOutClock(props: MinimalTimerProps) {
const { isMirrored, time, viewSettings } = props;

Check failure on line 18 in apps/client/src/features/viewers/pop-out-clock/PopOutTimer.tsx

View workflow job for this annotation

GitHub Actions / unit-test

'isMirrored' is assigned a value but never used. Allowed unused vars must match /^_/u

Check failure on line 18 in apps/client/src/features/viewers/pop-out-clock/PopOutTimer.tsx

View workflow job for this annotation

GitHub Actions / unit-test

'viewSettings' is assigned a value but never used. Allowed unused vars must match /^_/u
const [ready, setReady] = useState(false);
const [videoSource, setVideoSource] = useState<string | null>(null);
const canvasRef = useRef<HTMLCanvasElement | null>(null);
const videoRef = useRef<HTMLVideoElement | null>(null);

const { getLocalizedString } = useTranslation();



const stageTimer = getTimerByType(false, time);
const display = getFormattedTimer(stageTimer, time.timerType, getLocalizedString('common.minutes'), {
removeSeconds: false,
removeLeadingZero: true,
});

let color = "#000000";

Check failure on line 34 in apps/client/src/features/viewers/pop-out-clock/PopOutTimer.tsx

View workflow job for this annotation

GitHub Actions / unit-test

'color' is never reassigned. Use 'const' instead
let title = "";

Check failure on line 35 in apps/client/src/features/viewers/pop-out-clock/PopOutTimer.tsx

View workflow job for this annotation

GitHub Actions / unit-test

'title' is never reassigned. Use 'const' instead
let clicked = false;
Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using let for variables that persist across renders; use useState or useRef instead

The variables color, title, and clicked are declared with let and are re-initialized on every render. This means they won't retain any changes between renders, which may lead to unexpected behavior.

To maintain their values across renders, consider using useState or useRef. Additionally, if title and clicked are not being updated or used elsewhere in the component, they might be unnecessary.

Apply this diff to fix the issue:

- let color = "#000000";
- let title = "";
- let clicked = false;
+ const colorRef = useRef("#000000");
+ const titleRef = useRef("");
+ const clickedRef = useRef(false);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let color = "#000000";
let title = "";
let clicked = false;
const colorRef = useRef("#000000");
const titleRef = useRef("");
const clickedRef = useRef(false);
```
This suggestion replaces the original `let` declarations with `useRef` hooks, which will allow the values to persist across renders. The suggestion is complete and can be directly applied to replace the original code snippet.
Note: To use `useRef`, make sure to import it from 'react' at the top of the file if it's not already imported:
```typescript
import { useRef } from 'react';


useEffect(() => {
const canvas = canvasRef.current;
const videoElement = videoRef.current;
if (canvas && videoElement) {
const context = canvas.getContext('2d');
if (context) {
changeVideo(color, title, context, canvas, videoElement);
}
setReady(true);
}
}, []);
Comment on lines +38 to +48
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Variables color and title are not updated; consider removing or updating them

In the useEffect hook, changeVideo is called with color and title, but these variables are not updated anywhere in the component. As a result, the video might not display the intended content.

If color and title are meant to change based on certain conditions or props, ensure they are updated accordingly. Otherwise, consider removing them if they are not needed.


const openPip = async () => {
if (!videoRef.current) return;
clicked = true;
await videoRef.current.play();

if (videoRef.current !== document.pictureInPictureElement) {
try {
await videoRef.current.requestPictureInPicture();
} catch (error) {
console.error("Error: Unable to enter Picture-in-Picture mode:", error);
}
} else {
try {
await document.exitPictureInPicture();
} catch (error) {
console.error("Error: Unable to exit Picture-in-Picture mode:", error);
}
}
};

const drawFrame = (color: string, text: string, context: CanvasRenderingContext2D, canvas: HTMLCanvasElement) => {
context.fillStyle = color;
context.fillRect(0, 0, canvas.width, canvas.height);

context.font = "60px Arial";
context.fillStyle = "white";
const textWidth = context.measureText(text).width;
const x = (canvas.width - textWidth) / 2;
const y = canvas.height / 2 + 15;

context.fillText(text, x, y);
};

const createVideoBlob = (canvas: HTMLCanvasElement, context: CanvasRenderingContext2D, callback: (url: string) => void) => {
const stream = canvas.captureStream(30);
const mediaRecorder = new MediaRecorder(stream, { mimeType: 'video/webm' });
const chunks: BlobPart[] = [];

mediaRecorder.ondataavailable = (event) => {
if (event.data.size > 0) {
chunks.push(event.data);
}
};

mediaRecorder.onstop = () => {
const blob = new Blob(chunks, { type: 'video/webm' });
callback(URL.createObjectURL(blob));
};

mediaRecorder.start();
setTimeout(() => {
mediaRecorder.stop();
}, 100);
};
Comment on lines +83 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize video generation by using a continuous stream instead of creating blobs repeatedly

Currently, createVideoBlob is called every second, creating a new video blob each time. This approach is inefficient and may cause performance issues.

Consider using the canvas stream directly as the source for the video element without creating new blobs. This can be achieved by assigning the canvas stream to the video element's srcObject.

Apply this diff to refactor the video handling:

- createVideoBlob(canvas, context, (newVideoSource) => {
-   if (videoSource) {
-     URL.revokeObjectURL(videoSource);
-   }
-   setVideoSource(newVideoSource);
-   videoElement.src = newVideoSource;
-   videoElement.play().catch((error) => {
-     console.error("Error playing video:", error);
-   });
- });
+ const stream = canvas.captureStream(30);
+ videoElement.srcObject = stream;
+ videoElement.play().catch((error) => {
+   console.error("Error playing video:", error);
+ });

Committable suggestion was skipped due to low confidence.


💡 Codebase verification

Proper Cleanup of Media Resources is Not Fully Implemented

  • In apps/client/src/features/viewers/pop-out-clock/PopOutTimer.tsx, the URL.createObjectURL is used to create a blob URL but is never revoked using URL.revokeObjectURL, which can lead to memory leaks.
  • Additionally, ensure that the media stream obtained from canvas.captureStream is explicitly stopped after usage to release all associated resources.
🔗 Analysis chain

Ensure proper cleanup of media resources to prevent memory leaks

When creating media streams and blobs, it's crucial to properly clean up resources to avoid memory leaks. Ensure that any streams, blobs, or object URLs are correctly released when no longer needed.

Run the following script to check for proper cleanup of media resources:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for any unreleased media resources in the codebase.

# Expectation: All created media streams and object URLs should be properly revoked or released.

rg --type js 'URL\.createObjectURL|captureStream' -A 5

Length of output: 56


Script:

#!/bin/bash
# Description: Search for any unreleased media resources in JavaScript and TypeScript files.

# Expectation: All created media streams and object URLs should be properly revoked or released.

rg --type js --type ts --type jsx --type tsx 'URL\.createObjectURL|captureStream' -A 5

Length of output: 116


Script:

#!/bin/bash
# Description: Search for any unreleased media resources in JavaScript and TypeScript files, including JSX and TSX.

# Define custom types for jsx and tsx
rg --type-add 'jsx:*.jsx' --type-add 'tsx:*.tsx' 'URL\.createObjectURL|captureStream' -t js -t ts -t jsx -t tsx -A 5

Length of output: 1906


const changeVideo = (
color: string,
text: string,
context: CanvasRenderingContext2D,
canvas: HTMLCanvasElement,
videoElement: HTMLVideoElement
) => {
drawFrame(color, text, context, canvas);
createVideoBlob(canvas, context, (newVideoSource) => {
if (videoSource) {
URL.revokeObjectURL(videoSource);
}
setVideoSource(newVideoSource);
videoElement.src = newVideoSource;
videoElement.play().catch((error) => {
console.error("Error playing video:", error);
});
});
};

useEffect(() => {
if (ready && canvasRef.current && videoRef.current) {
const canvas = canvasRef.current;
const context = canvas.getContext('2d');
let i = 0;
const interval = setInterval(() => {
changeVideo("green", display, context!, canvas, videoRef.current!);
i++;
}, 1000);
return () => clearInterval(interval); // Clean up the interval on component unmount
}
}, [ready]);
Comment on lines +125 to +136
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider updating the useEffect to handle changes in time

The display value is derived from time, and if time changes, the interval may not reflect the updated timer. Consider updating the useEffect to handle changes in time to ensure the timer stays accurate.

Update the dependency array and adjust the effect:

- }, [ready, display]);
+ }, [ready, time]);

Committable suggestion was skipped due to low confidence.


⚠️ Potential issue

Include dependencies in the useEffect hook to prevent stale values

The useEffect hook depends on display, which is derived from time and may change over time. Currently, the dependency array only includes [ready], which could lead to the interval using stale values of display.

Consider adding display or its dependencies to the dependency array to ensure the interval reflects the latest timer values.

Apply this diff to fix the dependency array:

- }, [ready]);
+ }, [ready, display]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
useEffect(() => {
if (ready && canvasRef.current && videoRef.current) {
const canvas = canvasRef.current;
const context = canvas.getContext('2d');
let i = 0;
const interval = setInterval(() => {
changeVideo("green", display, context!, canvas, videoRef.current!);
i++;
}, 1000);
return () => clearInterval(interval); // Clean up the interval on component unmount
}
}, [ready]);
useEffect(() => {
if (ready && canvasRef.current && videoRef.current) {
const canvas = canvasRef.current;
const context = canvas.getContext('2d');
let i = 0;
const interval = setInterval(() => {
changeVideo("green", display, context!, canvas, videoRef.current!);
i++;
}, 1000);
return () => clearInterval(interval); // Clean up the interval on component unmount
}
}, [ready, display]);


return (
<div>
<div>{display}</div>
<canvas
ref={canvasRef}
id="canvas"
width="640"
height="360"
/>
<video
ref={videoRef}
id="pip-video"
loop
controls
>
{videoSource && <source src={videoSource} type="video/webm" />}
</video>
<button onClick={openPip}>
Picture-in-Picture
</button>
</div>
);
}
Loading