Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't render when code ahead #6069

Merged
merged 5 commits into from
Jul 12, 2024
+119 −81
Merged

Don't render when code ahead #6069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
61044c8
fix(editor) don't render when code ahead
Rheeseyb Jul 10, 2024
db36d50
fix(editor) Tweaked the condition for shouldRerender.
seanparsons Jul 10, 2024
b55430b
Merge remote-tracking branch 'origin' into fix/dont-render-when-code-…
seanparsons Jul 11, 2024
1c41ac1
fix(editor.tsx) Fix gating of dom walker, remove gating of dispatch c…
Rheeseyb Jul 12, 2024
aa79ac5
Merge branch 'fix/dont-render-when-code-ahead' of github.com:concrete…
Rheeseyb Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion editor/src/components/assets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@ import type {
AssetFile,
ParseSuccess,
} from '../core/shared/project-file-types'
import { directory, isDirectory, isImageFile } from '../core/shared/project-file-types'
import {
directory,
isDirectory,
isImageFile,
RevisionsState,
} from '../core/shared/project-file-types'
import { isTextFile, isParseSuccess, isAssetFile } from '../core/shared/project-file-types'
import Utils from '../utils/utils'
import { dropLeadingSlash } from './filebrowser/filepath-utils'
Expand All @@ -28,6 +33,8 @@ import type {
ProjectContentsTree,
PathAndFileEntry,
} from 'utopia-shared/src/types/assets'
import { filtered, fromField, fromTypeGuard } from '../core/shared/optics/optic-creators'
import { anyBy, toArrayOf } from '../core/shared/optics/optic-utilities'
export type {
AssetFileWithFileName,
ProjectContentTreeRoot,
Expand Down Expand Up @@ -391,6 +398,32 @@ export const contentsTreeOptic: Optic<ProjectContentTreeRoot, PathAndFileEntry>
},
)

export function anyCodeAhead(tree: ProjectContentTreeRoot): boolean {
const revisionsStateOptic = contentsTreeOptic
.compose(fromField('file'))
.compose(fromTypeGuard(isTextFile))
.compose(fromField('fileContents'))
.compose(filtered((f) => f.parsed.type === 'PARSE_SUCCESS'))
.compose(fromField('revisionsState'))

return anyBy(
revisionsStateOptic,
(revisionsState) => {
switch (revisionsState) {
case 'BOTH_MATCH':
case 'PARSED_AHEAD':
return false
case 'CODE_AHEAD':
case 'CODE_AHEAD_BUT_PLEASE_TELL_VSCODE_ABOUT_IT':
return true
default:
assertNever(revisionsState)
}
},
tree,
)
}

