Make view alignment not sensitive to current frame #146
Conversation
The .align() method computes an offset, and then applies that offset to the receiver's frame. This makes the result sensitive to the *current* frame of the receiver, rather than being an absolute computation. In certain circumstances, this can result in layout that "jitters" across a rounding threshold on each layout pass, instead of always behaving the same way. For example: 1. A label starts out at (0,0) 2. untransformedFrameOffset returns (0,4.83333339) to vertically center the label (note: just *beyond* the half-pixel boundary at 3x resolution) 3. The offset is applied and rounded up to the nearest pixel: (0,5) 4. On an second layout pass, untransformedFrameOffset now returns (0,-1.666667) since it is subtracting from 5 instead of adding to 0 5. The offset is applied, gets (0,4.83333321) and now gets rounded *down* to (0,4.6666667) Signed-off-by: Peter Westen <[email protected]>
| /// - parameter otherView: The other view for the measurement. | ||
| /// - parameter otherPosition: The position in the `otherView`'s untransformed frame to use for the measurement. | ||
| /// - returns: The offset from the receiver's `position` to the `otherView`'s `otherPosition`. | ||
| public func untransformedFrameOrigin( |
There was a problem hiding this comment.
A couple things to consider:
- We should probably eliminate the
untransformedFrameOffsetmethod entirely -- but that's a breaking change - If not, we ought to DRY up the implementations, of course
There was a problem hiding this comment.
- I'm okay with breaking changes. We've been trending towards a breaking release for a bit now.
mainalready has breaking changes on it – example. One big thing though is the documentation for this method does not make it clear how it differs fromuntransformedFrameOffset, so I'm not exactly sure from reading the doc/name how I'd migrate from one to the other. (Haven't read the implementation just yet) - heh, yeah. We've been finding that difficult... been resorting to copy/paste recently
There was a problem hiding this comment.
yeah I'm struggling with this function name + parameter names
| } | ||
|
|
||
| let sourcePoint = receiverContext.pointInBounds(at: position) | ||
| return targetPoint.offset(by: .init(horizontal: -sourcePoint.x, vertical: -sourcePoint.y)) |
There was a problem hiding this comment.
So I think this is correct for the base case of calling superview.untransformedConvert. Are there snapshot tests that work on views with transformations applied?
There was a problem hiding this comment.
You know what I know there. Need to defer to @NickEntin
There was a problem hiding this comment.
these last two lines are the only difference between the above method and this one, right?
| ) | ||
| .offset(by: offset) | ||
| .roundedToPixel(in: receiverView) | ||
| receiverView.untransformedFrame.origin = try untransformedFrameOrigin( |
There was a problem hiding this comment.
There are two ways we could add test coverage here:
- Add a snapshot test with two identical sibling views, calling
.align()once on one of them, and twice on the other - Add a unit test with a specific configuration that triggers the issue
In either event, we should probably add the test in a distinct PR, merge it first, then rebase this PR against it with the fix.
| ) | ||
|
|
||
| case .untransformedFrame, | ||
| .automatic /* where !targetIsInSourceSuperviewChain */: |
There was a problem hiding this comment.
boy oh boy do I wish I knew why this where was commented out in the code we copied.
There was a problem hiding this comment.
It's referencing the opposite where in the case above, probably to avoid an exhaustive-switch error (assuming the compiler isn't quite clever enough to figure that out)
There was a problem hiding this comment.
Yep, exactly. Or at least the compiler wasn't clever enough to figure this out when I wrote it like 4 compiler versions ago 😂 It may have gotten smarter about it since then.
| /// - parameter otherView: The other view for the measurement. | ||
| /// - parameter otherPosition: The position in the `otherView`'s untransformed frame to use for the measurement. | ||
| /// - returns: The offset from the receiver's `position` to the `otherView`'s `otherPosition`. | ||
| public func untransformedFrameOrigin( |
There was a problem hiding this comment.
yeah I'm struggling with this function name + parameter names
| } | ||
|
|
||
| let sourcePoint = receiverContext.pointInBounds(at: position) | ||
| return targetPoint.offset(by: .init(horizontal: -sourcePoint.x, vertical: -sourcePoint.y)) |
There was a problem hiding this comment.
these last two lines are the only difference between the above method and this one, right?
| /// - parameter position: The position in the receiving view's untransformed frame. | ||
| /// - parameter otherView: The other view for the measurement. | ||
| /// - parameter otherPosition: The position in the `otherView`'s untransformed frame to use for the measurement. | ||
| /// - returns: The offset from the receiver's `position` to the `otherView`'s `otherPosition`. |
There was a problem hiding this comment.
Heh, copy-pasta of the other method's comments. Needs updating
| ) | ||
| } | ||
|
|
||
| let sourcePoint = receiverContext.pointInBounds(at: position) |
There was a problem hiding this comment.
I don't think this line is quite right. targetPoint is defined in the superview's coordinate space, and here we're getting the sourcePoint in the receiver's coordinate space. We're missing the untransformedConvert that was here before.
There was a problem hiding this comment.
(although you can't just add back the untransformedConvert since you're using the sourcePoint differently now, but I still think you need to receiver's transform in there somewhere)
The .align() method computes an offset, and then applies that offset to the receiver's frame. This makes the result sensitive to the current frame of the receiver, rather than being an absolute computation. In certain circumstances, this can result in layout that "jitters" across a rounding threshold on each layout pass, instead of always behaving the same way. For example:
Fix for #145