mirror of
https://github.com/cpvalente/ontime.git
synced 2026-08-20 14:39:06 +00:00
fix(timer): address review findings on the group timer
- keep the pending and none phases when showing the group timer. These describe the playback state rather than a threshold, so collapsing them made roll standby render as a running countdown and silently dropped user styling keyed on the phase - only show the group indicator for timer types which render the running timer. It was labelling the wall clock, and the invisible display of the none timer type, as a group - accept useGroupTimer in the MCP group patch. It was documented in the group shape but dropped on write, which reported success without applying the change - narrow the timer preview tooltip, which claimed all views follow the group timer when only the timer views do Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kniqs443KUNCRABwVJwT7K
This commit is contained in:
@@ -107,7 +107,7 @@ export default function TimerPreview() {
|
|||||||
<LuArrowDownToLine />
|
<LuArrowDownToLine />
|
||||||
</Tooltip>
|
</Tooltip>
|
||||||
<Tooltip
|
<Tooltip
|
||||||
text={isGroupTimerActive ? 'Views are showing the shared group timer' : 'Views are showing the event timer'}
|
text={`Timer views are showing the ${isGroupTimerActive ? 'shared group timer' : 'event timer'}`}
|
||||||
render={<span />}
|
render={<span />}
|
||||||
className={style.statusIcon}
|
className={style.statusIcon}
|
||||||
data-active={isGroupTimerActive}
|
data-active={isGroupTimerActive}
|
||||||
|
|||||||
@@ -10,7 +10,7 @@ import {
|
|||||||
getIsPlaying,
|
getIsPlaying,
|
||||||
getSecondaryDisplay,
|
getSecondaryDisplay,
|
||||||
getShowMessage,
|
getShowMessage,
|
||||||
getShowProgressBar,
|
getShowsTimerValue,
|
||||||
resolveTimerDisplay,
|
resolveTimerDisplay,
|
||||||
} from '../../timer/timer.utils';
|
} from '../../timer/timer.utils';
|
||||||
import { getTimerColour } from '../../utils/presentation.utils';
|
import { getTimerColour } from '../../utils/presentation.utils';
|
||||||
@@ -40,7 +40,8 @@ export function PipTimer({ viewSettings }: PipTimerProps) {
|
|||||||
const { showFinished, showWarning, showDanger } = timerDisplay;
|
const { showFinished, showWarning, showDanger } = timerDisplay;
|
||||||
|
|
||||||
const isPlaying = getIsPlaying(time.playback);
|
const isPlaying = getIsPlaying(time.playback);
|
||||||
const showProgressBar = getShowProgressBar(timerTypeNow);
|
const showsTimerValue = getShowsTimerValue(timerTypeNow);
|
||||||
|
const showProgressBar = showsTimerValue;
|
||||||
|
|
||||||
// gather timer data
|
// gather timer data
|
||||||
const stageTimer = getTimerByType(false, timerTypeNow, clock, timerDisplay.source, timerTypeNow);
|
const stageTimer = getTimerByType(false, timerTypeNow, clock, timerDisplay.source, timerTypeNow);
|
||||||
@@ -87,7 +88,7 @@ export function PipTimer({ viewSettings }: PipTimerProps) {
|
|||||||
>
|
>
|
||||||
{display}
|
{display}
|
||||||
</div>
|
</div>
|
||||||
{timerDisplay.isGroup && <div className='group-indicator'>group</div>}
|
{timerDisplay.isGroup && showsTimerValue && <div className='group-indicator'>group</div>}
|
||||||
<div className={cx(['secondary', !secondaryContent && 'secondary--hidden'])}>
|
<div className={cx(['secondary', !secondaryContent && 'secondary--hidden'])}>
|
||||||
<FitText mode='multi' min={12} max={256}>
|
<FitText mode='multi' min={12} max={256}>
|
||||||
{secondaryContent}
|
{secondaryContent}
|
||||||
|
|||||||
@@ -25,7 +25,7 @@ import {
|
|||||||
getSecondaryDisplay,
|
getSecondaryDisplay,
|
||||||
getShowClock,
|
getShowClock,
|
||||||
getShowMessage,
|
getShowMessage,
|
||||||
getShowProgressBar,
|
getShowsTimerValue,
|
||||||
resolveTimerDisplay,
|
resolveTimerDisplay,
|
||||||
} from './timer.utils';
|
} from './timer.utils';
|
||||||
import { TimerData, useTimerData } from './useTimerData';
|
import { TimerData, useTimerData } from './useTimerData';
|
||||||
@@ -92,7 +92,8 @@ function Timer({ customFields, projectData, isMirrored, settings, viewSettings,
|
|||||||
const { showEndMessage, showFinished, showWarning, showDanger } = timerDisplay;
|
const { showEndMessage, showFinished, showWarning, showDanger } = timerDisplay;
|
||||||
const isPlaying = getIsPlaying(time.playback);
|
const isPlaying = getIsPlaying(time.playback);
|
||||||
const showClock = !hideClock && getShowClock(viewTimerType);
|
const showClock = !hideClock && getShowClock(viewTimerType);
|
||||||
const showProgressBar = !hideProgress && getShowProgressBar(viewTimerType);
|
const showsTimerValue = getShowsTimerValue(viewTimerType);
|
||||||
|
const showProgressBar = !hideProgress && showsTimerValue;
|
||||||
|
|
||||||
// gather card data
|
// gather card data
|
||||||
const { showNow, nowMain, nowSecondary, showNext, nextMain, nextSecondary } = getCardData(
|
const { showNow, nowMain, nowSecondary, showNext, nextMain, nextSecondary } = getCardData(
|
||||||
@@ -186,7 +187,9 @@ function Timer({ customFields, projectData, isMirrored, settings, viewSettings,
|
|||||||
{display}
|
{display}
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
{timerDisplay.isGroup && !showEndMessage && <div className='group-indicator'>group</div>}
|
{timerDisplay.isGroup && showsTimerValue && !showEndMessage && (
|
||||||
|
<div className='group-indicator'>group</div>
|
||||||
|
)}
|
||||||
<div className={cx(['secondary', !secondaryContent && 'secondary--hidden'])}>
|
<div className={cx(['secondary', !secondaryContent && 'secondary--hidden'])}>
|
||||||
<FitText mode='multi' min={64} max={256}>
|
<FitText mode='multi' min={64} max={256}>
|
||||||
{secondaryContent}
|
{secondaryContent}
|
||||||
|
|||||||
@@ -0,0 +1,110 @@
|
|||||||
|
import { GroupTimerState, Playback, TimerPhase, TimerState, TimerType } from 'ontime-types';
|
||||||
|
|
||||||
|
import { getShowsTimerValue, resolveTimerDisplay } from '../timer.utils';
|
||||||
|
|
||||||
|
const makeTime = (patch: Partial<TimerState> = {}): TimerState => ({
|
||||||
|
addedTime: 0,
|
||||||
|
current: 10000,
|
||||||
|
duration: 60000,
|
||||||
|
elapsed: 50000,
|
||||||
|
expectedFinish: null,
|
||||||
|
phase: TimerPhase.Default,
|
||||||
|
playback: Playback.Play,
|
||||||
|
secondaryTimer: null,
|
||||||
|
startedAt: null,
|
||||||
|
...patch,
|
||||||
|
});
|
||||||
|
|
||||||
|
const makeGroupTimer = (patch: Partial<GroupTimerState> = {}): GroupTimerState => ({
|
||||||
|
current: 100000,
|
||||||
|
elapsed: 80000,
|
||||||
|
duration: 180000,
|
||||||
|
...patch,
|
||||||
|
});
|
||||||
|
|
||||||
|
const resolve = (time: TimerState, groupTimer: GroupTimerState | null) =>
|
||||||
|
resolveTimerDisplay({
|
||||||
|
time,
|
||||||
|
groupTimer,
|
||||||
|
event: { timeWarning: 120000, timeDanger: 60000 },
|
||||||
|
timerType: TimerType.CountDown,
|
||||||
|
countToEnd: false,
|
||||||
|
freezeOvertime: false,
|
||||||
|
freezeMessage: '',
|
||||||
|
hidePhase: false,
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('resolveTimerDisplay()', () => {
|
||||||
|
it('uses the event timer when there is no group timer', () => {
|
||||||
|
const time = makeTime();
|
||||||
|
const result = resolve(time, null);
|
||||||
|
|
||||||
|
expect(result.isGroup).toBe(false);
|
||||||
|
expect(result.source).toBe(time);
|
||||||
|
expect(result.total).toBe(60000);
|
||||||
|
expect(result.warning).toBe(120000);
|
||||||
|
expect(result.danger).toBe(60000);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('adds the time added to an event into the event total', () => {
|
||||||
|
expect(resolve(makeTime({ addedTime: 30000 }), null).total).toBe(90000);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('uses the group timer when one is present', () => {
|
||||||
|
const groupTimer = makeGroupTimer();
|
||||||
|
const result = resolve(makeTime(), groupTimer);
|
||||||
|
|
||||||
|
expect(result.isGroup).toBe(true);
|
||||||
|
expect(result.source).toBe(groupTimer);
|
||||||
|
expect(result.total).toBe(180000);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('drops the event thresholds while showing the group', () => {
|
||||||
|
const result = resolve(makeTime({ phase: TimerPhase.Danger }), makeGroupTimer());
|
||||||
|
|
||||||
|
expect(result.warning).toBeUndefined();
|
||||||
|
expect(result.danger).toBeUndefined();
|
||||||
|
});
|
||||||
|
|
||||||
|
it.each([TimerPhase.Warning, TimerPhase.Danger])(
|
||||||
|
'reports the group as running while the event is in %s',
|
||||||
|
(phase) => {
|
||||||
|
const result = resolve(makeTime({ phase }), makeGroupTimer());
|
||||||
|
|
||||||
|
expect(result.phase).toBe(TimerPhase.Default);
|
||||||
|
expect(result.showWarning).toBe(false);
|
||||||
|
expect(result.showDanger).toBe(false);
|
||||||
|
expect(result.showFinished).toBe(false);
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
|
it('reports overtime only once the group itself has run out of time', () => {
|
||||||
|
const overrunningEvent = makeTime({ current: -5000, phase: TimerPhase.Overtime });
|
||||||
|
|
||||||
|
// the event is over, but the group still has time left
|
||||||
|
const stillRunning = resolve(overrunningEvent, makeGroupTimer({ current: 60000 }));
|
||||||
|
expect(stillRunning.phase).toBe(TimerPhase.Default);
|
||||||
|
expect(stillRunning.showFinished).toBe(false);
|
||||||
|
|
||||||
|
const overtime = resolve(overrunningEvent, makeGroupTimer({ current: -1000 }));
|
||||||
|
expect(overtime.phase).toBe(TimerPhase.Overtime);
|
||||||
|
expect(overtime.showFinished).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it.each([TimerPhase.Pending, TimerPhase.None])('keeps the %s playback phase on the group', (phase) => {
|
||||||
|
// these describe the playback state rather than a threshold, so they are true of the group too
|
||||||
|
expect(resolve(makeTime({ phase }), makeGroupTimer()).phase).toBe(phase);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('getShowsTimerValue()', () => {
|
||||||
|
it('is true for timer types which render the running timer', () => {
|
||||||
|
expect(getShowsTimerValue(TimerType.CountDown)).toBe(true);
|
||||||
|
expect(getShowsTimerValue(TimerType.CountUp)).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('is false for timer types which do not reflect what is loaded', () => {
|
||||||
|
expect(getShowsTimerValue(TimerType.Clock)).toBe(false);
|
||||||
|
expect(getShowsTimerValue(TimerType.None)).toBe(false);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -36,9 +36,11 @@ export function getTotalTime(duration: MaybeNumber, addedTime: MaybeNumber): num
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Whether the progress bar should be shown for this timer type
|
* Whether this timer type renders a value derived from the running timer.
|
||||||
|
* Clock shows the time of day and none shows nothing, so neither reflects what is loaded,
|
||||||
|
* which also means neither can carry a progress bar or a group timer indicator
|
||||||
*/
|
*/
|
||||||
export function getShowProgressBar(timerType: TimerType) {
|
export function getShowsTimerValue(timerType: TimerType) {
|
||||||
return timerType !== TimerType.None && timerType !== TimerType.Clock;
|
return timerType !== TimerType.None && timerType !== TimerType.Clock;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -147,11 +149,13 @@ export function resolveTimerDisplay({
|
|||||||
hidePhase,
|
hidePhase,
|
||||||
}: ResolveTimerDisplayOptions) {
|
}: ResolveTimerDisplayOptions) {
|
||||||
const isGroup = groupTimer !== null;
|
const isGroup = groupTimer !== null;
|
||||||
const phase = isGroup
|
const phase = (() => {
|
||||||
? groupTimer.current <= 0
|
if (!isGroup) return time.phase;
|
||||||
? TimerPhase.Overtime
|
// pending and none describe the playback state rather than a threshold,
|
||||||
: TimerPhase.Default
|
// they are just as true of the group as they are of the event
|
||||||
: time.phase;
|
if (time.phase === TimerPhase.Pending || time.phase === TimerPhase.None) return time.phase;
|
||||||
|
return groupTimer.current <= 0 ? TimerPhase.Overtime : TimerPhase.Default;
|
||||||
|
})();
|
||||||
|
|
||||||
return {
|
return {
|
||||||
isGroup,
|
isGroup,
|
||||||
|
|||||||
@@ -54,7 +54,9 @@ export type EventFieldArgs = Partial<
|
|||||||
>;
|
>;
|
||||||
export type MilestoneFieldArgs = Partial<Pick<OntimeMilestone, 'cue' | 'title' | 'note' | 'colour' | 'custom'>>;
|
export type MilestoneFieldArgs = Partial<Pick<OntimeMilestone, 'cue' | 'title' | 'note' | 'colour' | 'custom'>>;
|
||||||
export type DelayFieldArgs = Partial<Pick<OntimeDelay, 'duration'>>;
|
export type DelayFieldArgs = Partial<Pick<OntimeDelay, 'duration'>>;
|
||||||
export type GroupFieldArgs = Partial<Pick<OntimeGroup, 'title' | 'note' | 'colour' | 'targetDuration' | 'custom'>>;
|
export type GroupFieldArgs = Partial<
|
||||||
|
Pick<OntimeGroup, 'title' | 'note' | 'colour' | 'targetDuration' | 'custom' | 'useGroupTimer'>
|
||||||
|
>;
|
||||||
|
|
||||||
export type EntryFieldArgs = EventFieldArgs & MilestoneFieldArgs & DelayFieldArgs & GroupFieldArgs;
|
export type EntryFieldArgs = EventFieldArgs & MilestoneFieldArgs & DelayFieldArgs & GroupFieldArgs;
|
||||||
export type TargetRundownArgs = { rundownId?: string };
|
export type TargetRundownArgs = { rundownId?: string };
|
||||||
@@ -170,6 +172,7 @@ function toGroupPatch(args: CreateEntryArgs, id: EntryId): PatchWithId<OntimeGro
|
|||||||
if (args.colour !== undefined) patch.colour = args.colour;
|
if (args.colour !== undefined) patch.colour = args.colour;
|
||||||
if (args.targetDuration !== undefined) patch.targetDuration = args.targetDuration;
|
if (args.targetDuration !== undefined) patch.targetDuration = args.targetDuration;
|
||||||
if (args.custom !== undefined) patch.custom = args.custom;
|
if (args.custom !== undefined) patch.custom = args.custom;
|
||||||
|
if (args.useGroupTimer !== undefined) patch.useGroupTimer = args.useGroupTimer;
|
||||||
|
|
||||||
return Object.keys(patch).length > 1 ? patch : null;
|
return Object.keys(patch).length > 1 ? patch : null;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user