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.

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

Fix environment changes causing remounted scenes with lost state #223

Merged
merged 10 commits into from
Jul 29, 2020

Conversation

MaxDesiatov
Copy link
Collaborator

@MaxDesiatov MaxDesiatov commented Jul 28, 2020

Implements support for custom scenes by adding the new MountedScene type and fixes environment propagation from mounted parent to mounted child elements.

This will unblock #136 and potentially #214. In the former scenes are always completely unmounted and remounted from scratch when root environment changes, which causes descendants to lose all @State values. I saw some environment propagation issues in the latter, but not sure if those were caused by the lack of correct scene updates, just wanted to tackle the usual suspects first.

I've also improved reconciler-related doc comments to clarify some of the design desicions and naming.

Resolves #222.

@MaxDesiatov MaxDesiatov added the bug Something isn't working label Jul 28, 2020
@MaxDesiatov MaxDesiatov changed the title Fix environment propagation remounting scene elements Fix environment changes causing remounted scenes with lost state Jul 28, 2020
Base automatically changed from unify-reconciler-algorithm to main July 28, 2020 17:01
# Conflicts:
#	Sources/TokamakCore/App/_AnyScene.swift
#	Sources/TokamakCore/MountedViews/MountedApp.swift
#	Sources/TokamakCore/MountedViews/MountedElement.swift
#	Sources/TokamakCore/StackReconciler.swift
@MaxDesiatov MaxDesiatov marked this pull request as ready for review July 29, 2020 17:45
@MaxDesiatov MaxDesiatov requested review from carson-katri and j-f1 July 29, 2020 17:45
protocol ViewContainingScene {
var anyContent: AnyView { get }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There doesn't seem to be a difference between ViewContainingScene and SceneDeferredToRenderer, so I'm only keeping the latter.

@@ -34,49 +34,43 @@ final class MountedApp<R: Renderer>: MountedCompositeElement<R> {
mountedChildren.forEach { $0.unmount(with: reconciler) }
}

private func mountChild<S: Scene>(_ childBody: S) -> MountedElement<R> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In practice S is always _AnyScene here as everything is type-erased by this point, so no need for it to be generic.

updateChild: { $0.scene = _AnyScene(element) },
getElementType: { $0.type },
updateChild: {
$0.environmentValues = environmentValues
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is where environment was not passed from a parent to its children previously.

let parentTarget: R.TargetType

var state = [Any]()
var subscriptions = [AnyCancellable]()
var environmentValues: EnvironmentValues
Copy link
Collaborator Author

@MaxDesiatov MaxDesiatov Jul 29, 2020

Choose a reason for hiding this comment

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

Both environmentValues and mountedChildren now live in the base MountedElement superclass as all subclasses need them except the MountedEmptyView. I think the overhead of MountedEmptyView having empty children and environment containers on it is justified for now when considering the simplification we get here.

Copy link
Member

@carson-katri carson-katri left a comment

Choose a reason for hiding this comment

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

👍 Looks good

Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

I see no problems but I was also a tad distracted while reviewing

@MaxDesiatov
Copy link
Collaborator Author

No problem, thanks for the review!

@MaxDesiatov MaxDesiatov merged commit 1271281 into main Jul 29, 2020
@MaxDesiatov MaxDesiatov deleted the mounted-scene branch July 29, 2020 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

Runtime issues with dynamic properties in App types
3 participants