From 383a71fc252974934c783ac9ef076c8aa79aa62c Mon Sep 17 00:00:00 2001 From: Evan Simkowitz Date: Tue, 3 Sep 2024 11:24:45 -0700 Subject: [PATCH] Fix infinite loop in layoutAtom, improve iconbutton disable code (#306) Fixes an infinite loop in the layoutModel atom synchronization that would cause the atom to update indefinitely when the root node is deleted. Also adds a dedicated `disabled` flag for the IconButton decl so we can disable the onClick handler when the button is disabled. Also updates the Magnify toggle button to use this new flag, so that when there's only one leaf in a layout, the magnify button is disabed. --- cmd/wsh/cmd/wshcmd-root.go | 2 +- frontend/app/block/blockframe.tsx | 30 +++++++++++++++------------ frontend/app/block/blockutil.tsx | 8 +++++-- frontend/app/hook/useLongClick.tsx | 4 ++-- frontend/app/view/webview/webview.tsx | 4 ++-- frontend/layout/lib/layoutModel.ts | 6 +++--- frontend/layout/lib/types.ts | 1 + frontend/types/custom.d.ts | 1 + 8 files changed, 33 insertions(+), 23 deletions(-) diff --git a/cmd/wsh/cmd/wshcmd-root.go b/cmd/wsh/cmd/wshcmd-root.go index 7e53503d..784a9e20 100644 --- a/cmd/wsh/cmd/wshcmd-root.go +++ b/cmd/wsh/cmd/wshcmd-root.go @@ -134,7 +134,7 @@ func validateEasyORef(oref string) error { } _, err := uuid.Parse(oref) if err != nil { - return fmt.Errorf("invalid object reference (must be UUID, or a positive nonzero integer): %v", err) + return fmt.Errorf("invalid object reference (must be UUID, or a positive integer): %v", err) } return nil } diff --git a/frontend/app/block/blockframe.tsx b/frontend/app/block/blockframe.tsx index 8954eaae..9e9818bc 100644 --- a/frontend/app/block/blockframe.tsx +++ b/frontend/app/block/blockframe.tsx @@ -91,12 +91,13 @@ function getViewIconElem(viewIconUnion: string | HeaderIconButton, blockData: Bl } const OptMagnifyButton = React.memo( - ({ magnified, toggleMagnify }: { magnified: boolean; toggleMagnify: () => void }) => { + ({ magnified, toggleMagnify, disabled }: { magnified: boolean; toggleMagnify: () => void; disabled: boolean }) => { const magnifyDecl: HeaderIconButton = { elemtype: "iconbutton", icon: , title: magnified ? "Minimize" : "Magnify", click: toggleMagnify, + disabled, }; return ; } @@ -104,13 +105,15 @@ const OptMagnifyButton = React.memo( function computeEndIcons( viewModel: ViewModel, - magnified: boolean, - toggleMagnify: () => void, - onClose: () => void, + nodeModel: NodeModel, onContextMenu: (e: React.MouseEvent) => void ): JSX.Element[] { const endIconsElem: JSX.Element[] = []; const endIconButtons = util.useAtomValueSafe(viewModel.endIconButtons); + const magnified = jotai.useAtomValue(nodeModel.isMagnified); + const numLeafs = jotai.useAtomValue(nodeModel.numLeafs); + const magnifyDisabled = numLeafs <= 1; + if (endIconButtons && endIconButtons.length > 0) { endIconsElem.push(...endIconButtons.map((button, idx) => )); } @@ -121,12 +124,19 @@ function computeEndIcons( click: onContextMenu, }; endIconsElem.push(); - endIconsElem.push(); + endIconsElem.push( + + ); const closeDecl: HeaderIconButton = { elemtype: "iconbutton", icon: "xmark-large", title: "Close", - click: onClose, + click: nodeModel.onClose, }; endIconsElem.push(); return endIconsElem; @@ -156,13 +166,7 @@ const BlockFrame_Header = ({ [magnified] ); - const endIconsElem = computeEndIcons( - viewModel, - magnified, - nodeModel.toggleMagnify, - nodeModel.onClose, - onContextMenu - ); + const endIconsElem = computeEndIcons(viewModel, nodeModel, onContextMenu); const viewIconElem = getViewIconElem(viewIconUnion, blockData); let preIconButtonElem: JSX.Element = null; if (preIconButton) { diff --git a/frontend/app/block/blockutil.tsx b/frontend/app/block/blockutil.tsx index d4053dda..5c2981c4 100644 --- a/frontend/app/block/blockutil.tsx +++ b/frontend/app/block/blockutil.tsx @@ -139,9 +139,13 @@ export function getBlockHeaderIcon(blockIcon: string, blockData: Block): React.R export const IconButton = React.memo(({ decl, className }: { decl: HeaderIconButton; className?: string }) => { const buttonRef = React.useRef(null); - useLongClick(buttonRef, decl.click, decl.longClick); + useLongClick(buttonRef, decl.click, decl.longClick, decl.disabled); return ( -
+
{typeof decl.icon === "string" ? : decl.icon}
); diff --git a/frontend/app/hook/useLongClick.tsx b/frontend/app/hook/useLongClick.tsx index 21de895b..5b71563f 100644 --- a/frontend/app/hook/useLongClick.tsx +++ b/frontend/app/hook/useLongClick.tsx @@ -3,7 +3,7 @@ import { useCallback, useEffect, useRef, useState } from "react"; -export const useLongClick = (ref, onClick, onLongClick, ms = 300) => { +export const useLongClick = (ref, onClick, onLongClick, disabled = false, ms = 300) => { const timerRef = useRef(null); const [longClickTriggered, setLongClickTriggered] = useState(false); @@ -40,7 +40,7 @@ export const useLongClick = (ref, onClick, onLongClick, ms = 300) => { useEffect(() => { const element = ref.current; - if (!element) return; + if (!element || disabled) return; element.addEventListener("mousedown", startPress); element.addEventListener("mouseup", stopPress); diff --git a/frontend/app/view/webview/webview.tsx b/frontend/app/view/webview/webview.tsx index f07e1307..3bc0212a 100644 --- a/frontend/app/view/webview/webview.tsx +++ b/frontend/app/view/webview/webview.tsx @@ -63,15 +63,15 @@ export class WebViewModel implements ViewModel { return [ { elemtype: "iconbutton", - className: this.shouldDisabledBackButton() ? "disabled" : "", icon: "chevron-left", click: this.handleBack.bind(this), + disabled: this.shouldDisabledBackButton(), }, { elemtype: "iconbutton", - className: this.shouldDisabledForwardButton() ? "disabled" : "", icon: "chevron-right", click: this.handleForward.bind(this), + disabled: this.shouldDisabledForwardButton(), }, { elemtype: "div", diff --git a/frontend/layout/lib/layoutModel.ts b/frontend/layout/lib/layoutModel.ts index 1da6e39f..85663097 100644 --- a/frontend/layout/lib/layoutModel.ts +++ b/frontend/layout/lib/layoutModel.ts @@ -439,7 +439,6 @@ export class LayoutModel { } } else { this.updateTree(); - this.setTreeStateAtom(); } } } @@ -448,7 +447,7 @@ export class LayoutModel { * Set the upstream tree state atom to the value of the local tree state. * @param bumpGeneration Whether to bump the generation of the tree state before setting the atom. */ - setTreeStateAtom(bumpGeneration = true) { + setTreeStateAtom(bumpGeneration = false) { if (bumpGeneration) { this.treeState.generation++; } @@ -460,7 +459,7 @@ export class LayoutModel { * This is a hack to ensure that when the updateTree first successfully runs, we set the upstream atom state to persist the initial leaf order. * @see updateTree should be the only caller of this method. */ - setTreeStateAtomOnce = lazy(() => this.setTreeStateAtom()); + setTreeStateAtomOnce = lazy(() => this.setTreeStateAtom(true)); /** * Recursively walks the tree to find leaf nodes, update the resize handles, and compute additional properties for each node. @@ -761,6 +760,7 @@ export class LayoutModel { const isFocused = treeState.focusedNodeId === nodeid; return isFocused; }), + numLeafs: this.numLeafs, isMagnified: atom((get) => { const treeState = get(this.treeStateAtom); return treeState.magnifiedNodeId === nodeid; diff --git a/frontend/layout/lib/types.ts b/frontend/layout/lib/types.ts index 5c1f45a3..7144a8d8 100644 --- a/frontend/layout/lib/types.ts +++ b/frontend/layout/lib/types.ts @@ -328,6 +328,7 @@ export interface NodeModel { animationTimeS: number; innerRect: Atom; blockNum: Atom; + numLeafs: Atom; nodeId: string; blockId: string; isFocused: Atom; diff --git a/frontend/types/custom.d.ts b/frontend/types/custom.d.ts index 9e201f67..7c5a5bdd 100644 --- a/frontend/types/custom.d.ts +++ b/frontend/types/custom.d.ts @@ -154,6 +154,7 @@ declare global { title?: string; click?: (e: React.MouseEvent) => void; longClick?: (e: React.MouseEvent) => void; + disabled?: boolean; }; type HeaderTextButton = {