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.

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

Introduce LayoutWriter #187

Merged
merged 6 commits into from
Jan 5, 2021
Merged

Introduce LayoutWriter #187

merged 6 commits into from
Jan 5, 2021

Conversation

kyleve
Copy link
Collaborator

@kyleve kyleve commented Dec 9, 2020

Introduce LayoutWriter, for easier custom layouts without needing to use the Layout protocol. Check out the added swift file for the documentation – but the idea here is to support easier custom arbitrary layouts. I pulled this over from the second https://github.com/squareup/market/pull/549 navigation controller PR.

Copy link
Contributor

@kylebshr kylebshr left a comment

Choose a reason for hiding this comment

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

Pretty neat!

BlueprintUI/Sources/Layout/LayoutWriter.swift Show resolved Hide resolved
BlueprintUI/Sources/Layout/LayoutWriter.swift Outdated Show resolved Hide resolved
BlueprintUI/Sources/Layout/LayoutWriter.swift Outdated Show resolved Hide resolved
switch builder.sizing {
case .unionOfChildren:
return builder.children.reduce(CGRect.zero) { rect, child in
rect.union(child.frame)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there's some odd behavior here if any frames have negative values. The element's size will be large enough to bound everything, but the frames won't be placed inside that space when they're laid out.

I think you might want to do one of these, depending on desired behavior:

  • In layout, offset each frame by the minX/minY of all frames, to normalize the origin. This is probably the expected thing?
  • Here, clamp frames to positive values. This would allow negative frames to lay out outside of the element's bounds, but the size is not really the union anymore (e.g. if all children are negative the size would be zero).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm.... interesting. Let me do a couple examples with negative frames. I kinda feel like if you make a frame negative it should just appear starting at that negative frame outside the bounds (since it is an arbitrary layout), and I thought that's what would happen here, but will check and add screenshots.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

So doing negative frames results in this kinda thing; which lines up with other elements like Row and Column that kinda just let their children lay out outside their bounds – which seems fine? You have to try pretty intentionally to end up in this state, and then the layout does what you ask, so I'm not worried about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you make a frame negative it should just appear starting at that negative frame outside the bounds

That seems fine, but what's the measured size in this case? I think you'd want the measured size to exclude content outside the positive area, or else you'll have extra space to the top and right.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the docs and added an example here.

// MARK: Element

var content: ElementContent {
ElementContent(layout: Layout(builder: self.builder)) { builder in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the environment capture is gone, I think you can hoist this ElementContent up to LayoutWriter itself, and remove the InnerElement entirely!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ohhhhh right, good call

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, hmm, I might still need it; lemme check if the various ElementContent inits do what I need (need the size).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Though I guess if I re-build during measure and layout, that won't matter, which I think happens anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh duh, the one with measureFunction as a param...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh wait that only returns a CGSize (also duh...), so I think I do need the inner element – since I basically need to lazily generate the content, depending on the size passed by the parent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok this works now, thanks for coming to my stream of consciousness.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok actuallllly wait this won't work; since the children of the layout writer might change based on the provided layout size, so therefore we can't build the layout and provide the children with

        ElementContent(layout: Layout(build: self.build)) { builder in
            builder.add(element: ...)
        }

Since the children aren't actually know at that point.

/// └──────┴──────────┘
/// ```
/// The large rect will be the calculated size / bounds of the layout, starting at (0,0). Any rects with
/// negative origins will overhang the layout to the top or left, respectively.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome, thanks for clarifying this.

@kyleve kyleve merged commit a2e6e98 into main Jan 5, 2021
@watt watt deleted the kve/layout-writer branch November 24, 2021 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants