From 637aa1e038d4c3862e1181b318234b7049116ea9 Mon Sep 17 00:00:00 2001 From: Cameron Askew Date: Thu, 2 Aug 2018 13:14:17 -0700 Subject: [PATCH 1/2] Fix bug where a parent LK node purges child LK node's subviews --- LayoutKitTests/ViewRecyclerTests.swift | 32 ++++++++++++++++++++++---- Sources/LayoutArrangement.swift | 6 ++--- Sources/ViewRecycler.swift | 16 +++++++------ 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/LayoutKitTests/ViewRecyclerTests.swift b/LayoutKitTests/ViewRecyclerTests.swift index 5f4e4541..94204f4a 100644 --- a/LayoutKitTests/ViewRecyclerTests.swift +++ b/LayoutKitTests/ViewRecyclerTests.swift @@ -14,7 +14,7 @@ class ViewRecyclerTests: XCTestCase { func testNilIdNotRecycledAndNotRemoved() { let root = View() let zero = View() - zero.isLayoutKitView = false // default + zero.layoutKitRootView = nil // default root.addSubview(zero) let recycler = ViewRecycler(rootView: root) @@ -25,13 +25,13 @@ class ViewRecyclerTests: XCTestCase { XCTAssertEqual(v, expectedView) recycler.purgeViews() - XCTAssertNotNil(zero.superview, "`zero` should not be removed because `isLayoutKitView` is false") + XCTAssertNotNil(zero.superview, "`zero` should not be removed because `layoutKitRootView` is nil") } func testNilIdNotRecycledAndRemoved() { let root = View() let zero = View() - zero.isLayoutKitView = true // requires this flag to be removed by `ViewRecycler` + zero.layoutKitRootView = root root.addSubview(zero) let recycler = ViewRecycler(rootView: root) @@ -42,7 +42,7 @@ class ViewRecyclerTests: XCTestCase { XCTAssertEqual(v, expectedView) recycler.purgeViews() - XCTAssertNil(zero.superview, "`zero` should be removed because `isLayoutKitView` is true") + XCTAssertNil(zero.superview, "`zero` should be removed because `layoutKitRootView` is set") } func testNonNilIdRecycled() { @@ -91,6 +91,30 @@ class ViewRecyclerTests: XCTestCase { XCTAssertNotNil(one.superview) } + func testDoesntPurgeOtherNodesSubviews() { + let rootOne = View(viewReuseId: "1") + let middleView = View() + let rootTwo = View(viewReuseId: "2") + let someSubview = View() + + rootOne.addSubview(middleView) + middleView.addSubview(rootTwo) + rootTwo.addSubview(someSubview) + + let recycler = ViewRecycler(rootView: rootOne) + _ = recycler.makeOrRecycleView(havingViewReuseId: "3") { + return middleView + } + + let recycler2 = ViewRecycler(rootView: rootTwo) + _ = recycler2.makeOrRecycleView(havingViewReuseId: "4") { + return someSubview + } + + recycler.purgeViews() + XCTAssertEqual(someSubview.superview, rootTwo) + } + /// Test for safe subview-purge in composite view e.g. UIButton. /// - SeeAlso: https://github.com/linkedin/LayoutKit/pull/85 #if os(iOS) || os(tvOS) diff --git a/Sources/LayoutArrangement.swift b/Sources/LayoutArrangement.swift index 94289741..6c9c79f7 100644 --- a/Sources/LayoutArrangement.swift +++ b/Sources/LayoutArrangement.swift @@ -72,7 +72,7 @@ public struct LayoutArrangement { @discardableResult private func makeViews(in view: View? = nil, direction: UserInterfaceLayoutDirection, prepareAnimation: Bool) -> View { let recycler = ViewRecycler(rootView: view) - let views = makeSubviews(from: recycler, prepareAnimation: prepareAnimation) + let views = makeSubviews(from: recycler, prepareAnimation: prepareAnimation, rootView: view) let rootView: View if let view = view { @@ -114,9 +114,9 @@ public struct LayoutArrangement { } /// Returns the views for the layout and all of its sublayouts. - private func makeSubviews(from recycler: ViewRecycler, prepareAnimation: Bool) -> [View] { + private func makeSubviews(from recycler: ViewRecycler, prepareAnimation: Bool, rootView: View?) -> [View] { let subviews = sublayouts.flatMap({ (sublayout: LayoutArrangement) -> [View] in - return sublayout.makeSubviews(from: recycler, prepareAnimation: prepareAnimation) + return sublayout.makeSubviews(from: recycler, prepareAnimation: prepareAnimation, rootView: rootView) }) // If we are preparing an animation, then we don't want to update frames or configure views. if layout.needsView, let view = recycler.makeOrRecycleView(havingViewReuseId: layout.viewReuseId, viewProvider: layout.makeView) { diff --git a/Sources/ViewRecycler.swift b/Sources/ViewRecycler.swift index 1f62ab92..1cc8ecd8 100644 --- a/Sources/ViewRecycler.swift +++ b/Sources/ViewRecycler.swift @@ -20,9 +20,11 @@ class ViewRecycler { private var viewsById = [String: View]() private var unidentifiedViews = Set() + private let rootView: View? /// Retains all subviews of rootView for recycling. init(rootView: View?) { + self.rootView = rootView rootView?.walkSubviews { (view) in if let viewReuseId = view.viewReuseId { self.viewsById[viewReuseId] = view @@ -44,7 +46,7 @@ class ViewRecycler { } let providedView = viewProvider() - providedView.isLayoutKitView = true + providedView.layoutKitRootView = rootView // Remove the provided view from the list of cached views. if let viewReuseId = providedView.viewReuseId, let oldView = viewsById[viewReuseId], oldView == providedView { @@ -63,7 +65,7 @@ class ViewRecycler { } viewsById.removeAll() - for view in unidentifiedViews where view.isLayoutKitView { + for view in unidentifiedViews where view.layoutKitRootView == rootView { view.removeFromSuperview() } unidentifiedViews.removeAll() @@ -71,7 +73,7 @@ class ViewRecycler { } private var viewReuseIdKey: UInt8 = 0 -private var isLayoutKitViewKey: UInt8 = 0 +private var layoutKitRootViewKey: UInt8 = 0 extension View { @@ -93,13 +95,13 @@ extension View { } } - /// Indicates the view is managed by LayoutKit that can be safely removed. - var isLayoutKitView: Bool { + /// We must keep track of the root view associated with the views we manage so that we only purge from its hierarchy. + var layoutKitRootView: View? { get { - return (objc_getAssociatedObject(self, &isLayoutKitViewKey) as? NSNumber)?.boolValue ?? false + return objc_getAssociatedObject(self, &layoutKitRootViewKey) as? View } set { - objc_setAssociatedObject(self, &isLayoutKitViewKey, NSNumber(value: newValue), .OBJC_ASSOCIATION_RETAIN) + objc_setAssociatedObject(self, &layoutKitRootViewKey, newValue, .OBJC_ASSOCIATION_ASSIGN) } } } From e05b32306de70b7b495bc3c99550ae440c4d9590 Mon Sep 17 00:00:00 2001 From: Cameron Askew Date: Thu, 2 Aug 2018 15:01:00 -0700 Subject: [PATCH 2/2] add comments, clean unused parameter, add root node namespace for reuseId, add test --- LayoutKitTests/ViewRecyclerTests.swift | 28 ++++++++++++++++++++++++++ Sources/LayoutArrangement.swift | 6 +++--- Sources/ViewRecycler.swift | 8 ++++++-- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/LayoutKitTests/ViewRecyclerTests.swift b/LayoutKitTests/ViewRecyclerTests.swift index 94204f4a..999d5d04 100644 --- a/LayoutKitTests/ViewRecyclerTests.swift +++ b/LayoutKitTests/ViewRecyclerTests.swift @@ -49,6 +49,7 @@ class ViewRecyclerTests: XCTestCase { let root = View() let one = View(viewReuseId: "1") root.addSubview(one) + one.layoutKitRootView = root let recycler = ViewRecycler(rootView: root) let v: View? = recycler.makeOrRecycleView(havingViewReuseId: "1", viewProvider: { @@ -91,6 +92,8 @@ class ViewRecyclerTests: XCTestCase { XCTAssertNotNil(one.superview) } + /// Create a hierarchy (rootOne > middleView > rootTwo > someSubview) where rootOne and rootTwo + /// are LayoutKit root nodes. Then ensure that purging rootOne doesn't remove someSubview. func testDoesntPurgeOtherNodesSubviews() { let rootOne = View(viewReuseId: "1") let middleView = View() @@ -115,6 +118,30 @@ class ViewRecyclerTests: XCTestCase { XCTAssertEqual(someSubview.superview, rootTwo) } + /// Create a hierarchy (rootOne > middleView > rootTwo > someSubview) where rootOne and rootTwo + /// are LayoutKit root nodes. Then ensure that rootOne can't recycle someSubview. + func testDoesntRecycleOtherNodesViews() { + let rootOne = View(viewReuseId: "1") + let middleView = View(viewReuseId: "2") + let rootTwo = View(viewReuseId: "3") + let someSubview = View(viewReuseId: "4") + + rootOne.addSubview(middleView) + middleView.layoutKitRootView = rootOne + + middleView.addSubview(rootTwo) + + rootTwo.addSubview(someSubview) + someSubview.layoutKitRootView = rootTwo + + let recycler = ViewRecycler(rootView: rootOne) + let aDifferentView = View() + let v: View? = recycler.makeOrRecycleView(havingViewReuseId: "4") { + return aDifferentView + } + XCTAssertEqual(v, aDifferentView) + } + /// Test for safe subview-purge in composite view e.g. UIButton. /// - SeeAlso: https://github.com/linkedin/LayoutKit/pull/85 #if os(iOS) || os(tvOS) @@ -122,6 +149,7 @@ class ViewRecyclerTests: XCTestCase { let root = View() let button = UIButton(viewReuseId: "1") root.addSubview(button) + button.layoutKitRootView = root button.setTitle("dummy", for: .normal) button.layoutIfNeeded() diff --git a/Sources/LayoutArrangement.swift b/Sources/LayoutArrangement.swift index 6c9c79f7..94289741 100644 --- a/Sources/LayoutArrangement.swift +++ b/Sources/LayoutArrangement.swift @@ -72,7 +72,7 @@ public struct LayoutArrangement { @discardableResult private func makeViews(in view: View? = nil, direction: UserInterfaceLayoutDirection, prepareAnimation: Bool) -> View { let recycler = ViewRecycler(rootView: view) - let views = makeSubviews(from: recycler, prepareAnimation: prepareAnimation, rootView: view) + let views = makeSubviews(from: recycler, prepareAnimation: prepareAnimation) let rootView: View if let view = view { @@ -114,9 +114,9 @@ public struct LayoutArrangement { } /// Returns the views for the layout and all of its sublayouts. - private func makeSubviews(from recycler: ViewRecycler, prepareAnimation: Bool, rootView: View?) -> [View] { + private func makeSubviews(from recycler: ViewRecycler, prepareAnimation: Bool) -> [View] { let subviews = sublayouts.flatMap({ (sublayout: LayoutArrangement) -> [View] in - return sublayout.makeSubviews(from: recycler, prepareAnimation: prepareAnimation, rootView: rootView) + return sublayout.makeSubviews(from: recycler, prepareAnimation: prepareAnimation) }) // If we are preparing an animation, then we don't want to update frames or configure views. if layout.needsView, let view = recycler.makeOrRecycleView(havingViewReuseId: layout.viewReuseId, viewProvider: layout.makeView) { diff --git a/Sources/ViewRecycler.swift b/Sources/ViewRecycler.swift index 1cc8ecd8..79817144 100644 --- a/Sources/ViewRecycler.swift +++ b/Sources/ViewRecycler.swift @@ -26,7 +26,7 @@ class ViewRecycler { init(rootView: View?) { self.rootView = rootView rootView?.walkSubviews { (view) in - if let viewReuseId = view.viewReuseId { + if let viewReuseId = view.viewReuseId, view.layoutKitRootView == rootView { self.viewsById[viewReuseId] = view } else { self.unidentifiedViews.insert(view) @@ -95,12 +95,16 @@ extension View { } } - /// We must keep track of the root view associated with the views we manage so that we only purge from its hierarchy. + /// All views managed by LayoutKit keep a weak reference to their LayoutKit root node view. This is to ensure + /// that when we purge views from a node, we only purge views that belong to that node - rather than purging + /// potentially any view in the subview hierarchy which could belong to a different LayoutKit root node. var layoutKitRootView: View? { get { return objc_getAssociatedObject(self, &layoutKitRootViewKey) as? View } set { + // OBJC_ASSOCIATION_ASSIGN creates a weak reference which avoids a retain cycle since we'll + // have a view being referenced by another view which is somewhere in its subview hierarchy. objc_setAssociatedObject(self, &layoutKitRootViewKey, newValue, .OBJC_ASSOCIATION_ASSIGN) } }