From 6ba19b3e01b871cc55304f108eb381739f340c3b Mon Sep 17 00:00:00 2001 From: Rob Watson Date: Sat, 29 Jan 2022 12:32:39 +0100 Subject: [PATCH] Bug fix: prevent incorrect selectionChange callbacks When re-rendering the HudCanvas component, the selectionChange callback should not be triggered with the passed-in properties. Doing so leads to incorrect selection values being bubbled up when the selection is not enclosed in the viewport. The state management should probably be improved to avoid this dance completely, possibly by hoisting all of this state up to the top-level. --- frontend/src/HudCanvas.tsx | 2 -- frontend/src/HudCanvasState.test.ts | 24 +++++++++++++++++++++++- frontend/src/HudCanvasState.ts | 9 ++++++++- frontend/src/Waveform.tsx | 5 +++++ 4 files changed, 36 insertions(+), 4 deletions(-) diff --git a/frontend/src/HudCanvas.tsx b/frontend/src/HudCanvas.tsx index 9fff965..654a482 100644 --- a/frontend/src/HudCanvas.tsx +++ b/frontend/src/HudCanvas.tsx @@ -98,8 +98,6 @@ export const HudCanvas: React.FC = ({ // trigger onSelectionChange callback. useEffect(() => { - // really we only care if hoverX hasn't changed, not checking this will - // result in a flood of events being published. if (!state.shouldPublish) { return; } diff --git a/frontend/src/HudCanvasState.test.ts b/frontend/src/HudCanvasState.test.ts index c156a24..8a04a3f 100644 --- a/frontend/src/HudCanvasState.test.ts +++ b/frontend/src/HudCanvasState.test.ts @@ -27,6 +27,7 @@ describe('stateReducer', () => { { type: 'setselection', x: 0, selection: { start: 100, end: 200 } } ); expect(state.selection).toEqual({ start: 100, end: 200 }); + expect(state.shouldPublish).toBeFalsy(); }); }); @@ -40,6 +41,7 @@ describe('stateReducer', () => { expect(state.mode).toEqual(SelectionMode.ResizingStart); expect(state.selection).toEqual({ start: 1000, end: 2000 }); expect(state.mousedownX).toEqual(995); + expect(state.shouldPublish).toBeTruthy(); }); }); describe('when hovering over the selection end', () => { @@ -51,6 +53,7 @@ describe('stateReducer', () => { expect(state.mode).toEqual(SelectionMode.ResizingEnd); expect(state.selection).toEqual({ start: 1000, end: 2000 }); expect(state.mousedownX).toEqual(2003); + expect(state.shouldPublish).toBeTruthy(); }); }); describe('when hovering over the selection', () => { @@ -62,6 +65,7 @@ describe('stateReducer', () => { expect(state.mode).toEqual(SelectionMode.Dragging); expect(state.selection).toEqual({ start: 1000, end: 2000 }); expect(state.mousedownX).toEqual(1500); + expect(state.shouldPublish).toBeTruthy(); }); }); describe('when not hovering over the selection', () => { @@ -73,6 +77,7 @@ describe('stateReducer', () => { expect(state.mode).toEqual(SelectionMode.Selecting); expect(state.selection).toEqual({ start: 3000, end: 3000 }); expect(state.mousedownX).toEqual(3000); + expect(state.shouldPublish).toBeTruthy(); }); }); }); @@ -92,6 +97,7 @@ describe('stateReducer', () => { expect(state.mode).toEqual(SelectionMode.Normal); expect(state.selection).toEqual({ start: 1000, end: 2000 }); expect(state.mousedownX).toEqual(1200); + expect(state.shouldPublish).toBeTruthy(); }); }); @@ -108,6 +114,7 @@ describe('stateReducer', () => { ); expect(state.mode).toEqual(SelectionMode.Normal); expect(state.selection).toEqual({ start: 500, end: 500 }); + expect(state.shouldPublish).toBeTruthy(); }); }); @@ -124,11 +131,12 @@ describe('stateReducer', () => { ); expect(state.mode).toEqual(SelectionMode.Normal); expect(state.selection).toEqual({ start: 1000, end: 2000 }); + expect(state.shouldPublish).toBeTruthy(); }); }); }); - describe('mouseup', () => { + describe('mouseleave', () => { it('sets the state', () => { const state = stateReducer( { @@ -142,6 +150,7 @@ describe('stateReducer', () => { expect(state.mode).toEqual(SelectionMode.Dragging); expect(state.selection).toEqual({ start: 2000, end: 3000 }); expect(state.mousedownX).toEqual(475); + expect(state.shouldPublish).toBeFalsy(); }); }); @@ -160,6 +169,7 @@ describe('stateReducer', () => { expect(state.mode).toEqual(SelectionMode.Normal); expect(state.selection).toEqual({ start: 1000, end: 3000 }); expect(state.hoverState).toEqual(HoverState.OverSelectionStart); + expect(state.shouldPublish).toBeFalsy(); }); }); @@ -176,6 +186,7 @@ describe('stateReducer', () => { expect(state.mode).toEqual(SelectionMode.Normal); expect(state.selection).toEqual({ start: 1000, end: 3000 }); expect(state.hoverState).toEqual(HoverState.OverSelectionEnd); + expect(state.shouldPublish).toBeFalsy(); }); }); @@ -192,6 +203,7 @@ describe('stateReducer', () => { expect(state.mode).toEqual(SelectionMode.Normal); expect(state.selection).toEqual({ start: 1000, end: 3000 }); expect(state.hoverState).toEqual(HoverState.OverSelection); + expect(state.shouldPublish).toBeFalsy(); }); }); @@ -208,6 +220,7 @@ describe('stateReducer', () => { expect(state.mode).toEqual(SelectionMode.Normal); expect(state.selection).toEqual({ start: 1000, end: 3000 }); expect(state.hoverState).toEqual(HoverState.Normal); + expect(state.shouldPublish).toBeFalsy(); }); }); }); @@ -226,6 +239,7 @@ describe('stateReducer', () => { ); expect(state.mode).toEqual(SelectionMode.Selecting); expect(state.selection).toEqual({ start: 2000, end: 3005 }); + expect(state.shouldPublish).toBeTruthy(); }); }); @@ -242,6 +256,7 @@ describe('stateReducer', () => { ); expect(state.mode).toEqual(SelectionMode.Selecting); expect(state.selection).toEqual({ start: 1995, end: 2000 }); + expect(state.shouldPublish).toBeTruthy(); }); }); }); @@ -261,6 +276,7 @@ describe('stateReducer', () => { ); expect(state.mode).toEqual(SelectionMode.Dragging); expect(state.selection).toEqual({ start: 1020, end: 1520 }); + expect(state.shouldPublish).toBeTruthy(); }); }); @@ -278,6 +294,7 @@ describe('stateReducer', () => { ); expect(state.mode).toEqual(SelectionMode.Dragging); expect(state.selection).toEqual({ start: 0, end: 200 }); + expect(state.shouldPublish).toBeTruthy(); }); }); @@ -296,6 +313,7 @@ describe('stateReducer', () => { ); expect(state.mode).toEqual(SelectionMode.Dragging); expect(state.selection).toEqual({ start: 2900, end: 3000 }); + expect(state.shouldPublish).toBeTruthy(); }); }); }); @@ -314,6 +332,7 @@ describe('stateReducer', () => { ); expect(state.mode).toEqual(SelectionMode.ResizingStart); expect(state.selection).toEqual({ start: 2020, end: 3000 }); + expect(state.shouldPublish).toBeTruthy(); }); }); @@ -330,6 +349,7 @@ describe('stateReducer', () => { ); expect(state.mode).toEqual(SelectionMode.ResizingStart); expect(state.selection).toEqual({ start: 2002, end: 2010 }); + expect(state.shouldPublish).toBeTruthy(); }); }); }); @@ -348,6 +368,7 @@ describe('stateReducer', () => { ); expect(state.mode).toEqual(SelectionMode.ResizingEnd); expect(state.selection).toEqual({ start: 1000, end: 2007 }); + expect(state.shouldPublish).toBeTruthy(); }); }); @@ -364,6 +385,7 @@ describe('stateReducer', () => { ); expect(state.mode).toEqual(SelectionMode.ResizingEnd); expect(state.selection).toEqual({ start: 1995, end: 2000 }); + expect(state.shouldPublish).toBeTruthy(); }); }); }); diff --git a/frontend/src/HudCanvasState.ts b/frontend/src/HudCanvasState.ts index b8e70d9..6453cea 100644 --- a/frontend/src/HudCanvasState.ts +++ b/frontend/src/HudCanvasState.ts @@ -63,6 +63,7 @@ export const stateReducer = ( let mousedownX: number; let cursorClass: string; let hoverState: HoverState; + let shouldPublish: boolean | null = null; switch (type) { case 'setselection': @@ -71,6 +72,7 @@ export const stateReducer = ( mode = SelectionMode.Normal; cursorClass = 'cursor-auto'; hoverState = HoverState.Normal; + shouldPublish = false; break; case 'mousedown': @@ -215,6 +217,11 @@ export const stateReducer = ( throw new Error(); } + // by default, only trigger the callback if the selection or mode has changed. + if (shouldPublish == null) { + shouldPublish = newSelection != prevSelection || mode != prevMode; + } + return { width: width, emptySelectionAction: emptySelectionAction, @@ -226,7 +233,7 @@ export const stateReducer = ( prevMode: prevMode, cursorClass: cursorClass, hoverState: hoverState, - shouldPublish: newSelection != prevSelection || mode != prevMode, + shouldPublish: shouldPublish, }; }; diff --git a/frontend/src/Waveform.tsx b/frontend/src/Waveform.tsx index 87740e9..f5d81f5 100644 --- a/frontend/src/Waveform.tsx +++ b/frontend/src/Waveform.tsx @@ -25,10 +25,15 @@ export const Waveform: React.FC = ({ }: Props) => { const [peaks, setPeaks] = useState>(from([])); const [selectedFrames, setSelectedFrames] = useState({ start: 0, end: 0 }); + + // selectedPixels are the currently selected waveform canvas pixels. They may + // not match the actual selection due to the viewport setting, and probably + // shouldn't be leaking outside of the HudCanvasState. const [selectedPixels, setSelectedPixels] = useState({ start: 0, end: 0, }); + const [positionPixels, setPositionPixels] = useState(0); // effects