Skip to content

Commit

Permalink
Less dom queries in grid strategies (#6377)
Browse files Browse the repository at this point in the history
Followup to #6373
I continued the work to
- remove dom api queries from strategies
- migrate from window points and rectangle to canvas ones.
- unify the cell query functions, because we have more than 5 functions
which basically do the same thing (return cell data under the a point)

**Fix:**
- Replaced all `TargetGridCellData` usage with the new
`TargetGridCellDataCanvas`, which contains canvas and not window bounds
(and renamed that back to `TargetGridCellData`)
- Deleted a bunch of similar query functions which returned cell data
under a certain point (`getTargetCell`, `getGridCellUnderMouse`,
`getGridCellUnderMouseRecursive`, `getTargetGridCellUnderCursor`, etc),
and replaced them all with `getGridCellUnderMouseFromMetadata `
- Updated grid-resize-element, grid-rearrange-move,
grid-rearrange-move-duplicate, grid-draw-to-insert to use
`getGridCellUnderMouseFromMetadata`
- I had some 1-pixel differences in draw-to-insert tests... not sure
why, maybe a rounding error in the canvas<->window conversions which I
removed. I updated the tests, I think draw to insert works fine.

**Next step:**
- [ ] Continue deleting/converting grid query functions to metadata:
`getGridCellBoundsFromCanvas`, `getGridCellAtPoint`, `getCellWindowRect`

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Preview mode
  • Loading branch information
gbalint authored Sep 17, 2024
1 parent 1697de8 commit 57d569c
Show file tree
Hide file tree
Showing 9 changed files with 59 additions and 143 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -324,9 +324,9 @@ export var storyboard = (
gridColumn: '2',
gridRow: '1',
height: '60px',
left: '83px',
left: '84px',
position: 'absolute',
top: '150px',
top: '151px',
width: '40px',
})
})
Expand Down Expand Up @@ -371,9 +371,9 @@ export var storyboard = (
gridColumn: '1',
gridRow: '1',
height: '30px',
left: '125px',
left: '126px',
position: 'absolute',
top: '75px',
top: '76px',
width: '20px',
})
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import * as EP from '../../../../core/shared/element-path'
import {
getGlobalFramesOfGridCells,
type GridCellGlobalFrames,
type TargetGridCellDataCanvas,
type TargetGridCellData,
} from './grid-helpers'
import type { CanvasPoint } from '../../../../core/shared/math-utils'

