Skip to content

Make view alignment not sensitive to current frame #146

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+82 −11
Draft

Make view alignment not sensitive to current frame #146

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
93 changes: 82 additions & 11 deletions Paralayout/UIView+Alignment.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,80 @@ extension Alignable {
return sourcePoint.offset(to: targetPoint)
}

/// Calculates the offset between two views' positions, ignoring any transforms in the view hierarchy.
///
/// - precondition: The receiver and `otherView` must be in the same view hierarchy.
///
/// - 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`.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Heh, copy-pasta of the other method's comments. Needs updating

public func untransformedFrameOrigin(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A couple things to consider:

  1. We should probably eliminate the untransformedFrameOffset method entirely -- but that's a breaking change
  2. If not, we ought to DRY up the implementations, of course

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. I'm okay with breaking changes. We've been trending towards a breaking release for a bit now. main already has breaking changes on it – example. One big thing though is the documentation for this method does not make it clear how it differs from untransformedFrameOffset, 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)
  2. heh, yeah. We've been finding that difficult... been resorting to copy/paste recently

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I'm struggling with this function name + parameter names

toAlign position: Position,
to otherView: Alignable,
_ otherPosition: Position,
alignmentBehavior: TargetAlignmentBehavior = .automatic
) throws -> CGPoint {
let receiverContext = alignmentContext
let receiverView = receiverContext.view

let targetContext = otherView.alignmentContext
let targetView = targetContext.view

// We can't be aligned to another view if we don't have a superview.
guard let superview = receiverView.superview else {
ParalayoutAlertForInvalidViewHierarchy()
return .zero
}

switch position {
case .topLeft, .topRight, .leftCenter, .rightCenter, .bottomLeft, .bottomRight:
switch otherPosition {
case .topLeading, .topTrailing, .leadingCenter, .trailingCenter, .bottomLeading, .bottomTrailing:
ParalayoutAlertForMismatchedAlignmentPositionTypes()
default:
break
}

case .topLeading, .topTrailing, .leadingCenter, .trailingCenter, .bottomLeading, .bottomTrailing:
switch otherPosition {
case .topLeft, .topRight, .leftCenter, .rightCenter, .bottomLeft, .bottomRight:
ParalayoutAlertForMismatchedAlignmentPositionTypes()
default:
break
}

default:
break
}

let targetIsInSourceSuperviewChain = sequence(first: receiverView, next: { $0.superview }).contains(targetView)

let targetPoint: CGPoint
switch alignmentBehavior {
case .bounds,
.automatic where targetIsInSourceSuperviewChain:
targetPoint = try superview.untransformedConvert(
targetContext
.pointInBounds(at: otherPosition)
.offset(
by: UIOffset(horizontal: -targetView.bounds.origin.x, vertical: -targetView.bounds.origin.y)
),
from: targetView
)

case .untransformedFrame,
.automatic /* where !targetIsInSourceSuperviewChain */:
Copy link
Collaborator

Choose a reason for hiding this comment

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

boy oh boy do I wish I knew why this where was commented out in the code we copied.

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 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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

targetPoint = try superview.untransformedConvert(
targetContext.pointInBounds(at: otherPosition),
from: targetView
)
}

let sourcePoint = receiverContext.pointInBounds(at: position)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@NickEntin NickEntin Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(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)

return targetPoint.offset(by: .init(horizontal: -sourcePoint.x, vertical: -sourcePoint.y))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You know what I know there. Need to defer to @NickEntin

Copy link
Collaborator

Choose a reason for hiding this comment

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

these last two lines are the only difference between the above method and this one, right?

}

/// Move the view to align it with another view.
///
/// - precondition: The receiver and the `otherView` must be in the same view hierarchy.
Expand All @@ -135,17 +209,14 @@ extension Alignable {
) {
do {
let receiverView = alignmentContext.view
receiverView.untransformedFrame.origin = receiverView.untransformedFrame.origin
.offset(
by: try untransformedFrameOffset(
from: position,
to: otherView,
otherPosition,
alignmentBehavior: alignmentBehavior
)
)
.offset(by: offset)
.roundedToPixel(in: receiverView)
receiverView.untransformedFrame.origin = try untransformedFrameOrigin(
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 are two ways we could add test coverage here:

  1. Add a snapshot test with two identical sibling views, calling .align() once on one of them, and twice on the other
  2. 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.

toAlign: position,
to: otherView,
otherPosition,
alignmentBehavior: alignmentBehavior
)
.offset(by: offset)
.roundedToPixel(in: receiverView)

} catch {
ParalayoutAlertForInvalidViewHierarchy()
Expand Down
Loading