From 14f3a58389e15c5e019cf0055e02dfa73f7e1874 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Pardou-Piquemal?= Date: Sun, 20 Feb 2022 23:03:06 +0100 Subject: [PATCH] Improve stability (#4) * Hardened code to avoid bugs --- src/board/ActionPane.jsx | 4 +- src/board/Cursors/Cursor.jsx | 3 + src/board/Gesture.jsx | 208 +++++++++++++++-------------- src/board/Items/useItemActions.js | 209 ++++++++++++++++-------------- src/board/Selector.jsx | 26 ++-- src/stories/MainView.stories.jsx | 4 +- src/users/useUsers.jsx | 9 +- src/utils.js | 27 +++- 8 files changed, 268 insertions(+), 222 deletions(-) diff --git a/src/board/ActionPane.jsx b/src/board/ActionPane.jsx index 1fcd8ff..a0f9f4b 100644 --- a/src/board/ActionPane.jsx +++ b/src/board/ActionPane.jsx @@ -8,7 +8,7 @@ import { BoardConfigAtom, } from "./atoms"; import { useItemActions } from "./Items"; -import { insideClass, hasClass } from "../utils"; +import { insideClass, hasClass, getIdFromElem } from "../utils"; import Gesture from "./Gesture"; @@ -41,7 +41,7 @@ const ActionPane = ({ children }) => { selectedItemRef.current.items = selectedItems; - const itemId = foundElement.dataset.id; + const itemId = getIdFromElem(foundElement); if (!selectedItems.includes(itemId)) { if (ctrlKey || metaKey) { diff --git a/src/board/Cursors/Cursor.jsx b/src/board/Cursors/Cursor.jsx index 8cc9638..a79b9a1 100644 --- a/src/board/Cursors/Cursor.jsx +++ b/src/board/Cursors/Cursor.jsx @@ -9,6 +9,7 @@ const StyledCursor = styled.div` flex-direction: row; align-items: center; z-index: 210; + pointer-events: none; `; const CursorName = styled.div` @@ -22,6 +23,7 @@ const CursorName = styled.div` margin-left: -0.5em; margin-top: 1.7em; whitespace: nowrap; + pointer-events: none; background-color: ${({ color }) => color}; `; @@ -70,6 +72,7 @@ const PositionnedCursor = ({ pos, ...rest }) => ( left: 0, zIndex: 210, position: "fixed", + pointerEvents: "none", }} > diff --git a/src/board/Gesture.jsx b/src/board/Gesture.jsx index bfee042..a7900b1 100644 --- a/src/board/Gesture.jsx +++ b/src/board/Gesture.jsx @@ -45,6 +45,17 @@ const computeDistance = ([x1, y1], [x2, y2]) => { const empty = () => {}; +const protect = + (fn) => + async (...args) => { + try { + await fn(...args); + } catch (e) { + // eslint-disable-next-line no-console + console.error(e); + } + }; + const Gesture = ({ children, onDrag = empty, @@ -65,16 +76,16 @@ const Gesture = ({ const queueRef = React.useRef([]); // Queue event to avoid async mess - const queue = React.useCallback((callback) => { + const queue = React.useCallback((callback, args) => { queueRef.current.push(async () => { - await callback(); + await protect(callback)(args); queueRef.current.shift(); if (queueRef.current.length !== 0) { - await queueRef.current[0](); + await protect(queueRef.current[0])(); } }); if (queueRef.current.length === 1) { - queueRef.current[0](); + protect(queueRef.current[0])(); } }, []); @@ -104,17 +115,15 @@ const Gesture = ({ // If we are only moving the fingers in the same direction, a pan is needed. // Ref: https://medium.com/@auchenberg/detecting-multi-touch-trackpad-gestures-in-javascript-a2505babb10e if (isMacOS() && !ctrlKey) { - queue(() => - onPan({ - deltaX: -2 * deltaX, - deltaY: -2 * deltaY, - button: 1, - ctrlKey, - metaKey, - target, - event: e, - }) - ); + queue(onPan, { + deltaX: -2 * deltaX, + deltaY: -2 * deltaY, + button: 1, + ctrlKey, + metaKey, + target, + event: e, + }); } else { // Quit if onZoom is not set if (onZoom === undefined || !deltaY) return; @@ -135,7 +144,7 @@ const Gesture = ({ scale *= 2; } - queue(() => onZoom({ scale, clientX, clientY, event: e })); + queue(onZoom, { scale, clientX, clientY, event: e }); } }, [onPan, onZoom, queue] @@ -190,6 +199,7 @@ const Gesture = ({ prevDistance: distance, }); } catch (e) { + // eslint-disable-next-line no-console console.log("Error while getting other pointer. Ignoring", e); // eslint-disable-next-line no-unused-expressions stateRef.current.mainPointer === undefined; @@ -216,22 +226,21 @@ const Gesture = ({ timeStart: Date.now(), longTapTimeout: setTimeout(async () => { stateRef.current.noTap = true; - queue(() => - onLongTap({ - clientX, - clientY, - altKey, - ctrlKey, - metaKey, - target, - }) - ); + queue(onLongTap, { + clientX, + clientY, + altKey, + ctrlKey, + metaKey, + target, + }); }, 750), }); try { target.setPointerCapture(pointerId); } catch (e) { + // eslint-disable-next-line no-console console.log("Fail to capture pointer", e); } }, @@ -307,46 +316,42 @@ const Gesture = ({ // Clear tap timeout clearTimeout(stateRef.current.longTapTimeout); - queue(() => - onDragStart({ - deltaX: 0, - deltaY: 0, - startX: stateRef.current.startX, - startY: stateRef.current.startY, - distanceX: 0, - distanceY: 0, - button: stateRef.current.currentButton, - altKey, - ctrlKey, - metaKey, - target: stateRef.current.target, - event: e, - }) - ); - } - // Create closure - const deltaX = clientX - stateRef.current.prevX; - const deltaY = clientY - stateRef.current.prevY; - const distanceX = clientX - stateRef.current.startX; - const distanceY = clientY - stateRef.current.startY; - - // Drag event - queue(() => - onDrag({ - deltaX, - deltaY, + queue(onDragStart, { + deltaX: 0, + deltaY: 0, startX: stateRef.current.startX, startY: stateRef.current.startY, - distanceX, - distanceY, + distanceX: 0, + distanceY: 0, button: stateRef.current.currentButton, altKey, ctrlKey, metaKey, target: stateRef.current.target, event: e, - }) - ); + }); + } + // Create closure + const deltaX = clientX - stateRef.current.prevX; + const deltaY = clientY - stateRef.current.prevY; + const distanceX = clientX - stateRef.current.startX; + const distanceY = clientY - stateRef.current.startY; + + // Drag event + queue(onDrag, { + deltaX, + deltaY, + startX: stateRef.current.startX, + startY: stateRef.current.startY, + distanceX, + distanceY, + button: stateRef.current.currentButton, + altKey, + ctrlKey, + metaKey, + target: stateRef.current.target, + event: e, + }); } else { if (!stateRef.current.gestureStart) { wrapperRef.current.style.cursor = "move"; @@ -361,18 +366,16 @@ const Gesture = ({ const { target } = stateRef.current; // Pan event - queue(() => - onPan({ - deltaX, - deltaY, - button: stateRef.current.currentButton, - altKey, - ctrlKey, - metaKey, - target, - event: e, - }) - ); + queue(onPan, { + deltaX, + deltaY, + button: stateRef.current.currentButton, + altKey, + ctrlKey, + metaKey, + target, + event: e, + }); if ( twoFingers && @@ -382,14 +385,12 @@ const Gesture = ({ const scale = stateRef.current.prevDistance - distance; if (Math.abs(scale) > 0) { - queue(() => - onZoom({ - scale, - clientX, - clientY, - event: e, - }) - ); + queue(onZoom, { + scale, + clientX, + clientY, + event: e, + }); stateRef.current.prevDistance = distance; } } @@ -432,6 +433,7 @@ const Gesture = ({ ); return; } catch (error) { + // eslint-disable-next-line no-console console.log("Fails to set pointer capture", error); stateRef.current.mainPointer = undefined; delete stateRef.current.pointers[ @@ -449,21 +451,19 @@ const Gesture = ({ if (stateRef.current.moving) { // If we were moving, send drag end event stateRef.current.moving = false; - queue(() => - onDragEnd({ - deltaX: clientX - stateRef.current.prevX, - deltaY: clientY - stateRef.current.prevY, - startX: stateRef.current.startX, - startY: stateRef.current.startY, - distanceX: clientX - stateRef.current.startX, - distanceY: clientY - stateRef.current.startY, - button: stateRef.current.currentButton, - altKey, - ctrlKey, - metaKey, - event: e, - }) - ); + queue(onDragEnd, { + deltaX: clientX - stateRef.current.prevX, + deltaY: clientY - stateRef.current.prevY, + startX: stateRef.current.startX, + startY: stateRef.current.startY, + distanceX: clientX - stateRef.current.startX, + distanceY: clientY - stateRef.current.startY, + button: stateRef.current.currentButton, + altKey, + ctrlKey, + metaKey, + event: e, + }); wrapperRef.current.style.cursor = "auto"; } else { const now = Date.now(); @@ -473,16 +473,14 @@ const Gesture = ({ } // Send tap event only if time less than 300ms else if (stateRef.current.timeStart - now < 300) { - queue(() => - onTap({ - clientX, - clientY, - altKey, - ctrlKey, - metaKey, - target, - }) - ); + queue(onTap, { + clientX, + clientY, + altKey, + ctrlKey, + metaKey, + target, + }); } } }, @@ -491,9 +489,9 @@ const Gesture = ({ const onDoubleTapHandler = React.useCallback( (event) => { - onDoubleTap(event); + queue(onDoubleTap, event); }, - [onDoubleTap] + [onDoubleTap, queue] ); return ( diff --git a/src/board/Items/useItemActions.js b/src/board/Items/useItemActions.js index 845538c..6625cf6 100644 --- a/src/board/Items/useItemActions.js +++ b/src/board/Items/useItemActions.js @@ -10,7 +10,11 @@ import { AllItemsSelector, } from "../atoms"; import useDim from "../useDim"; + +import { getItemElem } from "../../utils"; + import useItemInteraction from "./useItemInteraction"; +import { ConfigurationAtom } from ".."; const useItemActions = () => { const { wire } = useWire("board"); @@ -82,6 +86,10 @@ const useItemActions = () => { itemIds.forEach((id) => { const item = prevItemMap[id]; + if (!item) { + return; + } + result[id] = { ...item, x: (item.x || 0) + posDelta.x, @@ -123,106 +131,115 @@ const useItemActions = () => { [setItemList, wire] ); - const stickOnGrid = React.useCallback( - (itemIds, { type: globalType, size: globalSize } = {}, sync = true) => { - const updatedItems = {}; - setItemMap((prevItemMap) => { - const result = { ...prevItemMap }; - itemIds.forEach((id) => { - const item = prevItemMap[id]; - const elems = document.getElementsByClassName(`item ${id}`); - const elem = elems[0]; - - const { type: itemType, size: itemSize } = item.grid || {}; - let type = globalType; - let size = globalSize || 1; - // If item specific - if (itemType) { - type = itemType; - size = itemSize || 1; - } + const stickOnGrid = useRecoilCallback( + ({ snapshot }) => + async ( + itemIds, + { type: globalType, size: globalSize } = {}, + sync = true + ) => { + const { boardWrapper } = await snapshot.getPromise(ConfigurationAtom); + const updatedItems = {}; + setItemMap((prevItemMap) => { + const result = { ...prevItemMap }; + itemIds.forEach((id) => { + const item = prevItemMap[id]; + const elem = getItemElem(boardWrapper, id); - const [centerX, centerY] = [ - item.x + elem.clientWidth / 2, - item.y + elem.clientHeight / 2, - ]; - - let newX; - let newY; - let sizeX; - let sizeY; - let px1; - let px2; - let py1; - let py2; - let diff1; - let diff2; - const h = size / 1.1547; - - switch (type) { - case "grid": - newX = Math.round(centerX / size) * size; - newY = Math.round(centerY / size) * size; - break; - case "hexH": - sizeX = 2 * h; - sizeY = 3 * size; - px1 = Math.round(centerX / sizeX) * sizeX; - py1 = Math.round(centerY / sizeY) * sizeY; - - px2 = px1 > centerX ? px1 - h : px1 + h; - py2 = py1 > centerY ? py1 - 1.5 * size : py1 + 1.5 * size; - - diff1 = Math.hypot(...[px1 - centerX, py1 - centerY]); - diff2 = Math.hypot(...[px2 - centerX, py2 - centerY]); - - if (diff1 < diff2) { - newX = px1; - newY = py1; - } else { - newX = px2; - newY = py2; - } - break; - case "hexV": - sizeX = 3 * size; - sizeY = 2 * h; - px1 = Math.round(centerX / sizeX) * sizeX; - py1 = Math.round(centerY / sizeY) * sizeY; - - px2 = px1 > centerX ? px1 - 1.5 * size : px1 + 1.5 * size; - py2 = py1 > centerY ? py1 - h : py1 + h; - - diff1 = Math.hypot(...[px1 - centerX, py1 - centerY]); - diff2 = Math.hypot(...[px2 - centerX, py2 - centerY]); - - if (diff1 < diff2) { - newX = px1; - newY = py1; - } else { - newX = px2; - newY = py2; - } - break; - default: - newX = item.x + elem.clientWidth / 2; - newY = item.y + elem.clientHeight / 2; - } + if (!elem) { + return; + } - result[id] = { - ...item, - x: newX - elem.clientWidth / 2, - y: newY - elem.clientHeight / 2, - }; - updatedItems[id] = result[id]; + const { type: itemType, size: itemSize } = item.grid || {}; + let type = globalType; + let size = globalSize || 1; + // If item specific + if (itemType) { + type = itemType; + size = itemSize || 1; + } + + const [centerX, centerY] = [ + item.x + elem.clientWidth / 2, + item.y + elem.clientHeight / 2, + ]; + + let newX; + let newY; + let sizeX; + let sizeY; + let px1; + let px2; + let py1; + let py2; + let diff1; + let diff2; + const h = size / 1.1547; + + switch (type) { + case "grid": + newX = Math.round(centerX / size) * size; + newY = Math.round(centerY / size) * size; + break; + case "hexH": + sizeX = 2 * h; + sizeY = 3 * size; + px1 = Math.round(centerX / sizeX) * sizeX; + py1 = Math.round(centerY / sizeY) * sizeY; + + px2 = px1 > centerX ? px1 - h : px1 + h; + py2 = py1 > centerY ? py1 - 1.5 * size : py1 + 1.5 * size; + + diff1 = Math.hypot(...[px1 - centerX, py1 - centerY]); + diff2 = Math.hypot(...[px2 - centerX, py2 - centerY]); + + if (diff1 < diff2) { + newX = px1; + newY = py1; + } else { + newX = px2; + newY = py2; + } + break; + case "hexV": + sizeX = 3 * size; + sizeY = 2 * h; + px1 = Math.round(centerX / sizeX) * sizeX; + py1 = Math.round(centerY / sizeY) * sizeY; + + px2 = px1 > centerX ? px1 - 1.5 * size : px1 + 1.5 * size; + py2 = py1 > centerY ? py1 - h : py1 + h; + + diff1 = Math.hypot(...[px1 - centerX, py1 - centerY]); + diff2 = Math.hypot(...[px2 - centerX, py2 - centerY]); + + if (diff1 < diff2) { + newX = px1; + newY = py1; + } else { + newX = px2; + newY = py2; + } + break; + default: + newX = item.x + elem.clientWidth / 2; + newY = item.y + elem.clientHeight / 2; + } + + result[id] = { + ...item, + x: newX - elem.clientWidth / 2, + y: newY - elem.clientHeight / 2, + }; + updatedItems[id] = result[id]; + }); + return result; }); - return result; - }); - if (sync) { - wire.publish("batchItemsUpdate", updatedItems); - } - }, + if (sync) { + wire.publish("batchItemsUpdate", updatedItems); + } + }, [wire, setItemMap] ); diff --git a/src/board/Selector.jsx b/src/board/Selector.jsx index c9ede5c..d78b90c 100644 --- a/src/board/Selector.jsx +++ b/src/board/Selector.jsx @@ -2,7 +2,7 @@ import React from "react"; import { useThrottledEffect } from "@react-hookz/web/esm"; import { useSetRecoilState, useRecoilCallback } from "recoil"; -import { insideClass, isItemInsideElement } from "../utils"; +import { insideClass, isItemInsideElement, getIdFromElem } from "../utils"; import { BoardTransformAtom, @@ -31,19 +31,15 @@ const findSelected = (itemMap, wrapper) => { return Array.from(wrapper.getElementsByClassName("item")) .filter((elem) => { - const { id } = elem.dataset; + const id = getIdFromElem(elem); + const item = itemMap[id]; - if (!item) { - // Avoid to find item that are not yet removed from DOM - console.error(`Missing item ${id}`); - return false; - } - if (item.locked) { + if (!item || item.locked) { return false; } return isItemInsideElement(elem, selector); }) - .map((elem) => elem.dataset.id); + .map((elem) => getIdFromElem(elem)); }; const Selector = ({ children, moveFirst }) => { @@ -151,7 +147,8 @@ const Selector = ({ children, moveFirst }) => { ({ target }) => { const foundElement = insideClass(target, "item"); if (foundElement) { - setSelected([foundElement.dataset.id]); + const id = getIdFromElem(foundElement); + setSelected([id]); } }, [setSelected] @@ -167,7 +164,14 @@ const Selector = ({ children, moveFirst }) => { ) { setSelected([]); } else { - const itemId = foundItem.dataset.id; + const itemId = getIdFromElem(foundItem); + + // Being defensive here to avoid bug + if (!itemId) { + setSelected([]); + return; + } + const selectedItems = await snapshot.getPromise(SelectedItemsAtom); if (foundItem && !selectedItems.includes(itemId)) { if (ctrlKey || metaKey) { diff --git a/src/stories/MainView.stories.jsx b/src/stories/MainView.stories.jsx index feef710..afe6fc4 100644 --- a/src/stories/MainView.stories.jsx +++ b/src/stories/MainView.stories.jsx @@ -171,7 +171,7 @@ export const OneView = (props) => ( ); OneView.args = { - moveFirst: true, + moveFirst: false, hideMenu: false, room: nanoid(), session: nanoid(), @@ -231,7 +231,7 @@ export const OneViewWithRoom = (props) => ( ); OneViewWithRoom.args = { - moveFirst: true, + moveFirst: false, hideMenu: false, room: nanoid(), session: nanoid(), diff --git a/src/users/useUsers.jsx b/src/users/useUsers.jsx index fde7b93..174a5dd 100644 --- a/src/users/useUsers.jsx +++ b/src/users/useUsers.jsx @@ -25,10 +25,11 @@ const useUsers = () => { [setCurrentUserState] ); - const localUsers = React.useMemo( - () => users.filter(({ space }) => space === currentUser.space), - [currentUser.space, users] - ); + const localUsers = React.useMemo(() => { + // eslint-disable-next-line no-console + console.debug(`Curent user is in space ${currentUser.space}`); + return users.filter(({ space }) => space === currentUser.space); + }, [currentUser.space, users]); return { currentUser, setCurrentUser, users, localUsers }; }; diff --git a/src/utils.js b/src/utils.js index f611cfc..8ab8479 100644 --- a/src/utils.js +++ b/src/utils.js @@ -43,10 +43,33 @@ export const isItemInsideElement = (itemElement, otherElem) => { }); }; +export const getItemElem = (wrapper, itemId) => { + const elems = wrapper.getElementsByClassName(`item ${itemId}`); + const elem = elems[0]; + if (!elem) { + // eslint-disable-next-line no-console + console.error(`Missing item ${itemId}`); + } + return elem; +}; + +export const getIdFromElem = (elem) => { + const value = elem?.dataset?.id; + if (!value) { + // eslint-disable-next-line no-console + console.error( + "getIdFromElem call fails", + elem, + JSON.stringify(elem?.dataset), + elem?.dataset?.id + ); + } + return value; +}; + export const getItemBoundingBox = (items, wrapper = document) => { const result = items.reduce((prev, itemId) => { - const elems = wrapper.getElementsByClassName(`item ${itemId}`); - const elem = elems[0]; + const elem = getItemElem(wrapper, itemId); if (!elem) return null;