From 70ca0a9af4752023ca4b30c2be443b7605fc2acc Mon Sep 17 00:00:00 2001 From: Dermot Duffy Date: Mon, 10 Aug 2026 20:22:39 -0700 Subject: [PATCH] fix: Stop a released PTZ key ending another held key's movement (#2675) - Closes: #2668 --- .../actions/actions/ptz-digital.ts | 20 +++- src/card-controller/actions/actions/ptz.ts | 15 ++- .../actions/utils/action-state.ts | 12 +- .../actions/actions/ptz-digital.test.ts | 111 +++++++++++++++++- .../actions/actions/ptz.test.ts | 111 ++++++++++++++++-- .../keyboard-state-manager.browser.test.ts | 101 +++++++++++++--- 6 files changed, 337 insertions(+), 33 deletions(-) diff --git a/src/card-controller/actions/actions/ptz-digital.ts b/src/card-controller/actions/actions/ptz-digital.ts index 47685d13..31011e16 100644 --- a/src/card-controller/actions/actions/ptz-digital.ts +++ b/src/card-controller/actions/actions/ptz-digital.ts @@ -19,7 +19,7 @@ import { } from '../utils/action-state'; import { AdvancedCameraCardAction } from './base'; -const STEP_DELAY_SECONDS = 0.1; +export const STEP_DELAY_SECONDS = 0.1; const STEP_ZOOM = 0.1; export const STEP_PAN = 5; @@ -77,6 +77,10 @@ export class PTZDigitalAction extends AdvancedCameraCardAction + incumbent instanceof PTZDigitalAction && + incumbent._getAction().ptz_action === action.ptz_action + : undefined, + ); } } diff --git a/src/card-controller/actions/actions/ptz.ts b/src/card-controller/actions/actions/ptz.ts index 08e3c685..9624035c 100644 --- a/src/card-controller/actions/actions/ptz.ts +++ b/src/card-controller/actions/actions/ptz.ts @@ -114,10 +114,21 @@ export class PTZAction extends AdvancedCameraCardAction { } }; - await singleStep(); + if (!this._stopped) { + await singleStep(); + } } else if (action.ptz_phase === 'stop') { // Scenario: Asked to stop continuous move, camera only supports relative moves natively. - await clearInProgressForThisTarget(ptzCameraID, this._context, 'ptz'); + // A stop only stops the movement it names, as another movement may have + // replaced the one this stop was issued for. + await clearInProgressForThisTarget( + ptzCameraID, + this._context, + 'ptz', + (incumbent) => + incumbent instanceof PTZAction && + incumbent._getAction().ptz_action === action.ptz_action, + ); } else { this._stopped = false; diff --git a/src/card-controller/actions/utils/action-state.ts b/src/card-controller/actions/utils/action-state.ts index ac17ed4d..1f243f26 100644 --- a/src/card-controller/actions/utils/action-state.ts +++ b/src/card-controller/actions/utils/action-state.ts @@ -31,14 +31,20 @@ export const replaceInProgressForThisTarget = async ( await replaced?.stop(); }; -// The removal is made before the stop is awaited, so an action that registers -// for this target meanwhile is left in place. +// `matcher` decides whether the in-progress action is one this caller may +// stop; on a mismatch the action is left running. Without a matcher, whatever +// is in progress is stopped. The removal is made before the stop is awaited, +// so an action that registers for this target meanwhile is left in place. export const clearInProgressForThisTarget = async ( targetID: string, context: ActionContext, contextKey: keyof ActionContext, + matcher?: (incumbent: Action) => boolean, ): Promise => { const stopped = context[contextKey]?.[targetID]?.inProgressAction; + if (!stopped || (matcher && !matcher(stopped))) { + return; + } delete context[contextKey]?.[targetID]; - await stopped?.stop(); + await stopped.stop(); }; diff --git a/tests/card-controller/actions/actions/ptz-digital.test.ts b/tests/card-controller/actions/actions/ptz-digital.test.ts index 02b12aed..f27459cd 100644 --- a/tests/card-controller/actions/actions/ptz-digital.test.ts +++ b/tests/card-controller/actions/actions/ptz-digital.test.ts @@ -1,7 +1,9 @@ import type { ViewContext } from 'view'; import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; +import { mock } from 'vitest-mock-extended'; import { PTZDigitalAction } from '../../../../src/card-controller/actions/actions/ptz-digital'; +import type { Action } from '../../../../src/card-controller/actions/types'; import type { CardController } from '../../../../src/card-controller/controller'; import type { PartialZoomSettings, @@ -494,7 +496,7 @@ describe('should handle ptz digital action', () => { expect(api.getViewManager().setViewWithModifiers).not.toHaveBeenCalled(); }); - it('should stop movement started by two concurrent starts', async () => { + it('should only move for the latest of two concurrent starts', async () => { const api = createCardAPI(); const context = {}; vi.mocked(api.getViewManager().getView).mockReturnValue(createView()); @@ -512,6 +514,17 @@ describe('should handle ptz digital action', () => { createStartAction('up').execute(api), ]); + // The 'up' start replaces the 'left' start before the 'left' start has + // taken its first step, so only the 'up' start moves. + expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(1); + expect(getRequestedZoom(api)).toEqual({ + ...defaultSettings, + pan: { + x: 50, + y: 45, + }, + }); + const stopAction = new PTZDigitalAction(context, { action: 'fire-dom-event', advanced_camera_card_action: 'ptz_digital', @@ -519,12 +532,104 @@ describe('should handle ptz digital action', () => { }); await stopAction.execute(api); - // One step per start, and none after the stop. - expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(2); + vi.runOnlyPendingTimers(); + + expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(1); + }); + + it('should continue movement when a stop for a different movement arrives', async () => { + const api = createCardAPI(); + const context = {}; + vi.mocked(api.getViewManager().getView).mockReturnValue(createView()); + + const leftAction = new PTZDigitalAction(context, { + action: 'fire-dom-event', + advanced_camera_card_action: 'ptz_digital', + ptz_action: 'left', + ptz_phase: 'start', + }); + await leftAction.execute(api); + + expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(1); + expect(getRequestedZoom(api)).toEqual({ + ...defaultSettings, + pan: { + x: 45, + y: 50, + }, + }); + + // A stop for a movement that is not in progress leaves the 'left' + // movement running. + const stopUpAction = new PTZDigitalAction(context, { + action: 'fire-dom-event', + advanced_camera_card_action: 'ptz_digital', + ptz_action: 'up', + ptz_phase: 'stop', + }); + await stopUpAction.execute(api); vi.runOnlyPendingTimers(); expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(2); + + const stopLeftAction = new PTZDigitalAction(context, { + action: 'fire-dom-event', + advanced_camera_card_action: 'ptz_digital', + ptz_action: 'left', + ptz_phase: 'stop', + }); + await stopLeftAction.execute(api); + + vi.runOnlyPendingTimers(); + + expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(2); + }); + + it('should not stop an in-progress action that is not a digital PTZ action', async () => { + const api = createCardAPI(); + vi.mocked(api.getViewManager().getView).mockReturnValue(createView()); + + const incumbent = mock(); + const context = { ptzDigital: { camera: { inProgressAction: incumbent } } }; + + const stopAction = new PTZDigitalAction(context, { + action: 'fire-dom-event', + advanced_camera_card_action: 'ptz_digital', + ptz_action: 'left', + ptz_phase: 'stop', + }); + await stopAction.execute(api); + + expect(incumbent.stop).not.toHaveBeenCalled(); + }); + + it('should not repeat steps when stopped during the first step', async () => { + const api = createCardAPI(); + vi.mocked(api.getViewManager().getView).mockReturnValue(createView()); + + const action = new PTZDigitalAction( + {}, + { + action: 'fire-dom-event', + advanced_camera_card_action: 'ptz_digital', + ptz_action: 'left', + ptz_phase: 'start', + }, + ); + + // The stop arrives while the first step is being taken. + vi.mocked(api.getViewManager().setViewWithModifiers).mockImplementationOnce(() => { + action.stop(); + }); + + await action.execute(api); + + expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(1); + + vi.runOnlyPendingTimers(); + + expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(1); }); it('should continue movement when a start is concurrent with a stop', async () => { diff --git a/tests/card-controller/actions/actions/ptz.test.ts b/tests/card-controller/actions/actions/ptz.test.ts index 592a7e41..95289486 100644 --- a/tests/card-controller/actions/actions/ptz.test.ts +++ b/tests/card-controller/actions/actions/ptz.test.ts @@ -1,7 +1,9 @@ import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; +import { mock } from 'vitest-mock-extended'; import { Capabilities } from '../../../../src/camera-manager/capabilities'; import { PTZAction } from '../../../../src/card-controller/actions/actions/ptz'; +import type { Action } from '../../../../src/card-controller/actions/types'; import { PTZMovementType } from '../../../../src/types'; import { createCameraManager, createStore } from '../../../camera-manager/test-utils'; import { createCameraConfig } from '../../../config/test-utils'; @@ -612,7 +614,7 @@ describe('should handle ptz action', () => { expect(api.getCameraManager().executePTZAction).toHaveBeenCalledTimes(3); }); - it('should stop movement started by two concurrent starts', async () => { + it('should only move for the latest of two concurrent starts', async () => { const api = createCardAPI(); const store = createStore([ { @@ -644,6 +646,106 @@ describe('should handle ptz action', () => { createStartAction('up').execute(api), ]); + // The 'up' start replaces the 'left' start before the 'left' start has + // made its first move, so only the 'up' start moves. + expect(api.getCameraManager().executePTZAction).toHaveBeenCalledTimes(1); + expect(api.getCameraManager().executePTZAction).toHaveBeenCalledWith( + 'camera.office', + 'up', + { preset: undefined }, + ); + + const stopAction = new PTZAction(context, { + action: 'fire-dom-event', + advanced_camera_card_action: 'ptz', + ptz_action: 'up', + ptz_phase: 'stop', + }); + await stopAction.execute(api); + + await vi.runOnlyPendingTimersAsync(); + + expect(api.getCameraManager().executePTZAction).toHaveBeenCalledTimes(1); + }); + + it('should continue movement when a stop for a different movement arrives', async () => { + const api = createCardAPI(); + const store = createStore([ + { + cameraID: 'camera.office', + capabilities: new Capabilities({ + ptz: { + left: [PTZMovementType.Relative], + up: [PTZMovementType.Relative], + }, + }), + }, + ]); + vi.mocked(api.getCameraManager).mockReturnValue(createCameraManager(store)); + vi.mocked(api.getViewManager().getView).mockReturnValue( + createView({ camera: 'camera.office' }), + ); + + const context = {}; + await new PTZAction(context, { + action: 'fire-dom-event', + advanced_camera_card_action: 'ptz', + ptz_action: 'left', + ptz_phase: 'start', + }).execute(api); + + expect(api.getCameraManager().executePTZAction).toHaveBeenCalledTimes(1); + + // A stop for a movement that is not in progress leaves the 'left' + // movement running. + await new PTZAction(context, { + action: 'fire-dom-event', + advanced_camera_card_action: 'ptz', + ptz_action: 'up', + ptz_phase: 'stop', + }).execute(api); + + await vi.runOnlyPendingTimersAsync(); + + expect(api.getCameraManager().executePTZAction).toHaveBeenCalledTimes(2); + expect(api.getCameraManager().executePTZAction).toHaveBeenLastCalledWith( + 'camera.office', + 'left', + { preset: undefined }, + ); + + await new PTZAction(context, { + action: 'fire-dom-event', + advanced_camera_card_action: 'ptz', + ptz_action: 'left', + ptz_phase: 'stop', + }).execute(api); + + await vi.runOnlyPendingTimersAsync(); + + expect(api.getCameraManager().executePTZAction).toHaveBeenCalledTimes(2); + }); + + it('should not stop an in-progress action that is not a PTZ action', async () => { + const api = createCardAPI(); + const store = createStore([ + { + cameraID: 'camera.office', + capabilities: new Capabilities({ + ptz: { + left: [PTZMovementType.Relative], + }, + }), + }, + ]); + vi.mocked(api.getCameraManager).mockReturnValue(createCameraManager(store)); + vi.mocked(api.getViewManager().getView).mockReturnValue( + createView({ camera: 'camera.office' }), + ); + + const incumbent = mock(); + const context = { ptz: { 'camera.office': { inProgressAction: incumbent } } }; + const stopAction = new PTZAction(context, { action: 'fire-dom-event', advanced_camera_card_action: 'ptz', @@ -652,12 +754,7 @@ describe('should handle ptz action', () => { }); await stopAction.execute(api); - // One move per start, and none after the stop. - expect(api.getCameraManager().executePTZAction).toHaveBeenCalledTimes(2); - - await vi.runOnlyPendingTimersAsync(); - - expect(api.getCameraManager().executePTZAction).toHaveBeenCalledTimes(2); + expect(incumbent.stop).not.toHaveBeenCalled(); }); it('should continue movement when a start is concurrent with a stop', async () => { diff --git a/tests/card-controller/keyboard-state-manager.browser.test.ts b/tests/card-controller/keyboard-state-manager.browser.test.ts index 3cb284fa..754bc0be 100644 --- a/tests/card-controller/keyboard-state-manager.browser.test.ts +++ b/tests/card-controller/keyboard-state-manager.browser.test.ts @@ -1,6 +1,9 @@ import { assert, beforeEach, describe, expect, it, vi } from 'vitest'; -import { STEP_PAN } from '../../src/card-controller/actions/actions/ptz-digital'; +import { + STEP_DELAY_SECONDS, + STEP_PAN, +} from '../../src/card-controller/actions/actions/ptz-digital'; import type { ZoomSettingsObserved } from '../../src/components-lib/zoom/types'; import type { LogActionConfig } from '../../src/config/schema/actions/custom/log'; import { createLogAction } from '../../src/utils/action'; @@ -123,6 +126,38 @@ const mountCard = async (options?: MountCardOptions): Promise => { const isZoomSettingsObserved = (detail: unknown): detail is ZoomSettingsObserved => isRecord(detail) && isRecord(detail.pan) && typeof detail.pan.x === 'number'; +// How far across and down the camera the picture sits, as percentages, from the +// last change the card reported. It starts halfway on both. +const getPan = (card: MountedCard): { x: number; y: number } | null => { + const detail = card.events + .getEntries('advanced-camera-card:zoom:change') + .at(-1)?.detail; + return isZoomSettingsObserved(detail) ? detail.pan : null; +}; + +// A count of step-timer periods (STEP_DELAY_SECONDS each) to run the clock +// forward. A movement still running takes one step per period, so any count +// above the single late step the assertion tolerates would do; three is a +// comfortable margin. +const STEPS_TO_PROVE_STOPPED = 3; + +// The card reports a change per step taken, so counting the changes says +// whether a movement is still going, rather than where it has got to. +const countPanSteps = (card: MountedCard): number => + card.events.getEntries('advanced-camera-card:zoom:change').length; + +// Assert a movement has stopped by running the step timer several periods +// forward and checking no further steps are taken. +const expectPanStopped = async (card: MountedCard): Promise => { + const steps = countPanSteps(card); + + await card.advanceSeconds(STEP_DELAY_SECONDS * STEPS_TO_PROVE_STOPPED); + + // The one step already scheduled when the movement stopped is allowed; a + // movement still going would take several more. + expect(countPanSteps(card)).toBeLessThanOrEqual(steps + 1); +}; + // What the live view draws the camera into, which is the part of the card a // user looks at and the largest part of it that is not a control. const LIVE_MEDIA_SELECTOR = 'advanced-camera-card-live-provider'; @@ -328,22 +363,13 @@ describe('KeyboardStateManager', () => { // Press a key to ensure holds after initial press are functional. await pressKey('ArrowUp'); - // How far across the camera the picture sits, as a percentage, from the - // last change the card reported. It starts halfway across. - const panX = (): number | null => { - const detail = card.events - .getEntries('advanced-camera-card:zoom:change') - .at(-1)?.detail; - return isZoomSettingsObserved(detail) ? detail.pan.x : null; - }; - //`shouldAdvanceTime` lets the clock run at its own pace until "controlled", // necessary for the panning while a key is being "held" to work. vi.useFakeTimers({ shouldAdvanceTime: true }); await holdKey('ArrowLeft'); await card.waitForRender(() => { - const x = panX(); + const x = getPan(card)?.x ?? null; // Stop well short of the left edge, to allow detection of continuous pan // that never stopped. @@ -351,15 +377,58 @@ describe('KeyboardStateManager', () => { }, 'the picture to pan left'); await releaseKey('ArrowLeft'); - const atRelease = panX(); + await expectPanStopped(card); + }); + + // See: https://github.com/dermotduffy/advanced-camera-card/issues/2668 + it('should keep panning for a held key when another key is released', async () => { + const card = await mountCard(); + + card.setEntityState(ZOOM_ENTITY, 'on'); + await card.events.waitForFirst('advanced-camera-card:zoom:zoomed'); + await clickMedia(card); + + //`shouldAdvanceTime` lets the clock run at its own pace until "controlled", + // necessary for the panning while a key is being "held" to work. + vi.useFakeTimers({ shouldAdvanceTime: true }); + + await holdKey('ArrowUp'); + await card.waitForRender(() => { + const y = getPan(card)?.y ?? null; + return y !== null && y < 50 ? y : null; + }, 'the picture to pan up'); + + // The left key is pressed while the up key is still held, which takes the + // movement over from it. + await holdKey('ArrowLeft'); + await card.waitForRender(() => { + const x = getPan(card)?.x ?? null; + return x !== null && x < 50 ? x : null; + }, 'the picture to pan left'); + + const atRelease = getPan(card); assert(atRelease !== null); - await card.advanceSeconds(5); + await releaseKey('ArrowUp'); - const afterWaiting = panX(); - assert(afterWaiting !== null); + // The left key is still held, so the picture keeps moving left. A user who + // lets go of one arrow key while holding another expects the camera to + // carry on in the direction they are still requesting. + await card.waitForRender(() => { + const x = getPan(card)?.x ?? null; + return x !== null && x < atRelease.x - STEP_PAN ? x : null; + }, 'the picture to keep panning left'); - expect(Math.abs(atRelease - afterWaiting)).toBeLessThanOrEqual(STEP_PAN); + await releaseKey('ArrowLeft'); + + const atStop = getPan(card); + assert(atStop !== null); + + // The pan must stop with room to spare before the left edge, so + // expectPanStopped below can tell a stopped pan from a moving one. + expect(atStop.x).toBeGreaterThan(STEPS_TO_PROVE_STOPPED * STEP_PAN); + + await expectPanStopped(card); }); it('should not act on a key aimed at another card', async () => {