Bug fix: prevent incorrect selectionChange callbacks
continuous-integration/drone/push Build is passing Details

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.
This commit is contained in:
Rob Watson 2022-01-29 12:32:39 +01:00
parent bff15098e6
commit 6ba19b3e01
4 changed files with 36 additions and 4 deletions

View File

@ -98,8 +98,6 @@ export const HudCanvas: React.FC<Props> = ({
// 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;
}

View File

@ -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();
});
});
});

View File

@ -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,
};
};

View File

@ -25,10 +25,15 @@ export const Waveform: React.FC<Props> = ({
}: Props) => {
const [peaks, setPeaks] = useState<Observable<number[]>>(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<number | null>(0);
// effects