Upstream/waveterm
1
0
Fork 0
mirror of https://github.com/wavetermdev/waveterm.git synced 2025-01-17 20:51:55 +01:00
Code Issues Packages Projects Releases Wiki Activity

Fix bug in TileLayout where computeMoveNode had stale node data (#361)

Browse Source 
This was hard to debug. It manifested in nodes that had been collapsed
into their parent. When a node was dragged over the recently-collapsed
node, the placeholder would show the action as if the flex direction was
reversed. It turns out useDrag has trouble tracking changes to the
LayoutNode objects. For now, I am just finding the nodes again in the
computeMoveNode function. Later when I redo the drag system, I will fix
this better.
This commit is contained in:
Evan Simkowitz 2024-09-10 17:23:28 -07:00 committed by GitHub
parent f3940f7456
commit 9b12aa9882
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 43 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
frontend/layout/lib/TileLayout.tsx
View File

@ -303,8 +303,8 @@ const OverlayNode = memo(({ node, layoutModel }: OverlayNodeProps) => {
() => ({ () => ({
accept: dragItemType, accept: dragItemType,
canDrop: (_, monitor) => { canDrop: (_, monitor) => {
const dragItem = monitor.getItem<LayoutNode>(); const dragItemId = monitor.getItem<string>();
if (monitor.isOver({ shallow: true }) && dragItem?.id !== node.id) { if (monitor.isOver({ shallow: true }) && dragItemId !== node.id) {
return true; return true;
} }
return false; return false;
@ -325,8 +325,8 @@ const OverlayNode = memo(({ node, layoutModel }: OverlayNodeProps) => {
offset.y -= containerRect.y; offset.y -= containerRect.y;
layoutModel.treeReducer({ layoutModel.treeReducer({
type: LayoutTreeActionType.ComputeMove, type: LayoutTreeActionType.ComputeMove,
node: node, nodeId: node.id,
nodeToMove: dragItem, nodeToMoveId: dragItem.id,
direction: determineDropDirection(additionalProps.rect, offset), direction: determineDropDirection(additionalProps.rect, offset),
} as LayoutTreeComputeMoveNodeAction); } as LayoutTreeComputeMoveNodeAction);
} else { } else {
@ -337,7 +337,7 @@ const OverlayNode = memo(({ node, layoutModel }: OverlayNodeProps) => {
} }
}), }),
}), }),
[node, additionalProps?.rect, layoutModel.displayContainerRef, layoutModel.onDrop, layoutModel.treeReducer] [node.id, additionalProps?.rect, layoutModel.displayContainerRef, layoutModel.onDrop, layoutModel.treeReducer]
); );
// Register the overlay node as a drop target // Register the overlay node as a drop target

42
frontend/layout/lib/layoutTree.ts
View File

@ -39,27 +39,40 @@ export const DEFAULT_MAX_CHILDREN = 5;
*/ */
export function computeMoveNode(layoutState: LayoutTreeState, computeInsertAction: LayoutTreeComputeMoveNodeAction) { export function computeMoveNode(layoutState: LayoutTreeState, computeInsertAction: LayoutTreeComputeMoveNodeAction) {
const rootNode = layoutState.rootNode; const rootNode = layoutState.rootNode;
const { node, nodeToMove, direction } = computeInsertAction; const { nodeId, nodeToMoveId, direction } = computeInsertAction;
if (!nodeId || !nodeToMoveId) {
console.warn("either nodeId or nodeToMoveId not set", nodeId, nodeToMoveId);
return;
}
if (direction === undefined) { if (direction === undefined) {
console.warn("No direction provided for insertItemInDirection"); console.warn("No direction provided for insertItemInDirection");
return; return;
} }
if (node.id === nodeToMove.id) { if (nodeId === nodeToMoveId) {
console.warn("Cannot compute move node action since both nodes are equal"); console.warn("Cannot compute move node action since both nodes are equal");
return; return;
} }
let newMoveOperation: MoveOperation; let newMoveOperation: MoveOperation;
const parent = lazy(() => findParent(rootNode, node.id)); const parent = lazy(() => findParent(rootNode, nodeId));
const grandparent = lazy(() => findParent(rootNode, parent().id)); const grandparent = lazy(() => findParent(rootNode, parent().id));
const indexInParent = lazy(() => parent()?.children.findIndex((child) => node.id === child.id)); const indexInParent = lazy(() => parent()?.children.findIndex((child) => nodeId === child.id));
const indexInGrandparent = lazy(() => grandparent()?.children.findIndex((child) => parent().id === child.id)); const indexInGrandparent = lazy(() => grandparent()?.children.findIndex((child) => parent().id === child.id));
const nodeToMoveParent = lazy(() => findParent(rootNode, nodeToMove.id)); const nodeToMoveParent = lazy(() => findParent(rootNode, nodeToMoveId));
const nodeToMoveIndexInParent = lazy(() => const nodeToMoveIndexInParent = lazy(() =>
nodeToMoveParent()?.children.findIndex((child) => nodeToMove.id === child.id) nodeToMoveParent()?.children.findIndex((child) => nodeToMoveId === child.id)
); );
const isRoot = rootNode.id === node.id; const isRoot = rootNode.id === nodeId;
// TODO: this should not be necessary. The drag layer is having trouble tracking changes to the LayoutNode fields, so I need to grab the node again here to get the latest data.
const node = findNode(rootNode, nodeId);
const nodeToMove = findNode(rootNode, nodeToMoveId);
if (!node || !nodeToMove) {
console.warn("node or nodeToMove not set", nodeId, nodeToMoveId);
return;
}
switch (direction) { switch (direction) {
case DropDirection.OuterTop: case DropDirection.OuterTop:
@ -77,7 +90,7 @@ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertActio
} }
case DropDirection.Top: case DropDirection.Top:
if (node.flexDirection === FlexDirection.Column) { if (node.flexDirection === FlexDirection.Column) {
newMoveOperation = { parentId: node.id, index: 0, node: nodeToMove }; newMoveOperation = { parentId: nodeId, index: 0, node: nodeToMove };
} else { } else {
if (isRoot) if (isRoot)
newMoveOperation = { newMoveOperation = {
@ -110,7 +123,7 @@ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertActio
} }
case DropDirection.Bottom: case DropDirection.Bottom:
if (node.flexDirection === FlexDirection.Column) { if (node.flexDirection === FlexDirection.Column) {
newMoveOperation = { parentId: node.id, index: 1, node: nodeToMove }; newMoveOperation = { parentId: nodeId, index: 1, node: nodeToMove };
} else { } else {
if (isRoot) if (isRoot)
newMoveOperation = { newMoveOperation = {
@ -143,7 +156,7 @@ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertActio
} }
case DropDirection.Left: case DropDirection.Left:
if (node.flexDirection === FlexDirection.Row) { if (node.flexDirection === FlexDirection.Row) {
newMoveOperation = { parentId: node.id, index: 0, node: nodeToMove }; newMoveOperation = { parentId: nodeId, index: 0, node: nodeToMove };
} else { } else {
const parentNode = parent(); const parentNode = parent();
if (parentNode) if (parentNode)
@ -169,7 +182,7 @@ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertActio
} }
case DropDirection.Right: case DropDirection.Right:
if (node.flexDirection === FlexDirection.Row) { if (node.flexDirection === FlexDirection.Row) {
newMoveOperation = { parentId: node.id, index: 1, node: nodeToMove }; newMoveOperation = { parentId: nodeId, index: 1, node: nodeToMove };
} else { } else {
const parentNode = parent(); const parentNode = parent();
if (parentNode) if (parentNode)
@ -181,11 +194,11 @@ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertActio
} }
break; break;
case DropDirection.Center: case DropDirection.Center:
if (node.id !== rootNode.id && nodeToMove.id !== rootNode.id) { if (nodeId !== rootNode.id && nodeToMoveId !== rootNode.id) {
const swapAction: LayoutTreeSwapNodeAction = { const swapAction: LayoutTreeSwapNodeAction = {
type: LayoutTreeActionType.Swap, type: LayoutTreeActionType.Swap,
node1Id: node.id, node1Id: nodeId,
node2Id: nodeToMove.id, node2Id: nodeToMoveId,
}; };
return swapAction; return swapAction;
} else { } else {
@ -208,6 +221,7 @@ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertActio
} }
export function moveNode(layoutState: LayoutTreeState, action: LayoutTreeMoveNodeAction) { export function moveNode(layoutState: LayoutTreeState, action: LayoutTreeMoveNodeAction) {
console.log("moveNode", layoutState, action);
const rootNode = layoutState.rootNode; const rootNode = layoutState.rootNode;
if (!action) { if (!action) {
console.error("no move node action provided"); console.error("no move node action provided");

4
frontend/layout/lib/types.ts
View File

@ -86,8 +86,8 @@ export interface LayoutTreeAction {
*/ */
export interface LayoutTreeComputeMoveNodeAction extends LayoutTreeAction { export interface LayoutTreeComputeMoveNodeAction extends LayoutTreeAction {
type: LayoutTreeActionType.ComputeMove; type: LayoutTreeActionType.ComputeMove;
node: LayoutNode; nodeId: string;
nodeToMove: LayoutNode; nodeToMoveId: string;
direction: DropDirection; direction: DropDirection;
} }

16
frontend/layout/tests/layoutTree.test.ts
View File

@ -18,8 +18,8 @@ test("layoutTreeStateReducer - compute move", () => {
let node1 = newLayoutNode(undefined, undefined, undefined, { blockId: "node1" }); let node1 = newLayoutNode(undefined, undefined, undefined, { blockId: "node1" });
let pendingAction = computeMoveNode(treeState, { let pendingAction = computeMoveNode(treeState, {
type: LayoutTreeActionType.ComputeMove, type: LayoutTreeActionType.ComputeMove,
node: treeState.rootNode, nodeId: treeState.rootNode.id,
nodeToMove: node1, nodeToMoveId: node1.id,
direction: DropDirection.Bottom, direction: DropDirection.Bottom,
}); });
const insertOperation = pendingAction as LayoutTreeMoveNodeAction; const insertOperation = pendingAction as LayoutTreeMoveNodeAction;
@ -37,8 +37,8 @@ test("layoutTreeStateReducer - compute move", () => {
let node2 = newLayoutNode(undefined, undefined, undefined, { blockId: "node2" }); let node2 = newLayoutNode(undefined, undefined, undefined, { blockId: "node2" });
pendingAction = computeMoveNode(treeState, { pendingAction = computeMoveNode(treeState, {
type: LayoutTreeActionType.ComputeMove, type: LayoutTreeActionType.ComputeMove,
node: node1, nodeId: node1.id,
nodeToMove: node2, nodeToMoveId: node2.id,
direction: DropDirection.Bottom, direction: DropDirection.Bottom,
}); });
const insertOperation2 = pendingAction as LayoutTreeMoveNodeAction; const insertOperation2 = pendingAction as LayoutTreeMoveNodeAction;
@ -64,8 +64,8 @@ test("computeMove - noop action", () => {
); );
let moveAction: LayoutTreeComputeMoveNodeAction = { let moveAction: LayoutTreeComputeMoveNodeAction = {
type: LayoutTreeActionType.ComputeMove, type: LayoutTreeActionType.ComputeMove,
node: treeState.rootNode, nodeId: treeState.rootNode.id,
nodeToMove, nodeToMoveId: nodeToMove.id,
direction: DropDirection.Left, direction: DropDirection.Left,
}; };
let pendingAction = computeMoveNode(treeState, moveAction); let pendingAction = computeMoveNode(treeState, moveAction);
@ -74,8 +74,8 @@ test("computeMove - noop action", () => {
moveAction = { moveAction = {
type: LayoutTreeActionType.ComputeMove, type: LayoutTreeActionType.ComputeMove,
node: treeState.rootNode, nodeId: treeState.rootNode.id,
nodeToMove, nodeToMoveId: nodeToMove.id,
direction: DropDirection.Right, direction: DropDirection.Right,
}; };