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.
This commit is contained in:
Evan Simkowitz 2024-09-03 11:24:45 -07:00 committed by GitHub
parent 0084f8eb97
commit 383a71fc25
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 33 additions and 23 deletions

View File

@ -134,7 +134,7 @@ func validateEasyORef(oref string) error {
} }
_, err := uuid.Parse(oref) _, err := uuid.Parse(oref)
if err != nil { 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 return nil
} }

View File

@ -91,12 +91,13 @@ function getViewIconElem(viewIconUnion: string | HeaderIconButton, blockData: Bl
} }
const OptMagnifyButton = React.memo( const OptMagnifyButton = React.memo(
({ magnified, toggleMagnify }: { magnified: boolean; toggleMagnify: () => void }) => { ({ magnified, toggleMagnify, disabled }: { magnified: boolean; toggleMagnify: () => void; disabled: boolean }) => {
const magnifyDecl: HeaderIconButton = { const magnifyDecl: HeaderIconButton = {
elemtype: "iconbutton", elemtype: "iconbutton",
icon: <MagnifyIcon enabled={magnified} />, icon: <MagnifyIcon enabled={magnified} />,
title: magnified ? "Minimize" : "Magnify", title: magnified ? "Minimize" : "Magnify",
click: toggleMagnify, click: toggleMagnify,
disabled,
}; };
return <IconButton key="magnify" decl={magnifyDecl} className="block-frame-magnify" />; return <IconButton key="magnify" decl={magnifyDecl} className="block-frame-magnify" />;
} }
@ -104,13 +105,15 @@ const OptMagnifyButton = React.memo(
function computeEndIcons( function computeEndIcons(
viewModel: ViewModel, viewModel: ViewModel,
magnified: boolean, nodeModel: NodeModel,
toggleMagnify: () => void,
onClose: () => void,
onContextMenu: (e: React.MouseEvent<HTMLDivElement>) => void onContextMenu: (e: React.MouseEvent<HTMLDivElement>) => void
): JSX.Element[] { ): JSX.Element[] {
const endIconsElem: JSX.Element[] = []; const endIconsElem: JSX.Element[] = [];
const endIconButtons = util.useAtomValueSafe(viewModel.endIconButtons); 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) { if (endIconButtons && endIconButtons.length > 0) {
endIconsElem.push(...endIconButtons.map((button, idx) => <IconButton key={idx} decl={button} />)); endIconsElem.push(...endIconButtons.map((button, idx) => <IconButton key={idx} decl={button} />));
} }
@ -121,12 +124,19 @@ function computeEndIcons(
click: onContextMenu, click: onContextMenu,
}; };
endIconsElem.push(<IconButton key="settings" decl={settingsDecl} className="block-frame-settings" />); endIconsElem.push(<IconButton key="settings" decl={settingsDecl} className="block-frame-settings" />);
endIconsElem.push(<OptMagnifyButton key="unmagnify" magnified={magnified} toggleMagnify={toggleMagnify} />); endIconsElem.push(
<OptMagnifyButton
key="unmagnify"
magnified={magnified}
toggleMagnify={nodeModel.toggleMagnify}
disabled={magnifyDisabled}
/>
);
const closeDecl: HeaderIconButton = { const closeDecl: HeaderIconButton = {
elemtype: "iconbutton", elemtype: "iconbutton",
icon: "xmark-large", icon: "xmark-large",
title: "Close", title: "Close",
click: onClose, click: nodeModel.onClose,
}; };
endIconsElem.push(<IconButton key="close" decl={closeDecl} className="block-frame-default-close" />); endIconsElem.push(<IconButton key="close" decl={closeDecl} className="block-frame-default-close" />);
return endIconsElem; return endIconsElem;
@ -156,13 +166,7 @@ const BlockFrame_Header = ({
[magnified] [magnified]
); );
const endIconsElem = computeEndIcons( const endIconsElem = computeEndIcons(viewModel, nodeModel, onContextMenu);
viewModel,
magnified,
nodeModel.toggleMagnify,
nodeModel.onClose,
onContextMenu
);
const viewIconElem = getViewIconElem(viewIconUnion, blockData); const viewIconElem = getViewIconElem(viewIconUnion, blockData);
let preIconButtonElem: JSX.Element = null; let preIconButtonElem: JSX.Element = null;
if (preIconButton) { if (preIconButton) {

View File

@ -139,9 +139,13 @@ export function getBlockHeaderIcon(blockIcon: string, blockData: Block): React.R
export const IconButton = React.memo(({ decl, className }: { decl: HeaderIconButton; className?: string }) => { export const IconButton = React.memo(({ decl, className }: { decl: HeaderIconButton; className?: string }) => {
const buttonRef = React.useRef<HTMLDivElement>(null); const buttonRef = React.useRef<HTMLDivElement>(null);
useLongClick(buttonRef, decl.click, decl.longClick); useLongClick(buttonRef, decl.click, decl.longClick, decl.disabled);
return ( return (
<div ref={buttonRef} className={clsx("iconbutton", className)} title={decl.title}> <div
ref={buttonRef}
className={clsx("iconbutton", className, decl.className, { disabled: decl.disabled })}
title={decl.title}
>
{typeof decl.icon === "string" ? <i className={util.makeIconClass(decl.icon, true)} /> : decl.icon} {typeof decl.icon === "string" ? <i className={util.makeIconClass(decl.icon, true)} /> : decl.icon}
</div> </div>
); );

View File

@ -3,7 +3,7 @@
import { useCallback, useEffect, useRef, useState } from "react"; 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 timerRef = useRef(null);
const [longClickTriggered, setLongClickTriggered] = useState(false); const [longClickTriggered, setLongClickTriggered] = useState(false);
@ -40,7 +40,7 @@ export const useLongClick = (ref, onClick, onLongClick, ms = 300) => {
useEffect(() => { useEffect(() => {
const element = ref.current; const element = ref.current;
if (!element) return; if (!element || disabled) return;
element.addEventListener("mousedown", startPress); element.addEventListener("mousedown", startPress);
element.addEventListener("mouseup", stopPress); element.addEventListener("mouseup", stopPress);

View File

@ -63,15 +63,15 @@ export class WebViewModel implements ViewModel {
return [ return [
{ {
elemtype: "iconbutton", elemtype: "iconbutton",
className: this.shouldDisabledBackButton() ? "disabled" : "",
icon: "chevron-left", icon: "chevron-left",
click: this.handleBack.bind(this), click: this.handleBack.bind(this),
disabled: this.shouldDisabledBackButton(),
}, },
{ {
elemtype: "iconbutton", elemtype: "iconbutton",
className: this.shouldDisabledForwardButton() ? "disabled" : "",
icon: "chevron-right", icon: "chevron-right",
click: this.handleForward.bind(this), click: this.handleForward.bind(this),
disabled: this.shouldDisabledForwardButton(),
}, },
{ {
elemtype: "div", elemtype: "div",

View File

@ -439,7 +439,6 @@ export class LayoutModel {
} }
} else { } else {
this.updateTree(); 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. * 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. * @param bumpGeneration Whether to bump the generation of the tree state before setting the atom.
*/ */
setTreeStateAtom(bumpGeneration = true) { setTreeStateAtom(bumpGeneration = false) {
if (bumpGeneration) { if (bumpGeneration) {
this.treeState.generation++; 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. * 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. * @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. * 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; const isFocused = treeState.focusedNodeId === nodeid;
return isFocused; return isFocused;
}), }),
numLeafs: this.numLeafs,
isMagnified: atom((get) => { isMagnified: atom((get) => {
const treeState = get(this.treeStateAtom); const treeState = get(this.treeStateAtom);
return treeState.magnifiedNodeId === nodeid; return treeState.magnifiedNodeId === nodeid;

View File

@ -328,6 +328,7 @@ export interface NodeModel {
animationTimeS: number; animationTimeS: number;
innerRect: Atom<CSSProperties>; innerRect: Atom<CSSProperties>;
blockNum: Atom<number>; blockNum: Atom<number>;
numLeafs: Atom<number>;
nodeId: string; nodeId: string;
blockId: string; blockId: string;
isFocused: Atom<boolean>; isFocused: Atom<boolean>;

View File

@ -154,6 +154,7 @@ declare global {
title?: string; title?: string;
click?: (e: React.MouseEvent<any>) => void; click?: (e: React.MouseEvent<any>) => void;
longClick?: (e: React.MouseEvent<any>) => void; longClick?: (e: React.MouseEvent<any>) => void;
disabled?: boolean;
}; };
type HeaderTextButton = { type HeaderTextButton = {