fix: Stop a released PTZ key ending another held key's movement (#2675)

- Closes: #2668

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<PTZDigitialAction
       this._stopped = false;
       await replaceInProgressForThisTarget(targetID, this._context, 'ptzDigital', this);
 
+      if (this._stopped) {
+        return;
+      }
+
       await this._stepChange(api, targetID);
 
       // The steps are repeated only once the first step returns, and only if
@@ -87,7 +91,19 @@ export class PTZDigitalAction extends AdvancedCameraCardAction<PTZDigitialAction
         );
       }
     } else if (action.ptz_phase === 'stop') {
-      await clearInProgressForThisTarget(targetID, this._context, 'ptzDigital');
+      // A stop only stops the movement it names, as another movement may have
+      // replaced the one this stop was issued for. An undefined ptz_action
+      // stops any movement.
+      await clearInProgressForThisTarget(
+        targetID,
+        this._context,
+        'ptzDigital',
+        action.ptz_action
+          ? (incumbent) =>
+              incumbent instanceof PTZDigitalAction &&
+              incumbent._getAction().ptz_action === action.ptz_action
+          : undefined,
+      );
     }
   }
 

src/card-controller/actions/actions/ptz.ts

@@ -114,10 +114,21 @@ export class PTZAction extends AdvancedCameraCardAction<PTZActionConfig> {
         }
       };
 
+      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;
 

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
+// `matcher` decides whether the in-progress action is one this caller may
-// for this target meanwhile is left in place.
+// 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<void> => {
   const stopped = context[contextKey]?.[targetID]?.inProgressAction;
+  if (!stopped || (matcher && !matcher(stopped))) {
+    return;
+  }
   delete context[contextKey]?.[targetID];
-  await stopped?.stop();
+  await stopped.stop();
 };

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.
+      vi.runOnlyPendingTimers();
-      expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(2);
+
+      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<Action>();
+      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 () => {

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<Action>();
+      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(incumbent.stop).not.toHaveBeenCalled();
-      expect(api.getCameraManager().executePTZAction).toHaveBeenCalledTimes(2);
-
-      await vi.runOnlyPendingTimersAsync();
-
-      expect(api.getCameraManager().executePTZAction).toHaveBeenCalledTimes(2);
     });
 
     it('should continue movement when a start is concurrent with a stop', async () => {

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<MountedCard> => {
 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<void> => {
+  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();
+    // The left key is still held, so the picture keeps moving left. A user who
-    assert(afterWaiting !== null);
+    // 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 () => {