From 1697de806e3e9a9459133f21ed0bbbf33eb79dbb Mon Sep 17 00:00:00 2001 From: Balazs Bajorics <2226774+balazsbajorics@users.noreply.github.com> Date: Tue, 17 Sep 2024 13:34:22 +0200 Subject: [PATCH] Fix scrubbing performance of FrameUpdatingLayoutControl (#6375) **Problem:** When using the left label (such as **L**) to scrub a FrameUpdatingLayoutControl, the performance was bad. image 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 --- .../components/inspector/controls/control.ts | 5 + .../inspector-strategy.ts | 20 +++- .../frame-updating-layout-section.tsx | 106 +++++++++++------- editor/src/uuiui/inputs/number-input.tsx | 29 ++--- 4 files changed, 103 insertions(+), 57 deletions(-) diff --git a/editor/src/components/inspector/controls/control.ts b/editor/src/components/inspector/controls/control.ts index 7d02aca9defe..bd1104b68fb8 100644 --- a/editor/src/components/inspector/controls/control.ts +++ b/editor/src/components/inspector/controls/control.ts @@ -19,6 +19,11 @@ export type OnSubmitValueOrUnknownOrEmpty = ( transient?: boolean, ) => void +export type OnSubmitValueOrUnknownOrEmptyMaybeTransient = ( + value: UnknownOrEmptyInput, + transient: boolean, +) => void + export interface DEPRECATEDControlProps { id: string testId: string diff --git a/editor/src/components/inspector/inspector-strategies/inspector-strategy.ts b/editor/src/components/inspector/inspector-strategies/inspector-strategy.ts index cdf7d85255cd..38af9cf837a9 100644 --- a/editor/src/components/inspector/inspector-strategies/inspector-strategy.ts +++ b/editor/src/components/inspector/inspector-strategies/inspector-strategy.ts @@ -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 @@ -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)]) + } } } diff --git a/editor/src/components/inspector/sections/layout-section/self-layout-subsection/frame-updating-layout-section.tsx b/editor/src/components/inspector/sections/layout-section/self-layout-subsection/frame-updating-layout-section.tsx index 33c5d446267d..0c729df8b904 100644 --- a/editor/src/components/inspector/sections/layout-section/self-layout-subsection/frame-updating-layout-section.tsx +++ b/editor/src/components/inspector/sections/layout-section/self-layout-subsection/frame-updating-layout-section.tsx @@ -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' @@ -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': @@ -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: @@ -341,7 +363,7 @@ interface LayoutPinPropertyControlProps { label: string property: TLWH currentValues: Array - updateFrame: (frameUpdate: FrameUpdate) => void + updateFrame: (frameUpdate: FrameUpdate, transient: boolean) => void invalid?: boolean } @@ -378,7 +400,7 @@ const FrameUpdatingLayoutControl = React.memo((props: LayoutPinPropertyControlPr currentValuesRef.current = currentValues const onSubmitValue = React.useCallback( - (newValue: UnknownOrEmptyInput) => { + (newValue: UnknownOrEmptyInput, transient: boolean = false) => { const calculatedSingleCommonValue = getSingleCommonValue(currentValuesRef.current) if (isUnknownInputValue(newValue)) { // Ignore right now. @@ -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.') diff --git a/editor/src/uuiui/inputs/number-input.tsx b/editor/src/uuiui/inputs/number-input.tsx index ba6d73773d6f..d0fcfc5ff9f3 100644 --- a/editor/src/uuiui/inputs/number-input.tsx +++ b/editor/src/uuiui/inputs/number-input.tsx @@ -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' @@ -151,9 +152,9 @@ export interface AbstractNumberInputProps } export interface NumberInputProps extends AbstractNumberInputProps { - onSubmitValue?: OnSubmitValueOrUnknownOrEmpty - onTransientSubmitValue?: OnSubmitValueOrUnknownOrEmpty - onForcedSubmitValue?: OnSubmitValueOrUnknownOrEmpty + onSubmitValue?: OnSubmitValueOrUnknownOrEmptyMaybeTransient + onTransientSubmitValue?: OnSubmitValueOrUnknownOrEmptyMaybeTransient + onForcedSubmitValue?: OnSubmitValueOrUnknownOrEmptyMaybeTransient setGlobalCursor?: (cursor: CSSCursor | null) => void onMouseEnter?: MouseEventHandler onMouseLeave?: MouseEventHandler @@ -282,15 +283,15 @@ export const NumberInput = React.memo( 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 @@ -339,15 +340,15 @@ export const NumberInput = React.memo( 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) @@ -484,7 +485,7 @@ export const NumberInput = React.memo( // 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) } } }, @@ -519,7 +520,7 @@ export const NumberInput = React.memo( if (valueChangedSinceFocus) { setValueChangedSinceFocus(false) if (onSubmitValue != null) { - onSubmitValue(newValue) + onSubmitValue(newValue, false) } } }, @@ -565,6 +566,7 @@ export const NumberInput = React.memo( repeatedValueRef.current != null ? repeatedValueRef.current : unknownInputValue(displayValue), + false, ) } @@ -611,6 +613,7 @@ export const NumberInput = React.memo( repeatedValueRef.current != null ? repeatedValueRef.current : unknownInputValue(displayValue), + false, ) }