diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b977e493..150f8fb3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ - Prevent UITextView from updating contentOffset while deallocating [Michael Schneider](https://github.com/maicki) - [ASCollectionNode/ASTableNode] Fix a crash occurs while remeasuring cell nodes. [Huy Nguyen](https://github.com/nguyenhuy) [#917](https://github.com/TextureGroup/Texture/pull/917) - Fix an issue where ASConfigurationDelegate would not call out for "control" users. If set, it now receives events whenever an experimental feature decision point occurs, whether it's enabled or not. [Adlai Holler](https://github.com/Adlai-Holler) +- [ASDisplayNode] Fix an issue that causes a node to sometimes return an outdated calculated size or size range. [Huy Nguyen](https://github.com/nguyenhuy) [#808](https://github.com/TextureGroup/Texture/pull/808) ## 2.6 - [Xcode 9] Updated to require Xcode 9 (to fix warnings) [Garrett Moon](https://github.com/garrettmoon) diff --git a/Source/ASDisplayNode+Layout.mm b/Source/ASDisplayNode+Layout.mm index d656a8983..6227f3f08 100644 --- a/Source/ASDisplayNode+Layout.mm +++ b/Source/ASDisplayNode+Layout.mm @@ -170,7 +170,7 @@ - (ASLayout *)calculatedLayout - (CGSize)calculatedSize { ASDN::MutexLocker l(__instanceLock__); - if (_pendingDisplayNodeLayout != nullptr) { + if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValid(_layoutVersion)) { return _pendingDisplayNodeLayout->layout.size; } return _calculatedDisplayNodeLayout->layout.size; @@ -184,7 +184,7 @@ - (ASSizeRange)constrainedSizeForCalculatedLayout - (ASSizeRange)_locked_constrainedSizeForCalculatedLayout { - if (_pendingDisplayNodeLayout != nullptr) { + if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValid(_layoutVersion)) { return _pendingDisplayNodeLayout->constrainedSize; } return _calculatedDisplayNodeLayout->constrainedSize; @@ -304,24 +304,22 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds } CGSize boundsSizeForLayout = ASCeilSizeValues(bounds.size); - NSUInteger calculatedVersion = _calculatedDisplayNodeLayout->version; // Prefer a newer and not yet applied _pendingDisplayNodeLayout over _calculatedDisplayNodeLayout // If there is no such _pending, check if _calculated is valid to reuse (avoiding recalculation below). BOOL pendingLayoutIsPreferred = NO; - if (_pendingDisplayNodeLayout != nullptr) { + if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValid(_layoutVersion)) { + NSUInteger calculatedVersion = _calculatedDisplayNodeLayout->version; NSUInteger pendingVersion = _pendingDisplayNodeLayout->version; - if (pendingVersion >= _layoutVersion) { - if (pendingVersion > calculatedVersion) { - pendingLayoutIsPreferred = YES; // Newer _pending - } else if (pendingVersion == calculatedVersion - && !ASSizeRangeEqualToSizeRange(_pendingDisplayNodeLayout->constrainedSize, - _calculatedDisplayNodeLayout->constrainedSize)) { - pendingLayoutIsPreferred = YES; // _pending with a different constrained size - } - } - } - BOOL calculatedLayoutIsReusable = (calculatedVersion >= _layoutVersion + if (pendingVersion > calculatedVersion) { + pendingLayoutIsPreferred = YES; // Newer _pending + } else if (pendingVersion == calculatedVersion + && !ASSizeRangeEqualToSizeRange(_pendingDisplayNodeLayout->constrainedSize, + _calculatedDisplayNodeLayout->constrainedSize)) { + pendingLayoutIsPreferred = YES; // _pending with a different constrained size + } + } + BOOL calculatedLayoutIsReusable = (_calculatedDisplayNodeLayout->isValid(_layoutVersion) && (_calculatedDisplayNodeLayout->requestedLayoutFromAbove || CGSizeEqualToSize(_calculatedDisplayNodeLayout->layout.size, boundsSizeForLayout))); if (!pendingLayoutIsPreferred && calculatedLayoutIsReusable) { diff --git a/Source/Private/ASDisplayNodeLayout.h b/Source/Private/ASDisplayNodeLayout.h index 3bc7782a7..45d15230a 100644 --- a/Source/Private/ASDisplayNodeLayout.h +++ b/Source/Private/ASDisplayNodeLayout.h @@ -47,7 +47,12 @@ struct ASDisplayNodeLayout { */ ASDisplayNodeLayout() : layout(nil), constrainedSize({{0, 0}, {0, 0}}), parentSize({0, 0}), requestedLayoutFromAbove(NO), version(0) {}; - + + /** + * Returns whether this is valid for a given version + */ + BOOL isValid(NSUInteger version); + /** * Returns whether this is valid for a given constrained size, parent size, and version */ diff --git a/Source/Private/ASDisplayNodeLayout.mm b/Source/Private/ASDisplayNodeLayout.mm index 694adab0b..15d51fd49 100644 --- a/Source/Private/ASDisplayNodeLayout.mm +++ b/Source/Private/ASDisplayNodeLayout.mm @@ -17,10 +17,14 @@ #import +BOOL ASDisplayNodeLayout::isValid(NSUInteger versionArg) +{ + return layout != nil && version >= versionArg; +} + BOOL ASDisplayNodeLayout::isValid(ASSizeRange theConstrainedSize, CGSize theParentSize, NSUInteger versionArg) { - return version >= versionArg - && layout != nil + return isValid(versionArg) && CGSizeEqualToSize(parentSize, theParentSize) && ASSizeRangeEqualToSizeRange(constrainedSize, theConstrainedSize); } diff --git a/Tests/ASLayoutEngineTests.mm b/Tests/ASLayoutEngineTests.mm index 550dffa47..719a84d5e 100644 --- a/Tests/ASLayoutEngineTests.mm +++ b/Tests/ASLayoutEngineTests.mm @@ -29,11 +29,12 @@ @implementation ASLayoutEngineTests { ASTLayoutFixture *fixture2; ASTLayoutFixture *fixture3; ASTLayoutFixture *fixture4; + ASTLayoutFixture *fixture5; - // fixtures 1 and 3 share the same exact node A layout spec block. + // fixtures 1, 3 and 5 share the same exact node A layout spec block. // we don't want the infra to call -setNeedsLayout when we switch fixtures // so we need to use the same exact block. - ASLayoutSpecBlock fixture1and3NodeALayoutSpecBlock; + ASLayoutSpecBlock fixture1and3and5NodeALayoutSpecBlock; UIWindow *window; UIViewController *vc; @@ -68,11 +69,12 @@ - (void)setUp ASLayoutSpecBlock b = ^ASLayoutSpec * _Nonnull(__kindof ASDisplayNode * _Nonnull node, ASSizeRange constrainedSize) { return [ASStackLayoutSpec stackLayoutSpecWithDirection:ASStackLayoutDirectionHorizontal spacing:0 justifyContent:ASStackLayoutJustifyContentSpaceBetween alignItems:ASStackLayoutAlignItemsStart children:@[ nodeB, nodeC, nodeD ]]; }; - fixture1and3NodeALayoutSpecBlock = b; + fixture1and3and5NodeALayoutSpecBlock = b; fixture1 = [self createFixture1]; fixture2 = [self createFixture2]; fixture3 = [self createFixture3]; fixture4 = [self createFixture4]; + fixture5 = [self createFixture5]; nodeA.frame = vc.view.bounds; nodeA.autoresizingMask = UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight; @@ -181,6 +183,55 @@ - (void)testLayoutTransitionWithAsyncMeasurement [self verifyFixture:fixture2]; } +/** + * Transition from fixture1 to Fixture2 on node A. + * + * Expect A and D to calculate once on main, and + * to receive calculatedLayoutDidChange on main, + * then to get animateLayoutTransition: and didCompleteLayoutTransition: on main. + */ +- (void)testLayoutTransitionWithSyncMeasurement +{ + [self stubCalculatedLayoutDidChange]; + + // Precondition + XCTAssertFalse(CGSizeEqualToSize(fixture5.layout.size, fixture1.layout.size)); + + // First, apply fixture 5 and run a measurement pass, but don't run a layout pass + // After this step, nodes will have pending layouts that are not yet applied + [fixture5 apply]; + [fixture5 withSizeRangesForAllNodesUsingBlock:^(ASLayoutTestNode * _Nonnull node, ASSizeRange sizeRange) { + OCMExpect([node.mock calculateLayoutThatFits:sizeRange]) + .onMainThread(); + }]; + + [nodeA layoutThatFits:ASSizeRangeMake(fixture5.layout.size)]; + + // Assert that node A has layout size and size range from fixture 5 + XCTAssertTrue(CGSizeEqualToSize(fixture5.layout.size, nodeA.calculatedSize)); + XCTAssertTrue(ASSizeRangeEqualToSizeRange([fixture5 firstSizeRangeForNode:nodeA], nodeA.constrainedSizeForCalculatedLayout)); + + // Then switch to fixture 1 and kick off a synchronous layout transition + // Unapplied pending layouts from the previous measurement pass will be outdated + [fixture1 apply]; + [fixture1 withSizeRangesForAllNodesUsingBlock:^(ASLayoutTestNode * _Nonnull node, ASSizeRange sizeRange) { + OCMExpect([node.mock calculateLayoutThatFits:sizeRange]) + .onMainThread(); + }]; + + OCMExpect([nodeA.mock animateLayoutTransition:OCMOCK_ANY]).onMainThread(); + OCMExpect([nodeA.mock didCompleteLayoutTransition:OCMOCK_ANY]).onMainThread(); + + [nodeA transitionLayoutWithAnimation:NO shouldMeasureAsync:NO measurementCompletion:nil]; + + // Assert that node A picks up new layout size and size range from fixture 1 + XCTAssertTrue(CGSizeEqualToSize(fixture1.layout.size, nodeA.calculatedSize)); + XCTAssertTrue(ASSizeRangeEqualToSizeRange([fixture1 firstSizeRangeForNode:nodeA], nodeA.constrainedSizeForCalculatedLayout)); + + [window layoutIfNeeded]; + [self verifyFixture:fixture1]; +} + /** * Start at fixture 1. * Trigger an async transition to fixture 2. @@ -351,7 +402,7 @@ - (void)stubCalculatedLayoutDidChange /** * Fixture 1: A basic horizontal stack, all single-pass. * - * [A: HorizStack([B, C, D])]. B is (1x1), C is (2x1), D is (1x1) + * [A: HorizStack([B, C, D])]. A is (10x1), B is (1x1), C is (2x1), D is (1x1) */ - (ASTLayoutFixture *)createFixture1 { @@ -373,14 +424,14 @@ - (ASTLayoutFixture *)createFixture1 auto layoutA = [ASLayout layoutWithLayoutElement:nodeA size:{10,1} position:ASPointNull sublayouts:@[ layoutB, layoutC, layoutD ]]; fixture.layout = layoutA; - [fixture.layoutSpecBlocks setObject:fixture1and3NodeALayoutSpecBlock forKey:nodeA]; + [fixture.layoutSpecBlocks setObject:fixture1and3and5NodeALayoutSpecBlock forKey:nodeA]; return fixture; } /** * Fixture 2: A simple transition away from fixture 1. * - * [A: HorizStack([B, C, E])]. B is (1x1), C is (4x1), E is (1x1) + * [A: HorizStack([B, C, E])]. A is (10x1), B is (1x1), C is (4x1), E is (1x1) * * From fixture 1: * B survives with same layout @@ -418,7 +469,7 @@ - (ASTLayoutFixture *)createFixture2 /** * Fixture 3: Multipass stack layout * - * [A: HorizStack([B, C, D])]. B is (7x1), C is (2x1), D is (1x1) + * [A: HorizStack([B, C, D])]. A is (10x1), B is (7x1), C is (2x1), D is (1x1) * * nodeB (which has flexShrink=1) will return 8x1 for its size during the first * stack pass, and it'll be subject to a second pass where it returns 7x1. @@ -446,14 +497,14 @@ - (ASTLayoutFixture *)createFixture3 auto layoutA = [ASLayout layoutWithLayoutElement:nodeA size:{10,1} position:ASPointNull sublayouts:@[ layoutB, layoutC, layoutD ]]; fixture.layout = layoutA; - [fixture.layoutSpecBlocks setObject:fixture1and3NodeALayoutSpecBlock forKey:nodeA]; + [fixture.layoutSpecBlocks setObject:fixture1and3and5NodeALayoutSpecBlock forKey:nodeA]; return fixture; } /** * Fixture 4: A different simple transition away from fixture 1. * - * [A: HorizStack([B, D, E])]. B is (1x1), D is (2x1), E is (1x1) + * [A: HorizStack([B, D, E])]. A is (10x1), B is (1x1), D is (2x1), E is (1x1) * * From fixture 1: * B survives with same layout @@ -494,18 +545,45 @@ - (ASTLayoutFixture *)createFixture4 return fixture; } +/** + * Fixture 5: Same as fixture 1, but with a bigger root node (node A). + * + * [A: HorizStack([B, C, D])]. A is (15x1), B is (1x1), C is (2x1), D is (1x1) + */ +- (ASTLayoutFixture *)createFixture5 +{ + auto fixture = [[ASTLayoutFixture alloc] init]; + + // nodeB + [fixture addSizeRange:{{0, 0}, {INFINITY, 1}} forNode:nodeB]; + auto layoutB = [ASLayout layoutWithLayoutElement:nodeB size:{1,1} position:{0,0} sublayouts:nil]; + + // nodeC + [fixture addSizeRange:{{0, 0}, {INFINITY, 1}} forNode:nodeC]; + auto layoutC = [ASLayout layoutWithLayoutElement:nodeC size:{2,1} position:{4,0} sublayouts:nil]; + + // nodeD + [fixture addSizeRange:{{0, 0}, {INFINITY, 1}} forNode:nodeD]; + auto layoutD = [ASLayout layoutWithLayoutElement:nodeD size:{1,1} position:{9,0} sublayouts:nil]; + + [fixture addSizeRange:{{15, 1}, {15, 1}} forNode:nodeA]; + auto layoutA = [ASLayout layoutWithLayoutElement:nodeA size:{15,1} position:ASPointNull sublayouts:@[ layoutB, layoutC, layoutD ]]; + fixture.layout = layoutA; + + [fixture.layoutSpecBlocks setObject:fixture1and3and5NodeALayoutSpecBlock forKey:nodeA]; + return fixture; +} + - (void)runFirstLayoutPassWithFixture:(ASTLayoutFixture *)fixture { [fixture apply]; - for (ASLayoutTestNode *node in fixture.allNodes) { - [fixture withSizeRangesForNode:node block:^(ASSizeRange sizeRange) { - OCMExpect([node.mock calculateLayoutThatFits:sizeRange]).onMainThread(); - }]; + [fixture withSizeRangesForAllNodesUsingBlock:^(ASLayoutTestNode * _Nonnull node, ASSizeRange sizeRange) { + OCMExpect([node.mock calculateLayoutThatFits:sizeRange]).onMainThread(); if (!stubbedCalculatedLayoutDidChange) { OCMExpect([node.mock calculatedLayoutDidChange]).onMainThread(); } - } + }]; // Trigger CA layout pass. [window layoutIfNeeded]; diff --git a/Tests/ASTLayoutFixture.h b/Tests/ASTLayoutFixture.h index ef590220a..3b561baf5 100644 --- a/Tests/ASTLayoutFixture.h +++ b/Tests/ASTLayoutFixture.h @@ -44,6 +44,9 @@ AS_SUBCLASSING_RESTRICTED /// Get the first expected size range for the node. - (ASSizeRange)firstSizeRangeForNode:(ASLayoutTestNode *)node; +/// Enumerate all the size ranges for all the nodes using the provided block. +- (void)withSizeRangesForAllNodesUsingBlock:(void (^)(ASLayoutTestNode *node, ASSizeRange sizeRange))block; + /// Enumerate all the size ranges for the node. - (void)withSizeRangesForNode:(ASLayoutTestNode *)node block:(void (^)(ASSizeRange sizeRange))block; diff --git a/Tests/ASTLayoutFixture.mm b/Tests/ASTLayoutFixture.mm index bdddbe5bf..0881fd068 100644 --- a/Tests/ASTLayoutFixture.mm +++ b/Tests/ASTLayoutFixture.mm @@ -58,6 +58,15 @@ - (ASSizeRange)firstSizeRangeForNode:(ASLayoutTestNode *)node return r; } +- (void)withSizeRangesForAllNodesUsingBlock:(void (^)(ASLayoutTestNode * _Nonnull, ASSizeRange))block +{ + for (ASLayoutTestNode *node in self.allNodes) { + [self withSizeRangesForNode:node block:^(ASSizeRange sizeRange) { + block(node, sizeRange); + }]; + } +} + - (void)withSizeRangesForNode:(ASLayoutTestNode *)node block:(void (^)(ASSizeRange))block { for (NSValue *value in [_sizeRanges objectForKey:node]) {