Expand All @@ -36,7 +36,7 @@ export function gridCellTargetId(
export function getGridCellUnderMouseFromMetadata(
grid: ElementInstanceMetadata,
canvasPoint: CanvasPoint,
): TargetGridCellDataCanvas | null {
): TargetGridCellData | null {
const gridCellGlobalFrames = getGlobalFramesOfGridCells(grid)

if (gridCellGlobalFrames == null) {
Expand All @@ -50,7 +50,7 @@ export function getGridCellUnderMouseFromMetadata(
function getGridCellUnderPoint(
gridCellGlobalFrames: GridCellGlobalFrames,
canvasPoint: CanvasPoint,
): TargetGridCellDataCanvas | null {
): TargetGridCellData | null {
for (let i = 0; i < gridCellGlobalFrames.length; i++) {
for (let j = 0; j < gridCellGlobalFrames[i].length; j++) {
if (rectContainsPoint(gridCellGlobalFrames[i][j], canvasPoint)) {
Expand All @@ -64,15 +64,6 @@ function getGridCellUnderPoint(
return null
}

// TODO: should be superseded by getGridCellUnderMouseFromMetadata
export function getGridCellUnderMouse(mousePoint: WindowPoint) {
return getGridCellAtPoint(mousePoint, false)
}

export function getGridCellUnderMouseRecursive(mousePoint: WindowPoint) {
return getGridCellAtPoint(mousePoint, true)
}

function isGridCellTargetId(id: string): boolean {
return id.startsWith(gridCellTargetIdPrefix)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { showGridControls } from '../../commands/show-grid-controls-command'
import { updateHighlightedViews } from '../../commands/update-highlighted-views-command'
import { wildcardPatch } from '../../commands/wildcard-patch-command'
import { controlsForGridPlaceholders } from '../../controls/grid-controls'
import { canvasPointToWindowPoint, windowToCanvasCoordinates } from '../../dom-lookup'
import { canvasPointToWindowPoint } from '../../dom-lookup'
import {
getWrapperWithGeneratedUid,
getWrappingCommands,
Expand All @@ -43,9 +43,10 @@ import {
getStyleAttributesForFrameInAbsolutePosition,
updateInsertionSubjectWithAttributes,
} from './draw-to-insert-metastrategy'
import { getTargetCell, setGridPropsCommands } from './grid-helpers'
import { setGridPropsCommands } from './grid-helpers'
import { newReparentSubjects } from './reparent-helpers/reparent-strategy-helpers'
import { getReparentTargetUnified } from './reparent-helpers/reparent-strategy-parent-lookup'
import { getGridCellUnderMouseFromMetadata } from './grid-cell-bounds'

export const gridDrawToInsertText: CanvasStrategyFactory = (
canvasState: InteractionCanvasState,
Expand Down Expand Up @@ -150,11 +151,10 @@ const gridDrawToInsertStrategyInner =
controlsToRender: [controlsForGridPlaceholders(targetParent)],
fitness: 5,
apply: (strategyLifecycle) => {
const newTargetCell = getGridCellUnderCursor(
interactionData,
canvasState,
customStrategyState,
)
const canvasPointToUse =
interactionData.type === 'DRAG' ? interactionData.dragStart : interactionData.point

const newTargetCell = getGridCellUnderMouseFromMetadata(parent, canvasPointToUse)

if (strategyLifecycle === 'mid-interaction' && interactionData.type === 'HOVER') {
return strategyApplicationResult(
Expand Down Expand Up @@ -182,13 +182,7 @@ const gridDrawToInsertStrategyInner =
return emptyStrategyApplicationResult
}

const { gridCellCoordinates, cellWindowRectangle } = newTargetCell

const cellCanvasOrigin = windowToCanvasCoordinates(
canvasState.scale,
canvasState.canvasOffset,
cellWindowRectangle,
).canvasPositionRounded
const { gridCellCoordinates, cellCanvasRectangle } = newTargetCell

const defaultSize =
interactionData.type === 'DRAG' &&
Expand All @@ -200,7 +194,7 @@ const gridDrawToInsertStrategyInner =
const insertionCommand = getInsertionCommand(
targetParent,
insertionSubject,
getFrameForInsertion(interactionData, defaultSize, cellCanvasOrigin),
getFrameForInsertion(interactionData, defaultSize, cellCanvasRectangle),
)

const gridTemplate = parent.specialSizeMeasurements.containerGridProperties
Expand Down Expand Up @@ -329,24 +323,3 @@ function getInsertionCommand(

return insertElementInsertionSubject('always', updatedInsertionSubject, insertionPath)
}

function getGridCellUnderCursor(
interactionData: DragInteractionData | HoverInteractionData,
canvasState: InteractionCanvasState,
customStrategyState: CustomStrategyState,
) {
const windowPointToUse =
interactionData.type === 'DRAG' ? interactionData.dragStart : interactionData.point

const mouseWindowPoint = canvasPointToWindowPoint(
windowPointToUse,
canvasState.scale,
canvasState.canvasOffset,
)

return getTargetCell(
customStrategyState.grid.targetCellData?.gridCellCoordinates ?? null,
false,
mouseWindowPoint,
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {
scaleVector,
windowPoint,
windowVector,
type WindowPoint,
} from '../../../../core/shared/math-utils'
import * as PP from '../../../../core/shared/property-path'
import { absolute } from '../../../../utils/utils'
Expand All @@ -42,11 +41,9 @@ import type { DragInteractionData } from '../interaction-state'
import type { GridCellCoordinates } from './grid-cell-bounds'
import {
getCellWindowRect,
getGridCellUnderMouse,
getGridCellUnderMouseRecursive,
getGridCellUnderMouseFromMetadata,
gridCellCoordinates,
} from './grid-cell-bounds'
import type { Sides } from 'utopia-api/core'
import { memoize } from '../../../../core/shared/memoize'

export function runGridRearrangeMove(
Expand All @@ -57,7 +54,6 @@ export function runGridRearrangeMove(
canvasScale: number,
canvasOffset: CanvasVector,
customState: GridCustomStrategyState,
duplicating: boolean,
): {
commands: CanvasCommand[]
targetCell: TargetGridCellData | null
Expand All @@ -75,18 +71,23 @@ export function runGridRearrangeMove(
}
}

const mouseWindowPoint = canvasPointToWindowPoint(
offsetPoint(interactionData.dragStart, interactionData.drag),
canvasScale,
canvasOffset,
)
const parentGridPath = EP.parentPath(selectedElement)
const grid = MetadataUtils.findElementByElementPath(jsxMetadata, parentGridPath)

if (grid == null) {
return {
commands: [],
targetCell: null,
originalRootCell: null,
draggingFromCell: null,
targetRootCell: null,
}
}

const mousePos = offsetPoint(interactionData.dragStart, interactionData.drag)

const targetCellData =
getTargetCell(
customState.targetCellData?.gridCellCoordinates ?? null,
duplicating,
mouseWindowPoint,
) ?? customState.targetCellData
getGridCellUnderMouseFromMetadata(grid, mousePos) ?? customState.targetCellData

if (targetCellData == null) {
return {
Expand Down Expand Up @@ -347,37 +348,10 @@ export function setGridPropsCommands(
}

export interface TargetGridCellData {
gridCellCoordinates: GridCellCoordinates
cellWindowRectangle: WindowRectangle
}

export interface TargetGridCellDataCanvas {
gridCellCoordinates: GridCellCoordinates
cellCanvasRectangle: CanvasRectangle
}

export function getTargetCell(
previousTargetCell: GridCellCoordinates | null,
duplicating: boolean,
mouseWindowPoint: WindowPoint,
): TargetGridCellData | null {
let cell = previousTargetCell ?? null
const cellUnderMouse = duplicating
? getGridCellUnderMouseRecursive(mouseWindowPoint)
: getGridCellUnderMouse(mouseWindowPoint)
if (cellUnderMouse == null) {
return null
}
cell = cellUnderMouse.coordinates
if (cell.row < 1 || cell.column < 1) {
return null
}
return {
gridCellCoordinates: cell,
cellWindowRectangle: cellUnderMouse.cellWindowRectangle,
}
}

function getElementGridProperties(
element: ElementInstanceMetadata,
cellUnderMouse: { row: number; column: number },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ export const gridRearrangeMoveDuplicateStrategy: CanvasStrategyFactory = (
canvasState.scale,
canvasState.canvasOffset,
customState.grid,
true,
)
if (moveCommands.length === 0) {
return emptyStrategyApplicationResult
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,6 @@ function getCommandsAndPatchForGridRearrange(
canvasState.scale,
canvasState.canvasOffset,
customState.grid,
false,
)

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ describe('grid reparent strategies', () => {

await selectComponentsForTest(editor, [EP.fromString('sb/grid/dragme')])

await dragOut(editor, 'grid', EP.fromString('sb/grid/dragme'), { x: 2200, y: 2500 })
await dragOut(editor, 'grid', EP.fromString('sb/grid/dragme'), { x: 2200, y: 2200 })

expect(getPrintedUiJsCode(editor.getEditorState())).toEqual(
formatTestProjectCode(
Expand Down Expand Up @@ -804,7 +804,11 @@ async function dragOut(
const sourceGridCell = renderResult.renderedDOM.getByTestId(GridCellTestId(cell))
const sourceRect = sourceGridCell.getBoundingClientRect()

await mouseClickAtPoint(sourceGridCell, sourceRect, { modifiers: cmdModifier })
await mouseClickAtPoint(
sourceGridCell,
{ x: sourceRect.x + 5, y: sourceRect.y + 5 },
{ modifiers: cmdModifier },
)
await mouseDragFromPointToPoint(sourceGridCell, sourceRect, endPoint, {
modifiers: cmdModifier,
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,11 @@ import { controlsForGridPlaceholders } from '../../controls/grid-controls'
import { ParentBounds } from '../../controls/parent-bounds'
import { ParentOutlines } from '../../controls/parent-outlines'
import { ZeroSizedElementControls } from '../../controls/zero-sized-element-controls'
import { canvasPointToWindowPoint } from '../../dom-lookup'
import type { CanvasStrategyFactory } from '../canvas-strategies'
import type {
CanvasStrategy,
ControlWithProps,
CustomStrategyState,
GridCustomStrategyState,
InteractionCanvasState,
} from '../canvas-strategy-types'
import {
Expand All @@ -41,14 +39,13 @@ import {
import type { DragInteractionData, InteractionSession, UpdatedPathMap } from '../interaction-state'
import { honoursPropsPosition, shouldKeepMovingDraggedGroupChildren } from './absolute-utils'
import { replaceFragmentLikePathsWithTheirChildrenRecursive } from './fragment-like-helpers'
import type { TargetGridCellData } from './grid-helpers'
import { getTargetCell, setGridPropsCommands } from './grid-helpers'
import { setGridPropsCommands } from './grid-helpers'
import { ifAllowedToReparent, isAllowedToReparent } from './reparent-helpers/reparent-helpers'
import { removeAbsolutePositioningProps } from './reparent-helpers/reparent-property-changes'
import type { ReparentTarget } from './reparent-helpers/reparent-strategy-helpers'
import { getReparentOutcome, pathToReparent } from './reparent-utils'
import { flattenSelection } from './shared-move-strategies-helpers'
import type { GridCellCoordinates } from './grid-cell-bounds'
import { getGridCellUnderMouseFromMetadata, type GridCellCoordinates } from './grid-cell-bounds'
import { setElementsToRerenderCommand } from '../../commands/set-elements-to-rerender-command'

export function gridReparentStrategy(
Expand Down Expand Up @@ -142,27 +139,6 @@ export function controlsForGridReparent(reparentTarget: ReparentTarget): Control
]
}

function getTargetGridCellUnderCursor(
interactionData: DragInteractionData,
canvasScale: number,
canvasOffset: CanvasVector,
customState: GridCustomStrategyState,
): TargetGridCellData | null {
const mouseWindowPoint = canvasPointToWindowPoint(
offsetPoint(interactionData.dragStart, interactionData.drag ?? canvasVector({ x: 0, y: 0 })),
canvasScale,
canvasOffset,
)

const targetCellUnderMouse = getTargetCell(
customState.targetCellData?.gridCellCoordinates ?? null,
false,
mouseWindowPoint,
)

return targetCellUnderMouse
}

export function applyGridReparent(
canvasState: InteractionCanvasState,
interactionData: DragInteractionData,
Expand All @@ -172,7 +148,7 @@ export function applyGridReparent(
gridFrame: CanvasRectangle,
) {
return () => {
if (interactionData.drag == null) {
if (interactionData.drag == null || selectedElements.length === 0) {
return emptyStrategyApplicationResult
}

Expand All @@ -184,7 +160,16 @@ export function applyGridReparent(
selectedElements,
newParent.intendedParentPath,
() => {
if (interactionData.drag == null) {
if (interactionData.drag == null || selectedElements.length === 0) {
return emptyStrategyApplicationResult
}

const grid = MetadataUtils.findElementByElementPath(
canvasState.startingMetadata,
newParent.intendedParentPath,
)

if (grid == null) {
return emptyStrategyApplicationResult
}

Expand All @@ -201,13 +186,14 @@ export function applyGridReparent(
return emptyStrategyApplicationResult
}

const mousePos = offsetPoint(
interactionData.dragStart,
interactionData.drag ?? canvasVector({ x: 0, y: 0 }),
)

const targetCellData =
getTargetGridCellUnderCursor(
interactionData,
canvasState.scale,
canvasState.canvasOffset,
customStrategyState.grid,
) ?? customStrategyState.grid.targetCellData
getGridCellUnderMouseFromMetadata(grid, mousePos) ??
customStrategyState.grid.targetCellData

if (targetCellData == null) {
return emptyStrategyApplicationResult
Expand Down
Loading

0 comments on commit 57d569c

Please sign in to comment.