Skip to content

Commit

Permalink
Fix scrubbing performance of FrameUpdatingLayoutControl (#6375)
Browse files Browse the repository at this point in the history
**Problem:**
When using the left label (such as **L**) to scrub a
FrameUpdatingLayoutControl, the performance was bad.
<img width="282" alt="image"
src="https://github.com/user-attachments/assets/9c44718d-a44d-41cc-845d-993ba515bdf9">

Turns out the problem is simply that the control was never using the
transient mode, for each scrub update it would fire a non-transient
action, rerendering the entire canvas, **creating a new undo step!!!**

**Fix:**
Make the control aware of transient updates.

**Commit Details:**

- `executeFirstApplicableStrategy` takes an array of elementsToRerender,
if it's a specific array of element paths, it automatically switches to
dispatching Transient Actions
- `NumberInput` now sets the transient flag for all onSubmit callbacks
when scrubbed
- `FrameUpdatingLayoutControl`'s onSubmitValue takes the transient
parameter and feeds it to updateFrame
- `updateFrame` calls `executeFirstApplicableStrategy` with the properly
set `elementsToRerender` array

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

- [x] I opened a hydrogen project and it loaded
- [ ] I could navigate to various routes in Preview mode
  • Loading branch information
balazsbajorics authored Sep 17, 2024
1 parent b1d8588 commit 1697de8
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 57 deletions.
5 changes: 5 additions & 0 deletions editor/src/components/inspector/controls/control.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ export type OnSubmitValueOrUnknownOrEmpty<T> = (
transient?: boolean,
) => void

export type OnSubmitValueOrUnknownOrEmptyMaybeTransient<T> = (
value: UnknownOrEmptyInput<T>,
transient: boolean,
) => void

export interface DEPRECATEDControlProps<T> {
id: string
testId: string
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { CanvasCommand } from '../../canvas/commands/commands'
import type { EditorDispatch } from '../../editor/action-types'
import { applyCommandsAction } from '../../editor/actions/action-creators'
import { applyCommandsAction, transientActions } from '../../editor/actions/action-creators'
import type { ElementsToRerender } from '../../editor/store/editor-state'

interface CustomInspectorStrategyResultBase {
commands: Array<CanvasCommand>
Expand Down Expand Up @@ -47,11 +48,26 @@ export function commandsForFirstApplicableStrategy(

export function executeFirstApplicableStrategy(
dispatch: EditorDispatch,
strategies: InspectorStrategy[],
): void {
return executeFirstApplicableStrategyForContinuousInteraction(
dispatch,
strategies,
'rerender-all-elements',
)
}

export function executeFirstApplicableStrategyForContinuousInteraction(
dispatch: EditorDispatch,
strategies: InspectorStrategy[],
elementsToRerenderTransient: ElementsToRerender,
): void {
const commands = commandsForFirstApplicableStrategy(strategies)
if (commands != null) {
dispatch([applyCommandsAction(commands)])
if (elementsToRerenderTransient !== 'rerender-all-elements') {
dispatch([transientActions([applyCommandsAction(commands)], elementsToRerenderTransient)])
} else {
dispatch([applyCommandsAction(commands)])
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ import {
type UnknownOrEmptyInput,
} from '../../../common/css-utils'
import { useInspectorLayoutInfo } from '../../../common/property-path-hooks'
import { executeFirstApplicableStrategy } from '../../../inspector-strategies/inspector-strategy'
import {
executeFirstApplicableStrategy,
executeFirstApplicableStrategyForContinuousInteraction,
} from '../../../inspector-strategies/inspector-strategy'
import { UIGridRow } from '../../../widgets/ui-grid-row'
import * as EP from '../../../../../core/shared/element-path'

Expand Down Expand Up @@ -201,32 +204,43 @@ export const FrameUpdatingLayoutSection = React.memo(() => {
)

const updateFrame = React.useCallback(
(frameUpdate: FrameUpdate) => {
(frameUpdate: FrameUpdate, transient: boolean) => {
const elementsToRerenderTransient = transient
? selectedViewsRef.current
: 'rerender-all-elements'
switch (frameUpdate.type) {
case 'DELTA_FRAME_UPDATE':
if (
frameUpdate.edgePosition === EdgePositionTop ||
frameUpdate.edgePosition === EdgePositionLeft
) {
executeFirstApplicableStrategy(dispatch, [
moveInspectorStrategy(
metadataRef.current,
selectedViewsRef.current,
projectContentsRef.current,
frameUpdate.edgeMovement,
),
])
executeFirstApplicableStrategyForContinuousInteraction(
dispatch,
[
moveInspectorStrategy(
metadataRef.current,
selectedViewsRef.current,
projectContentsRef.current,
frameUpdate.edgeMovement,
),
],
elementsToRerenderTransient,
)
} else {
executeFirstApplicableStrategy(dispatch, [
resizeInspectorStrategy(
metadataRef.current,
selectedViewsRef.current,
projectContentsRef.current,
originalGlobalFrame,
frameUpdate.edgePosition,
frameUpdate.edgeMovement,
),
])
executeFirstApplicableStrategyForContinuousInteraction(
dispatch,
[
resizeInspectorStrategy(
metadataRef.current,
selectedViewsRef.current,
projectContentsRef.current,
originalGlobalFrame,
frameUpdate.edgePosition,
frameUpdate.edgeMovement,
),
],
elementsToRerenderTransient,
)
}
break
case 'DIRECT_FRAME_UPDATE':
Expand All @@ -235,27 +249,35 @@ export const FrameUpdatingLayoutSection = React.memo(() => {
frameUpdate.edgePosition === EdgePositionLeft
) {
const leftOrTop = frameUpdate.edgePosition === EdgePositionLeft ? 'left' : 'top'
executeFirstApplicableStrategy(dispatch, [
directMoveInspectorStrategy(
metadataRef.current,
selectedViewsRef.current,
projectContentsRef.current,
leftOrTop,
frameUpdate.edgeValue,
),
])
executeFirstApplicableStrategyForContinuousInteraction(
dispatch,
[
directMoveInspectorStrategy(
metadataRef.current,
selectedViewsRef.current,
projectContentsRef.current,
leftOrTop,
frameUpdate.edgeValue,
),
],
elementsToRerenderTransient,
)
} else {
const widthOrHeight =
frameUpdate.edgePosition === EdgePositionRight ? 'width' : 'height'
executeFirstApplicableStrategy(dispatch, [
directResizeInspectorStrategy(
metadataRef.current,
selectedViewsRef.current,
projectContentsRef.current,
widthOrHeight,
frameUpdate.edgeValue,
),
])
executeFirstApplicableStrategyForContinuousInteraction(
dispatch,
[
directResizeInspectorStrategy(
metadataRef.current,
selectedViewsRef.current,
projectContentsRef.current,
widthOrHeight,
frameUpdate.edgeValue,
),
],
elementsToRerenderTransient,
)
}
break
default:
Expand Down Expand Up @@ -341,7 +363,7 @@ interface LayoutPinPropertyControlProps {
label: string
property: TLWH
currentValues: Array<number>
updateFrame: (frameUpdate: FrameUpdate) => void
updateFrame: (frameUpdate: FrameUpdate, transient: boolean) => void
invalid?: boolean
}

Expand Down Expand Up @@ -378,7 +400,7 @@ const FrameUpdatingLayoutControl = React.memo((props: LayoutPinPropertyControlPr
currentValuesRef.current = currentValues

const onSubmitValue = React.useCallback(
(newValue: UnknownOrEmptyInput<CSSNumber>) => {
(newValue: UnknownOrEmptyInput<CSSNumber>, transient: boolean = false) => {
const calculatedSingleCommonValue = getSingleCommonValue(currentValuesRef.current)
if (isUnknownInputValue(newValue)) {
// Ignore right now.
Expand All @@ -390,14 +412,14 @@ const FrameUpdatingLayoutControl = React.memo((props: LayoutPinPropertyControlPr
if (newValue.unit == null || newValue.unit === 'px') {
const edgePosition = getTLWHEdgePosition(property)
if (calculatedSingleCommonValue == null) {
updateFrame(directFrameUpdate(edgePosition, newValue.value))
updateFrame(directFrameUpdate(edgePosition, newValue.value), transient)
} else {
const movement = getMovementFromValues(
property,
calculatedSingleCommonValue,
newValue.value,
)
updateFrame(deltaFrameUpdate(edgePosition, movement))
updateFrame(deltaFrameUpdate(edgePosition, movement), transient)
}
} else {
console.error('Attempting to use a value with a unit, which is invalid.')
Expand Down
29 changes: 16 additions & 13 deletions editor/src/uuiui/inputs/number-input.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import type {
OnSubmitValue,
OnSubmitValueOrEmpty,
OnSubmitValueOrUnknownOrEmpty,
OnSubmitValueOrUnknownOrEmptyMaybeTransient,
} from '../../components/inspector/controls/control'
import type { Either } from '../../core/shared/either'
import { isLeft, mapEither } from '../../core/shared/either'
Expand Down Expand Up @@ -151,9 +152,9 @@ export interface AbstractNumberInputProps<T extends CSSNumber | number>
}

export interface NumberInputProps extends AbstractNumberInputProps<CSSNumber> {
onSubmitValue?: OnSubmitValueOrUnknownOrEmpty<CSSNumber>
onTransientSubmitValue?: OnSubmitValueOrUnknownOrEmpty<CSSNumber>
onForcedSubmitValue?: OnSubmitValueOrUnknownOrEmpty<CSSNumber>
onSubmitValue?: OnSubmitValueOrUnknownOrEmptyMaybeTransient<CSSNumber>
onTransientSubmitValue?: OnSubmitValueOrUnknownOrEmptyMaybeTransient<CSSNumber>
onForcedSubmitValue?: OnSubmitValueOrUnknownOrEmptyMaybeTransient<CSSNumber>
setGlobalCursor?: (cursor: CSSCursor | null) => void
onMouseEnter?: MouseEventHandler
onMouseLeave?: MouseEventHandler
Expand Down Expand Up @@ -282,15 +283,15 @@ export const NumberInput = React.memo<NumberInputProps>(
const newValue = setCSSNumberValue(value, newNumericValue)
if (transient) {
if (onTransientSubmitValue != null) {
onTransientSubmitValue(newValue)
onTransientSubmitValue(newValue, transient)
} else if (onSubmitValue != null) {
onSubmitValue(newValue)
onSubmitValue(newValue, transient)
}
} else {
if (onForcedSubmitValue != null) {
onForcedSubmitValue(newValue)
onForcedSubmitValue(newValue, transient)
} else if (onSubmitValue != null) {
onSubmitValue(newValue)
onSubmitValue(newValue, transient)
}
}
repeatedValueRef.current = newValue
Expand Down Expand Up @@ -339,15 +340,15 @@ export const NumberInput = React.memo<NumberInputProps>(

if (transient) {
if (onTransientSubmitValue != null) {
onTransientSubmitValue(newValue)
onTransientSubmitValue(newValue, transient)
} else if (onSubmitValue != null) {
onSubmitValue(newValue)
onSubmitValue(newValue, transient)
}
} else {
if (onForcedSubmitValue != null) {
onForcedSubmitValue(newValue)
onForcedSubmitValue(newValue, transient)
} else if (onSubmitValue != null) {
onSubmitValue(newValue)
onSubmitValue(newValue, transient)
}
}
updateValue(newValue)
Expand Down Expand Up @@ -484,7 +485,7 @@ export const NumberInput = React.memo<NumberInputProps>(
// todo make sure this isn't doubling up the value submit
if ((e.key === 'ArrowUp' || e.key === 'ArrowDown') && onForcedSubmitValue != null) {
if (value != null) {
onForcedSubmitValue(value)
onForcedSubmitValue(value, false)
}
}
},
Expand Down Expand Up @@ -519,7 +520,7 @@ export const NumberInput = React.memo<NumberInputProps>(
if (valueChangedSinceFocus) {
setValueChangedSinceFocus(false)
if (onSubmitValue != null) {
onSubmitValue(newValue)
onSubmitValue(newValue, false)
}
}
},
Expand Down Expand Up @@ -565,6 +566,7 @@ export const NumberInput = React.memo<NumberInputProps>(
repeatedValueRef.current != null
? repeatedValueRef.current
: unknownInputValue(displayValue),
false,
)
}

Expand Down Expand Up @@ -611,6 +613,7 @@ export const NumberInput = React.memo<NumberInputProps>(
repeatedValueRef.current != null
? repeatedValueRef.current
: unknownInputValue(displayValue),
false,
)
}

Expand Down

0 comments on commit 1697de8

Please sign in to comment.