Skip to content

Commit

Permalink
Unify elements-to-rerender calculation between the real and fake disp…
Browse files Browse the repository at this point in the history
…atch (#6465)

## Problem
The real dispatch in `editor.tsx` and the fake dispatch have a different
way of calculating which elements to sample with the DOM sampler. This
has been OK up until now, since often all elements we
re-rendered/re-sampled, but now that we started to get more selective
with what to re-render/re-sample, this discrepancy caused problems in an
upcoming PR, #6450.

The problem in #6450 is that in order for the group true-ups to work
with resizing, the absolute resize strategy would have to explicitly
declare any group children of the resized elements as elements to
re-render. Individual strategies shouldn't be aware of this, since
automatically resizing groups is a feature provided by the "platform"
code.

However, it's not enough to put the code to find the child groups into
`boundDispatch`, since then the same thing would have to be
re-implemented in the fake dispatch, so we need to find a way to share
this logic between the two dispatches.

## Fix
This PR contains the dispatch-related portion of the problem discovered
in #6450. The concrete fix is to extract out the logic to determine
which elements to re-render/re-sample into a function that's used by
both the real and fake dispatch. This logic contains which child groups
to re-render as well.

### Note on `ElementsToRerenderGLOBAL.current`
The real dispatch updates `ElementsToRerenderGLOBAL.current`, while the
fake one doesn't. Ideally this behaviour would be the same too, but
updating `ElementsToRerenderGLOBAL.current` inside
`collectElementsToRerender` led to test failures that didn't happen on
the PR where this problem was discovered, so this is out of scope for
this PR
  • Loading branch information
bkrmendy authored Oct 3, 2024
1 parent f0e1e90 commit 03ec7f4
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 30 deletions.
15 changes: 10 additions & 5 deletions editor/src/components/canvas/ui-jsx.test-utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ import {
FakeWatchdogWorker,
} from '../../core/workers/test-workers'
import { UtopiaTsWorkersImplementation } from '../../core/workers/workers'
import { EditorRoot } from '../../templates/editor'
import { collectElementsToRerender, EditorRoot } from '../../templates/editor'
import Utils from '../../utils/utils'
import { getNamedPath } from '../../utils/react-helpers'
import type {
Expand Down Expand Up @@ -435,8 +435,9 @@ export async function renderTestEditorWithModel(
{
resubscribeObservers(domWalkerMutableState)

const elementsToFocusOn = collectElementsToRerender(workingEditorState, actions)
const metadataResult = runDomSamplerRegular({
elementsToFocusOn: workingEditorState.patchedEditor.canvas.elementsToRerender,
elementsToFocusOn: elementsToFocusOn,
domWalkerAdditionalElementsToFocusOn:
workingEditorState.patchedEditor.canvas.domWalkerAdditionalElementsToUpdate,
scale: workingEditorState.patchedEditor.canvas.scale,
Expand Down Expand Up @@ -495,8 +496,6 @@ export async function renderTestEditorWithModel(

// re-render the canvas
{
// TODO run fixElementsToRerender and set ElementsToRerenderGLOBAL

flushSync(() => {
canvasStoreHook.setState(patchedStoreFromFullStore(workingEditorState, 'canvas-store'))
})
Expand All @@ -507,8 +506,14 @@ export async function renderTestEditorWithModel(
{
resubscribeObservers(domWalkerMutableState)

// TODO: The real dispatch updates ElementsToRerenderGLOBAL.current,
// while the fake one doesn't. Ideally this behaviour would be the
// same, but solving this was out of scope for the PR that introduced
// collectElementsToRerender
// (https://github.com/concrete-utopia/utopia/pull/6465)
const elementsToFocusOn = collectElementsToRerender(workingEditorState, actions)
const metadataResult = runDomSamplerGroups({
elementsToFocusOn: workingEditorState.patchedEditor.canvas.elementsToRerender,
elementsToFocusOn: elementsToFocusOn,
domWalkerAdditionalElementsToFocusOn:
workingEditorState.patchedEditor.canvas.domWalkerAdditionalElementsToUpdate,
scale: workingEditorState.patchedEditor.canvas.scale,
Expand Down
68 changes: 43 additions & 25 deletions editor/src/templates/editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ import {
runDomSamplerGroups,
} from '../components/canvas/dom-sampler'
import { omitWithPredicate } from '../core/shared/object-utils'
import { getChildGroupsForNonGroupParents } from '../components/canvas/canvas-strategies/strategies/fragment-like-helpers'

if (PROBABLY_ELECTRON) {
let { webFrame } = requireElectron()
Expand Down Expand Up @@ -165,38 +166,54 @@ function collectElementsToRerenderForTransientActions(
// for this pass use a union of the two arrays, to make sure we clear a previously focused element from the metadata
// and let the canvas re-render components that may have a missing child now.
let lastElementsToRerender: ElementsToRerender = 'rerender-all-elements'
function fixElementsToRerender(
currentElementsToRerender: ElementsToRerender,
dispatchedActions: readonly EditorAction[],
): ElementsToRerender {
// while running transient actions there is an optional elementsToRerender
const elementsToRerenderTransient = uniqBy<ElementPath>(
dispatchedActions.reduce(
collectElementsToRerenderForTransientActions,
[] as Array<ElementPath>,
),
EP.pathsEqual,
)

const currentOrTransientElementsToRerender =
elementsToRerenderTransient.length > 0 ? elementsToRerenderTransient : currentElementsToRerender

let fixedElementsToRerender: ElementsToRerender = currentOrTransientElementsToRerender
function fixElementsToRerender(elementsToRerender: ElementsToRerender): ElementsToRerender {
let fixedElementsToRerender: ElementsToRerender = elementsToRerender
if (
currentOrTransientElementsToRerender !== 'rerender-all-elements' &&
elementsToRerender !== 'rerender-all-elements' &&
lastElementsToRerender !== 'rerender-all-elements'
) {
// if the current elements to rerender array doesn't match the previous elements to rerender array, for a single frame let's use the union of the two arrays
fixedElementsToRerender = EP.uniqueElementPaths([
...lastElementsToRerender,
...currentOrTransientElementsToRerender,
...elementsToRerender,
])
}

lastElementsToRerender = currentOrTransientElementsToRerender
lastElementsToRerender = fixedElementsToRerender
return fixedElementsToRerender
}

export function collectElementsToRerender(
editorStore: EditorStoreFull,
dispatchedActions: readonly EditorAction[],
): ElementsToRerender {
const elementsToRerenderTransient = uniqBy<ElementPath>(
dispatchedActions.reduce(
collectElementsToRerenderForTransientActions,
[] as Array<ElementPath>,
),
EP.pathsEqual,
)

const elementsToRerender =
elementsToRerenderTransient.length > 0
? elementsToRerenderTransient
: editorStore.patchedEditor.canvas.elementsToRerender

const fixedElementsToRerender = fixElementsToRerender(elementsToRerender)
const fixedElementsWithChildGroups =
fixedElementsToRerender === 'rerender-all-elements'
? fixedElementsToRerender
: [
...fixedElementsToRerender,
...getChildGroupsForNonGroupParents(
editorStore.patchedEditor.jsxMetadata,
fixedElementsToRerender,
),
]
return fixedElementsWithChildGroups
}

export class Editor {
storedState: EditorStoreFull
utopiaStoreHook: UtopiaStoreAPI
Expand Down Expand Up @@ -460,8 +477,8 @@ export class Editor {
if (shouldUpdateCanvasStore) {
// this will re-render the canvas root and potentially the canvas contents itself
Measure.taskTime(`update canvas ${updateId}`, () => {
const currentElementsToRender = fixElementsToRerender(
this.storedState.patchedEditor.canvas.elementsToRerender,
const currentElementsToRender = collectElementsToRerender(
this.storedState,
dispatchedActions,
)
ElementsToRerenderGLOBAL.current = currentElementsToRender // Mutation!
Expand Down Expand Up @@ -521,10 +538,11 @@ export class Editor {

// re-render the canvas
Measure.taskTime(`Canvas re-render because of groups ${updateId}`, () => {
ElementsToRerenderGLOBAL.current = fixElementsToRerender(
this.storedState.patchedEditor.canvas.elementsToRerender,
const currentElementsToRender = collectElementsToRerender(
this.storedState,
dispatchedActions,
) // Mutation!
)
ElementsToRerenderGLOBAL.current = currentElementsToRender // Mutation!

ReactDOM.flushSync(() => {
ReactDOM.unstable_batchedUpdates(() => {
Expand Down

0 comments on commit 03ec7f4

Please sign in to comment.