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

Jump to bottom

[feat]: add imperative environment customization support #231

Merged
merged 9 commits into from
Jul 26, 2023
Merged

[feat]: add imperative environment customization support #231

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c67b999
[feat]: add imperative environment customization support
n8chur Jul 13, 2023
bc6dfa7
Fix typo in doc comment (cancel -> remove)
n8chur Jul 24, 2023
b3b6e32
Ensure remove is only called once
n8chur Jul 24, 2023
a693580
Use withExtendedLifetime instead of '_ = ...'
n8chur Jul 24, 2023
ae66e1e
Add explicit 'remove()' test
n8chur Jul 24, 2023
7242f1f
Fix doc comment describing when the remove function is called
n8chur Jul 25, 2023
5b7a48d
Use assertion instead of precondition and fix description
n8chur Jul 25, 2023
39d493d
Apply removal assertion change to customization lifetime
n8chur Jul 25, 2023
8e7df56
Fix comment about what remove does on customization lifetime
n8chur Jul 25, 2023
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
99 changes: 96 additions & 3 deletions ViewEnvironmentUI/Sources/ViewEnvironmentPropagating.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@ extension ViewEnvironmentPropagating {
public var environment: ViewEnvironment {
Copy link
Contributor

Choose a reason for hiding this comment

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

more of a general question about the changeset – is this alternate API mainly for convenience? curious what the motivating examples are (if they exist).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's mainly for convenience. The motivating example is support for what we call "pane-based size class", where a split container VC is able to override the horizontal size class for a particular pane based on the available horizontal width of that pane.

Without this change we'd either need to insert a node between the split container VC and it's children, or introduce a wrapper VC around those children to perform the environment mutation.

var environment = environmentAncestor?.environment ?? .empty

for storedCustomization in customizations {
storedCustomization.customization(&environment)
}

if let observing = self as? ViewEnvironmentObserving {
observing.customize(environment: &environment)
}
Expand Down Expand Up @@ -167,7 +171,7 @@ extension ViewEnvironmentPropagating {
///
@_spi(ViewEnvironmentWiring)
public func addEnvironmentNeedsUpdateObserver(
_ onNeedsUpdate: @escaping (ViewEnvironment) -> Void
_ onNeedsUpdate: @escaping ViewEnvironmentUpdateObservation
) -> ViewEnvironmentUpdateObservationLifetime {
let object = ViewEnvironmentUpdateObservationKey()
needsUpdateObservers[object] = onNeedsUpdate
Expand All @@ -176,6 +180,31 @@ extension ViewEnvironmentPropagating {
}
}

/// Adds a `ViewEnvironment` customization to this node.
///
/// These customizations will occur before the node's `customize(environment:)` in cases where
/// this node conforms to `ViewEnvironmentObserving`, and will occur the order in which they
/// were added.
///
/// The customization will only be active for as long as the returned lifetime is retained or
/// until `remove()` is called on it.
///
@_spi(ViewEnvironmentWiring)
public func addEnvironmentCustomization(
Copy link
Contributor

Choose a reason for hiding this comment

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

just to better my own understanding – should this trigger the 'setNeedsEnvironmentUpdate' (or whatever its name is) logic, or does that responsibility lie at a different level?

Copy link
Collaborator Author

@n8chur n8chur Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah—I'm fairly torn on whether or not this should automatically call setNeedsEnvironmentUpdate() on your behalf. You'll almost always want to call it after adding a customization, but I figured there may be cases where you'd prefer to defer that call or manually manage when the invalidation occurs (e.g. for performance reasons). setNeedsEnvironmentUpdate() does trigger environmentDidChange() calls which are not deferred to the next layout cycle, and we use this to perform updates in some cases within WorkflowUI and Market. For example, WorkflowHostingController will re-render its content whenever environmentDidChange() is called.

The current shape also makes it a bit easier to ensure events happen at more appropriate times. For example, if you were to be adding a customization to a new child VC in a container VC, you might have something like this:

addChild(childVC)
customizationLifetime = childVC
    .addEnvironmentCustomization(makeCustomization(for: column))
if let view = viewIfLoaded {
    view.add(childVC.view)
}
childVC.didMove(toParent: self)
childVC.setNeedsEnvironmentUpdate()

This order of operations means:

  • The child VC is added as a child and the customization is added before the child VC's view is (conditionally) loaded. This ensures any environment access during viewDidLoad would have the appropriate environment wiring path and customizations.
  • setNeedsEnvironmentUpdate() is called after the parent/child relationship is fully established (didMove(toParent:) has been called). I'm not aware of any real issues that could come from calling this before didMove(toParent:), but it seems like the most appropriate time in the VC lifecycle IMO.

I'm very open to automatically calling setNeedsEnvironmentUpdate() automatically when a customization is added if we prefer the safety here!

_ customization: @escaping ViewEnvironmentCustomization
) -> ViewEnvironmentCustomizationLifetime {
let storedCustomization = StoredViewEnvironmentCustomization(customization: customization)
customizations.append(storedCustomization)
return .init { [weak self] in
guard let self,
let index = self.customizations.firstIndex(where: { $0 === storedCustomization })
else {
return
}
self.customizations.remove(at: index)
}
}

/// The `ViewEnvironment` propagation ancestor.
///
/// This describes the ancestor that the `ViewEnvironment` is inherited from.
Expand Down Expand Up @@ -351,17 +380,21 @@ public final class ViewEnvironmentUpdateObservationLifetime {
/// This is called in `deinit`.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still true?

Copy link
Collaborator Author

@n8chur n8chur Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, yeah I should add some nuance to this comment. The closure provided for onRemove is only called in deinit if it was not called manually.

  • (7242f1f) Improve doc comment regarding remove() being called in deinit

///
public func remove() {
guard let onRemove else {
preconditionFailure("Environment customization was already removed")
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like an assert warning about unexpected behavior may be more appropriate. this is idempotent, right (other than the precondition)? will something break horribly if it's called more than once?

Copy link
Collaborator Author

@n8chur n8chur Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Nothing would break if this was called more than once with the current implementation.

I'm fine with moving this to an assert with early return, but I'll check with @watt who suggested this change!

  • (5b7a48d) Use assert instead of preconditionFailure?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Asserting is fine with me. I think the idea here is that we consider it a programmer error to try to remove a customization more than once, even if it is effectively idempotent.

Copy link
Collaborator Author

@n8chur n8chur Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oh whoops, I added this to the wrong lifetime 🤦

  • (39d493d) Apply removal assertion changes to customization lifetime

}
self.onRemove = nil
onRemove()
}

private let onRemove: () -> Void
private var onRemove: (() -> Void)?

init(onRemove: @escaping () -> Void) {
self.onRemove = onRemove
}

deinit {
remove()
onRemove?()
}
}

Expand All @@ -370,6 +403,7 @@ private enum ViewEnvironmentPropagatingNSObjectAssociatedKeys {
static var needsUpdateObservers = NSObject()
static var ancestorOverride = NSObject()
static var descendantsOverride = NSObject()
static var customizations = NSObject()
}

extension ViewEnvironmentPropagating {
Expand Down Expand Up @@ -432,3 +466,62 @@ extension ViewEnvironmentPropagating {
}

private class ViewEnvironmentUpdateObservationKey: NSObject {}

/// A closure that customizes the `ViewEnvironment` as it flows through a propagation node.
///
public typealias ViewEnvironmentCustomization = (inout ViewEnvironment) -> Void

/// Describes the lifetime of a `ViewEnvironment` customization.
///
/// This customization will be removed when `remove()` is called or the lifetime token is
/// de-initialized.
///
/// ## SeeAlso ##
/// - `addEnvironmentCustomization(_:)`
///
public final class ViewEnvironmentCustomizationLifetime {
/// Removes the observation.
///
/// This is called in `deinit`.
///
public func remove() {
onRemove()
}

private let onRemove: () -> Void
n8chur marked this conversation as resolved.
Show resolved Hide resolved

init(onRemove: @escaping () -> Void) {
self.onRemove = onRemove
}

deinit {
remove()
}
}

extension ViewEnvironmentPropagating {
fileprivate var customizations: [StoredViewEnvironmentCustomization] {
get {
objc_getAssociatedObject(
self,
&AssociatedKeys.customizations
) as? [StoredViewEnvironmentCustomization] ?? []
}
set {
objc_setAssociatedObject(
self,
&AssociatedKeys.customizations,
newValue,
.OBJC_ASSOCIATION_RETAIN_NONATOMIC
)
}
}
}

private final class StoredViewEnvironmentCustomization {
var customization: ViewEnvironmentCustomization

init(customization: @escaping ViewEnvironmentCustomization) {
self.customization = customization
}
}
99 changes: 92 additions & 7 deletions ViewEnvironmentUI/Tests/ViewEnvironmentObservingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ final class ViewEnvironmentObservingTests: XCTestCase {

var leafEnvironmentDidChangeCallCount = 0
let leafNode = ViewEnvironmentPropagationNode(
environmentAncestor: { [weak viewController] in
environmentAncestor: { [weak viewController] in
viewController
},
environmentDidChange: { _ in
Expand Down Expand Up @@ -226,22 +226,22 @@ final class ViewEnvironmentObservingTests: XCTestCase {

func test_descendant_customFlow() {
let descendant = TestViewEnvironmentObservingViewController()

let viewController = TestViewEnvironmentObservingViewController()
viewController.environmentDescendantsOverride = { [descendant] }

viewController.applyEnvironmentIfNeeded()
descendant.applyEnvironmentIfNeeded()
XCTAssertFalse(viewController.needsEnvironmentUpdate)
XCTAssertFalse(descendant.needsEnvironmentUpdate)

// With no ancestor configured the descendant should not respond to needing update
viewController.setNeedsEnvironmentUpdate()
XCTAssertTrue(viewController.needsEnvironmentUpdate)
XCTAssertFalse(descendant.needsEnvironmentUpdate)

// With an ancestor defined the VC should respond to needing update

descendant.environmentAncestorOverride = { [weak viewController] in
viewController
}
Expand Down Expand Up @@ -318,12 +318,97 @@ final class ViewEnvironmentObservingTests: XCTestCase {
XCTAssertEqual(observedEnvironments.count, 2)
XCTAssertEqual(expectedTestContext, observedEnvironments.last?.testContext)

_ = observation // Suppress warning about variable never being read
withExtendedLifetime(observation) {}
observation = nil

container.setNeedsEnvironmentUpdate()
XCTAssertEqual(observedEnvironments.count, 2)
}

// MARK: - Customizations

func test_customization() throws {
let viewController = TestViewEnvironmentObservingViewController()

// Customizations should be respected as long as the lifetime exists.
do {
var customizationLifetime: ViewEnvironmentCustomizationLifetime? = viewController
.addEnvironmentCustomization {
$0.testContext.number = 200
}

XCTAssertEqual(viewController.environment.testContext.number, 200)

withExtendedLifetime(customizationLifetime) {}
customizationLifetime = nil

// Customization should be removed when lifetime is deallocated
n8chur marked this conversation as resolved.
Show resolved Hide resolved
XCTAssertEqual(
viewController.environment.testContext.number,
TestContextKey.defaultValue.number
)
}

// Customizations should be respected until `remove()` is called on the lifetime.
do {
var customizationLifetime: ViewEnvironmentCustomizationLifetime? = viewController
.addEnvironmentCustomization {
$0.testContext.number = 200
}

XCTAssertEqual(viewController.environment.testContext.number, 200)

customizationLifetime?.remove()

// Customization should be removed when lifetime is deallocated
XCTAssertEqual(
viewController.environment.testContext.number,
TestContextKey.defaultValue.number
)

withExtendedLifetime(customizationLifetime) {}
customizationLifetime = nil
}

// Customizations should occur in the order they are added
do {
var customization1Lifetime: ViewEnvironmentCustomizationLifetime? = viewController
.addEnvironmentCustomization {
$0.testContext.number = 100
}

var customization2Lifetime: ViewEnvironmentCustomizationLifetime? = viewController
.addEnvironmentCustomization {
$0.testContext.number = 200
}

XCTAssertEqual(viewController.environment.testContext.number, 200)

withExtendedLifetime(customization1Lifetime) {}
customization1Lifetime = nil
withExtendedLifetime(customization2Lifetime) {}
customization2Lifetime = nil
}

// Customizations should favor the nodes customizations if present
do {
viewController.customizeEnvironment = {
$0.testContext.number = 300
}

var customizationLifetime: ViewEnvironmentCustomizationLifetime? = viewController
.addEnvironmentCustomization {
$0.testContext.number = 200
$0.testContext.string = "200"
}

XCTAssertEqual(viewController.environment.testContext.number, 300)
XCTAssertEqual(viewController.environment.testContext.string, "200")

withExtendedLifetime(customizationLifetime) {}
customizationLifetime = nil
}
}
}

// MARK: - Helpers
Expand Down