export function walkContentsTreeForParseSuccess(
tree: ProjectContentTreeRoot,
onElement: (fullPath: string, parseSuccess: ParseSuccess) => void,
Expand Down
165 changes: 85 additions & 80 deletions editor/src/templates/editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ import { hasReactRouterErrorBeenLogged } from '../core/shared/runtime-report-log
import { InitialOnlineState, startOnlineStatusPolling } from '../components/editor/online-status'
import { useAnimate } from 'framer-motion'
import { AnimationContext } from '../components/canvas/ui-jsx-canvas-renderer/animation-context'
import { anyCodeAhead } from '../components/assets'

if (PROBABLY_ELECTRON) {
let { webFrame } = requireElectron()
Expand Down Expand Up @@ -450,7 +451,12 @@ export class Editor {
const reactRouterErrorPreviouslyLogged = hasReactRouterErrorBeenLogged()

const runDomWalker = shouldRunDOMWalker(dispatchedActions, dispatchResult)
const shouldRerender = !dispatchResult.nothingChanged || runDomWalker
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that shouldRerender was actually very badly named here, and so changed some of the gating of the code below accordingly. It's worth expanding the diff below to understand what is actually happening now


const somethingChanged = !dispatchResult.nothingChanged

const shouldRerender =
(somethingChanged || runDomWalker) &&
!anyCodeAhead(dispatchResult.unpatchedEditor.projectContents)

const updateId = canvasUpdateId++
if (shouldRerender) {
Expand All @@ -467,102 +473,101 @@ export class Editor {
})
})
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:narrows_eyes: the original code assumes that it only makes sense to run the dom-walker if there was a canvas render. is that assumption false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldRerender was a bit of a misnomer, especially as it was gating logic that should run regardless of if that canvas rendered (e.g. the editorDispatchClosingOut at the end). There are cases where it makes sense to run the dom-walker without a canvas render (e.g. navigating to a different route). But that being said, the below condition should actually be somethingChanged || runDomWalker so I'll fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated it to shouldRerender || runDomWalker in the end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, but shouldRerender is already const shouldRerender = (somethingChanged || runDomWalker) && !anyCodeAhead(dispatchResult.unpatchedEditor.projectContents), aren't you defeating the anyCodeAhead check then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dom walker should run whenever the canvas is rendered, hence the shouldRerender check, but it should also run when we explicitly ask it to (e.g. when the editor is in play mode and the user has navigated to a different route), hence the || runDomWalker.


// run the dom-walker
if (shouldRerender || runDomWalker) {
const domWalkerDispatchResult = runDomWalkerAndSaveResults(
this.boundDispatch,
this.domWalkerMutableState,
this.storedState,
this.spyCollector,
ElementsToRerenderGLOBAL.current,
)

// run the dom-walker
if (runDomWalker) {
const domWalkerDispatchResult = runDomWalkerAndSaveResults(
if (domWalkerDispatchResult != null) {
this.storedState = domWalkerDispatchResult
entireUpdateFinished = Promise.all([
entireUpdateFinished,
domWalkerDispatchResult.entireUpdateFinished,
])
}
}

// true up groups if needed
if (this.storedState.unpatchedEditor.trueUpElementsAfterDomWalkerRuns.length > 0) {
// updated editor with trued up groups
Measure.taskTime(`Group true up ${updateId}`, () => {
const projectContentsBeforeGroupTrueUp = this.storedState.unpatchedEditor.projectContents
const dispatchResultWithTruedUpGroups = editorDispatchActionRunner(
this.boundDispatch,
this.domWalkerMutableState,
[{ action: 'TRUE_UP_ELEMENTS' }],
this.storedState,
this.spyCollector,
ElementsToRerenderGLOBAL.current,
)
this.storedState = dispatchResultWithTruedUpGroups

if (domWalkerDispatchResult != null) {
this.storedState = domWalkerDispatchResult
entireUpdateFinished = Promise.all([
entireUpdateFinished,
domWalkerDispatchResult.entireUpdateFinished,
])
entireUpdateFinished = Promise.all([
entireUpdateFinished,
dispatchResultWithTruedUpGroups.entireUpdateFinished,
])

if (
projectContentsBeforeGroupTrueUp === this.storedState.unpatchedEditor.projectContents
) {
// no group-related re-render / re-measure is needed, bail out
return
}
}

// true up groups if needed
if (this.storedState.unpatchedEditor.trueUpElementsAfterDomWalkerRuns.length > 0) {
// updated editor with trued up groups
Measure.taskTime(`Group true up ${updateId}`, () => {
const projectContentsBeforeGroupTrueUp =
this.storedState.unpatchedEditor.projectContents
const dispatchResultWithTruedUpGroups = editorDispatchActionRunner(
// re-render the canvas
Measure.taskTime(`Canvas re-render because of groups ${updateId}`, () => {
ElementsToRerenderGLOBAL.current = fixElementsToRerender(
this.storedState.patchedEditor.canvas.elementsToRerender,
dispatchedActions,
) // Mutation!

ReactDOM.flushSync(() => {
ReactDOM.unstable_batchedUpdates(() => {
this.canvasStore.setState(
patchedStoreFromFullStore(this.storedState, 'canvas-store'),
)
})
})
})

// re-run the dom-walker
Measure.taskTime(`Dom walker re-run because of groups ${updateId}`, () => {
const domWalkerDispatchResult = runDomWalkerAndSaveResults(
this.boundDispatch,
[{ action: 'TRUE_UP_ELEMENTS' }],
this.domWalkerMutableState,
this.storedState,
this.spyCollector,
ElementsToRerenderGLOBAL.current,
)
this.storedState = dispatchResultWithTruedUpGroups

entireUpdateFinished = Promise.all([
entireUpdateFinished,
dispatchResultWithTruedUpGroups.entireUpdateFinished,
])

if (
projectContentsBeforeGroupTrueUp === this.storedState.unpatchedEditor.projectContents
) {
// no group-related re-render / re-measure is needed, bail out
return
if (domWalkerDispatchResult != null) {
this.storedState = domWalkerDispatchResult
entireUpdateFinished = Promise.all([
entireUpdateFinished,
domWalkerDispatchResult.entireUpdateFinished,
])
}

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

ReactDOM.flushSync(() => {
ReactDOM.unstable_batchedUpdates(() => {
this.canvasStore.setState(
patchedStoreFromFullStore(this.storedState, 'canvas-store'),
)
})
})
})

// re-run the dom-walker
Measure.taskTime(`Dom walker re-run because of groups ${updateId}`, () => {
const domWalkerDispatchResult = runDomWalkerAndSaveResults(
this.boundDispatch,
this.domWalkerMutableState,
this.storedState,
this.spyCollector,
ElementsToRerenderGLOBAL.current,
)

if (domWalkerDispatchResult != null) {
this.storedState = domWalkerDispatchResult
entireUpdateFinished = Promise.all([
entireUpdateFinished,
domWalkerDispatchResult.entireUpdateFinished,
])
}
})
})
}

this.storedState = editorDispatchClosingOut(
this.boundDispatch,
dispatchedActions,
oldEditorState,
{
...this.storedState,
entireUpdateFinished: entireUpdateFinished,
nothingChanged: dispatchResult.nothingChanged,
},
reactRouterErrorPreviouslyLogged,
)
})
}

this.storedState = editorDispatchClosingOut(
this.boundDispatch,
dispatchedActions,
oldEditorState,
{
...this.storedState,
entireUpdateFinished: entireUpdateFinished,
nothingChanged: dispatchResult.nothingChanged,
},
reactRouterErrorPreviouslyLogged,
)

Measure.taskTime(`Update Editor ${updateId}`, () => {
ReactDOM.flushSync(() => {
ReactDOM.unstable_batchedUpdates(() => {
Expand Down
Loading