From ccbc573139a356680f874b1cad424791a67d9eab Mon Sep 17 00:00:00 2001 From: George Thomas Date: Tue, 21 Mar 2023 20:11:07 +0000 Subject: [PATCH 1/2] (WIP) Use ReactFlow for selection handling, rather than doing it more manually This has enough downsides that it's looking like a failed experiment. Overall, my impression is now more strongly that we should use RF for rendering only, rather than utilising its internal state. At which point, we may be able to get away with a much simpler tree/graph-rendering library. Benefits: - Original motivation: allows us to click on the background to cancel selection. - IDs can no longer pass `isNan`, because such nodes have `selectable: false`. - Slight increase in responsiveness (in theory), since initial selection change is frontend-only. - Opens a door to easy multi-selection. - Slightly simplifies `PrimerNode` type def. Drawbacks/todo: - The `deepEqual` check stops the obvious infinite loops, but feels like a hack, and I've now noticed some major flickering during edits, where selection goes on and off several times before settling. - The use of `useNodesState` is taken from examples, and crucial to this working, but docs say not to use it in production (without really saying why, or what to do instead). - Selecting multiple nodes is now possible (with shift-click and drag). We may not want this. And if we do, we shouldn't be ignoring most of them with `p.onNodeClick(p.nodes[0])`. - Our `assertType` call is getting complex as we use more and more fields from `RFNode`. - Several uses of `// @ts-ignore` - use `as` and encapsulate within `ReactFlowSafe`? - Do we really want to tie ourselves closer to RF anyway? Given that we have some concerns about performance, and other recent work has been done in the opposite direction. --- src/App.tsx | 32 ++++++------ src/components/TreeReactFlow/Types.ts | 3 +- src/components/TreeReactFlow/index.tsx | 69 +++++++++++++++++--------- 3 files changed, 65 insertions(+), 39 deletions(-) diff --git a/src/App.tsx b/src/App.tsx index f1957349..a8c5a07f 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -11,6 +11,7 @@ import { useQueryClient, UseQueryResult, } from "@tanstack/react-query"; +import deepEqual from "deep-equal"; import { DependencyList, useEffect, useState } from "react"; import { useParams } from "react-router-dom"; import { @@ -158,7 +159,7 @@ const AppNoError = ({ module: Module; imports: Module[]; selection: Selection | undefined; - setSelection: (s: Selection) => void; + setSelection: (s: Selection | undefined) => void; setProg: (p: Prog) => void; }): JSX.Element => { const [level, setLevel] = useState(initialLevel); @@ -223,20 +224,21 @@ const AppNoError = ({ { - if (!("nodeType" in node.data)) { - setSelection({ - def: node.data.def, - }); - } else { - const id = Number(node.id); - // Non-numeric IDs correspond to non-selectable nodes (those with no ID in backend) e.g. pattern constructors. - if (!isNaN(id)) { - setSelection({ - def: node.data.def, - node: { id, nodeType: node.data.nodeType }, - }); - } + onNodeClick={(node) => { + const s: Selection | undefined = + node == undefined + ? undefined + : node.type == "primer-def-name" + ? { + def: node.data.def, + } + : { + def: node.data.def, + node: { id: Number(node.id), nodeType: node.data.nodeType }, + }; + // guard needed to avoid infinite loop + if (!deepEqual(selection, s)) { + setSelection(s); } }} defs={p.module.defs} diff --git a/src/components/TreeReactFlow/Types.ts b/src/components/TreeReactFlow/Types.ts index 2b23bc35..72b04121 100644 --- a/src/components/TreeReactFlow/Types.ts +++ b/src/components/TreeReactFlow/Types.ts @@ -116,6 +116,8 @@ export type PrimerEdge = Edge & { zIndex: number }; export type PrimerNode = { id: string; zIndex: number; + selected: boolean; + selectable: boolean; // in the long run, this will be `false` precisely for box nodes data: PrimerCommonNodeProps & T; } & ( | { type: "primer"; data: PrimerNodeProps } @@ -161,7 +163,6 @@ export type PrimerDefNameNodeProps = { export type PrimerCommonNodeProps = { width: number; height: number; - selected: boolean; }; export type Positioned = T & { diff --git a/src/components/TreeReactFlow/index.tsx b/src/components/TreeReactFlow/index.tsx index 2fad41d8..7eb3458a 100644 --- a/src/components/TreeReactFlow/index.tsx +++ b/src/components/TreeReactFlow/index.tsx @@ -15,6 +15,8 @@ import { NodeProps, Background, HandleType, + useNodesState, + useEdgesState, } from "reactflow"; import "./reactflow.css"; import { useEffect, useId, useState } from "react"; @@ -62,10 +64,7 @@ type NodeParams = { }; export type TreeReactFlowProps = { defs: Def[]; - onNodeClick?: ( - event: React.MouseEvent, - node: Positioned - ) => void; + onNodeClick: (node: PrimerNodeWithDef | undefined) => void; treePadding: number; forestLayout: "Horizontal" | "Vertical"; } & NodeParams; @@ -82,8 +81,8 @@ const nodeTypes = {
[0]["data"]; @@ -284,7 +283,6 @@ const makePrimerNode = async ( const common = { width: p.nodeWidth, height: p.nodeHeight, - selected, nodeType, ...p, }; @@ -316,6 +314,8 @@ const makePrimerNode = async ( ...common, }, zIndex, + selectable: true, + selected, }, (child) => ({ className: flavorEdgeClasses(flavor), @@ -337,6 +337,8 @@ const makePrimerNode = async ( ...common, }, zIndex, + selectable: flavor != "PatternCon", + selected, }, (child) => ({ className: flavorEdgeClasses(flavor), @@ -366,6 +368,8 @@ const makePrimerNode = async ( width: 130, }, zIndex, + selectable: flavor != "PatternApp", + selected, }, makeChild, [], @@ -382,6 +386,8 @@ const makePrimerNode = async ( width: common.height, }, zIndex, + selectable: flavor != "PatternApp", + selected, }, makeChild, [], @@ -412,6 +418,8 @@ const makePrimerNode = async ( height: bodyLayout.height + p.boxPadding, }, zIndex, + selectable: false, + selected, }, (child) => ({ className: flavorEdgeClasses(flavor), @@ -444,12 +452,17 @@ type PrimerNodeWithDef = Positioned; // It ensures that these are clearly displayed as "one atomic thing", // i.e. to avoid confused readings that group the type of 'foo' with the body of 'bar' (etc) export const TreeReactFlow = (p: TreeReactFlowProps) => { - const [{ nodes, edges }, setLayout] = useState< - Graph - >({ - nodes: [], - edges: [], - }); + console.log(p.selection); + const [nodes1, setNodes, onNodesChange] = useNodesState([]); + const [edges1, setEdges, onEdgesChange] = useEdgesState([]); + const { nodes, edges }: Graph = { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + nodes: nodes1, + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + edges: edges1, + }; useEffect(() => { (async () => { @@ -471,11 +484,12 @@ export const TreeReactFlow = (p: TreeReactFlowProps) => { def: def.name, height: p.nodeHeight * 2, width: p.nodeWidth * 3, - selected: - deepEqual(p.selection?.def, def.name) && !p.selection?.node, }, type: "primer-def-name", zIndex: 0, + selectable: true, + selected: + deepEqual(p.selection?.def, def.name) && !p.selection?.node, }; const defEdge = async ( tree: APITree, @@ -559,9 +573,11 @@ export const TreeReactFlow = (p: TreeReactFlowProps) => { }, [[], 0] )[0]; - setLayout(combineGraphs([...graphs, ...nested.flat()])); + const { nodes, edges } = combineGraphs([...graphs, ...nested.flat()]); + setNodes(nodes); + setEdges(edges); })(); - }, [p]); + }, [p, setEdges, setNodes]); // ReactFlow requires a unique id to be passed in if there are // multiple flows on one page. We simply get react to generate @@ -570,12 +586,19 @@ export const TreeReactFlow = (p: TreeReactFlowProps) => { return ( { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + p.onNodeClick(ps.nodes[0]); + }} id={id} - {...(p.onNodeClick && { onNodeClick: p.onNodeClick })} nodes={nodes} edges={edges} nodeTypes={nodeTypes} proOptions={{ hideAttribution: true, account: "paid-pro" }} + onNodesChange={onNodesChange} + onEdgesChange={onEdgesChange} + nodesDraggable={false} > From 77efa828f04138927b3ce471bafba69825ca2b56 Mon Sep 17 00:00:00 2001 From: George Thomas Date: Tue, 21 Mar 2023 20:13:02 +0000 Subject: [PATCH 2/2] Hide action panel when no node is selected --- src/App.tsx | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/App.tsx b/src/App.tsx index a8c5a07f..73df44e0 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -187,7 +187,13 @@ const AppNoError = ({ [p.module] ); return ( -
+
) : ( -
- Click something on the canvas to see available actions! -
+ <> )} {showCreateDefModal ? (