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

Add zstack view to compose views on top of each other #737

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

viktorstrate
Copy link
Contributor

@viktorstrate viktorstrate commented Nov 10, 2024

Adds a new zstack Xilem view (along with an accompanied Masonry widget).
The view is inspired by the ZStack view in SwiftUI, which composes the child view on top of each other.

This pull-request is still work-in-progress.

  • It doesn't allow for changing the layout when BoxConstraints of the children are different.
    (top-left, top-right, bottom-left, bottom-right, center).
  • I want to update http_cat to put copyright on top of image like seen below.
Screenshot 2024-11-10 at 17 46 45

@viktorstrate viktorstrate marked this pull request as draft November 10, 2024 16:28
@viktorstrate viktorstrate changed the title WIP: Add zstack view to compose views on top of each other Add zstack view to compose views on top of each other Nov 10, 2024
@Philipp-M
Copy link
Contributor

See also #591, which goes a similar direction.

I think to make a zstack really useful we would need transforms (kurbo::Affine) on widgets in general and relevant logic for pointer events etc. See also here. I'm thinking about getting to that topic soon, as I think this is the main blocker for allowing fancy interactive vector graphics with vello and xilem.

I like the general direction, i.e. unifying a potential xilem_svg API using a zstack (instead of separate context such as in the web).

@DJMcNab
Copy link
Member

DJMcNab commented Nov 11, 2024

I don't think that arbitrary transforms should block this landing, though. I don't really have the bandwidth to look at this too closely at the moment, unfortunately.

@Philipp-M
Copy link
Contributor

To clarify, I didn't mean this to be a requirement, it's orthogonal to this change (and a lot more work...). I'll take a look at the PR when it's out of draft. On a skim it seems reasonable so far.

@viktorstrate viktorstrate force-pushed the zstack branch 2 times, most recently from 0730a16 to 7c21129 Compare November 16, 2024 21:53
@viktorstrate viktorstrate marked this pull request as ready for review November 16, 2024 21:56
@viktorstrate
Copy link
Contributor Author

viktorstrate commented Nov 16, 2024

The PR is now ready for review!

I still need to add comments to document the new stuff.

I have added the copyright on top of the image in the http_cats example. I'm concerned about the visibility on light images. Is there a way to add a border to the text? I would like to get suggestions on what to do in this case?

Screenshot 2024-11-16 at 22 56 38

@DJMcNab
Copy link
Member

DJMcNab commented Nov 18, 2024

Maybe adding a sized_box with a semi-transparent black background?

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

This looks like a good foundation, and it should be a useful view. Some review thoughts from a first pass.

One thing which does concern me is event handling; I suspect that the behaviour will be hard to reason about. Ideally, we'd warn if more than one layer is interactive, but we don't currently have a mechanism to do that.

xilem/examples/http_cats.rs Outdated Show resolved Hide resolved
xilem/src/view/zstack.rs Outdated Show resolved Hide resolved
masonry/src/widget/zstack.rs Show resolved Hide resolved
masonry/src/widget/zstack.rs Outdated Show resolved Hide resolved
masonry/src/widget/zstack.rs Outdated Show resolved Hide resolved
masonry/src/widget/zstack.rs Outdated Show resolved Hide resolved
masonry/src/widget/zstack.rs Show resolved Hide resolved
masonry/src/widget/zstack.rs Outdated Show resolved Hide resolved
xilem/src/view/zstack.rs Outdated Show resolved Hide resolved
}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum HorizontalAlignment {
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to check whether we can merge or reuse a few of these enums for different container widgets. (no action required here though, I think that should be done in follow-up PRs)

Copy link
Member

Choose a reason for hiding this comment

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

Lol, I just made this same comment (and then deleted when I saw this)

masonry/src/widget/zstack.rs Outdated Show resolved Hide resolved
masonry/src/widget/zstack.rs Outdated Show resolved Hide resolved
@Philipp-M
Copy link
Contributor

One thing which does concern me is event handling;

Can you expand on that a little bit?

@DJMcNab
Copy link
Member

DJMcNab commented Nov 18, 2024

Can you expand on that a little bit?

Sure. If you have a child widget of the zstack which accepts events in only a small region, and wants to pass through the rest (for example, a circular button), I don't think that will work because of how Masonry works, that events can only bubble. I haven't thought too deeply about it, and it doesn't need to be addressed in this PR.

@viktorstrate
Copy link
Contributor Author

One thing which does concern me is event handling; I suspect that the behaviour will be hard to reason about.

Would it somehow be possible to pass the event to the front most child of which the cursor is within the bounding box?

@Philipp-M
Copy link
Contributor

Hmm, I think we need a more significant rework of the pointer-intersection logic in masonry anyways, i.e. allow the widgets to customize the pointer intersection behavior (as right now only the bounds are checked). Possibly with a cheaper hierarchical bounds check (BVH) before.

I have similar issues in #753.

I'm thinking about tackling this, as this is relevant for kurbo shapes with pointer event listeners as well.

Anyway, I think this PR (and the linked) just shows issues with the mentioned pointer/widget-intersection logic, which I think is orthogonal and should probably be handled in another PR....

@viktorstrate
Copy link
Contributor Author

I fixed a lot of the comments you all made.

After rebasing the PR the copyright symbol in the http_cats example does not render anymore, I tried running the example on the main branch as well and it's the same.

I made a black semi-transparent background for the copyright text, to make it easier to read.
Thanks to @DJMcNab for the suggestion.

Screenshot 2024-11-22 at 21 05 38

There are still some open comments that are unresolved and that I'd like your input on to continue.

@Philipp-M
Copy link
Contributor

Sure. If you have a child widget of the zstack which accepts events in only a small region, and wants to pass through the rest (for example, a circular button), I don't think that will work because of how Masonry works, that events can only bubble. I haven't thought too deeply about it, and it doesn't need to be addressed in this PR.

FWIW I think this should be fixed with #753, as well as supporting custom pointer intersection shapes (like the circular button), when I understand you correctly.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Handful of small things, but otherwise this looks good.

masonry/src/widget/zstack.rs Show resolved Hide resolved
masonry/src/widget/zstack.rs Outdated Show resolved Hide resolved
xilem/src/view/zstack.rs Show resolved Hide resolved
xilem/examples/http_cats.rs Outdated Show resolved Hide resolved
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Thanks! There's just a merge conflict to fix

}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum HorizontalAlignment {
Copy link
Member

Choose a reason for hiding this comment

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

Lol, I just made this same comment (and then deleted when I saw this)

Copy link
Contributor

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for all the work!


// MARK: Sequence

/// A trait implementing `ViewSequence` for `ZStackElements`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think using the plural for ZStackElement may suggest that there's a type ZStackElements, so maybe rather ZStackElements or

[`ZStackElement`]s

to directly link to the type.

(same for the other occurences of this here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I fixed it in the latest commit

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.

3 participants