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

Nested overlays #1719

Merged
merged 13 commits into from
Jun 14, 2023
Merged

Nested overlays #1719

merged 13 commits into from
Jun 14, 2023

Conversation

tarkah
Copy link
Member

@tarkah tarkah commented Feb 18, 2023

This implements a new overlay which allows for nesting. It's fairly inefficient as it calls Overlay::overlay within every method, but this reduces complexity as dealing w/ lifetimes here will be really rough.

The nice part is this doesn't touch the UserInterface code at all except to wrap the root overlay into a Nested. Nested is only public to the crate and only meant to be used as the root overlay used by the user interface. This means that the layout caching done in user interface works for the Nested layout as well.

I haven't implemented the nested Overlay::overlay on any of the lazy overlay implementations yet, but I can implement if we feel this is a good approach.

If approved, we can rebase #1692 as that should now work w/ this.

simplescreenrecorder-2023-02-18_13.53.33.mp4

@tarkah
Copy link
Member Author

tarkah commented Feb 18, 2023

I'm getting a layout related panic (unwrap) in the modal example when interacting with the pick_list, but I can't always recreate it.

Edit: Removing all unwrap solved the problem: 4e9b9d7. No idea where the root cause is for why the layout tree was invalid, it seemed to be during on_event that the panic occurred.

@tarkah tarkah force-pushed the feat/nested-overlay branch from e4e6c50 to 01fb0b5 Compare February 18, 2023 22:33
Comment on lines 223 to 234
if matches!(status, event::Status::Ignored) {
element.on_event(
event,
layout,
cursor_position,
renderer,
clipboard,
shell,
)
} else {
status
}
Copy link
Member Author

@tarkah tarkah Feb 19, 2023

Choose a reason for hiding this comment

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

Still need to encode cursor availability here when the overlay a layer higher is_over. I've handled this on draw already.

I tried updating it here, but there was an issue in the modal example because of how pick_list handles mouse clicks on it's overlay menu. It doesn't capture the event when clicked in the overlay, and instead lets the base widget handle "closing" and publishing to shell the selection.

This is an issue because then the nested pick_list menu overlay (higher layer) reports is_over, but doesn't capture the event. So if we set cursor to (-1, -1), the "modal" overlay will think it's clicked outside and close, when we only want the pick_list to close.

I think we need to refactor pick_list to "capture" the mouse click in the overlay menu.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've resolved this with a617f3a and 4d439e4.

You can compare the behavior prior to a617f3a on modal example. If you select the middle picklist option over the text input, the text input focuses after selecting the option.

After a617f3a, the undesired behavior mentioned above occurs (modal closes when clicking outside menu bounds / onto modal).

After 4d439e4, the picklist menu now properly captures the status of the event in the overlay when an option is selected. Everything now works as expected.

@tarkah
Copy link
Member Author

tarkah commented Feb 19, 2023

I think we can just naively store the overlay element of the ouroboros builders tail field for the lazy widgets in a Nested and get nesting there without touching the existing overlay implementation. I don't even think it'll be possible to return an overlay from within those overlay implementations anyways. Maybe hackable for lazy / responsive but definitely not component.

Edit: This seems to have worked like a charm: 5283a47. I tested by wrapping the modal example view in a responsive then lazy and the nested overlay worked perfectly.

I had to pub Nested to access it from lazy crate, but I think this is fine. Maybe we could add a disclaimer that Nested shouldn't be used by consumers directly in most cases? Since Nested doesn't implement Overlay I don't think it's a huge risk.

@hecrj hecrj added this to the 0.9.0 milestone Mar 16, 2023
@hecrj hecrj added feature New feature or request widget layout labels Mar 16, 2023
@hecrj
Copy link
Member

hecrj commented Apr 11, 2023

Just to give an update here: I have plans to tackle proper cursor availability soon, and this is waiting for that.

@hecrj hecrj modified the milestones: 0.9.0, 0.10.0 Apr 12, 2023
@hecrj hecrj force-pushed the feat/nested-overlay branch from 5283a47 to 87db76a Compare June 14, 2023 09:25
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks 🥳 This is a huge win.

I have rebased on top of master and everything seems to be working. Hopefully I didn't break anything!

@hecrj hecrj changed the title Nested Overlay Nested overlays Jun 14, 2023
@tarkah
Copy link
Member Author

tarkah commented Jun 14, 2023

Awesome! Thanks partying_face This is a huge win.

I have rebased on top of master and everything seems to be working. Hopefully I didn't break anything!

Looks good to me and modal example is still working 👍 Thanks!

@hecrj hecrj merged commit 267dbf3 into iced-rs:master Jun 14, 2023
@hecrj hecrj mentioned this pull request Feb 7, 2024
@MichalLebeda
Copy link
Contributor

@tarkah As you said "Nested is only public to the crate and only meant to be used as the root overlay used by the user interface" I wonder why is it used in Responsive?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request layout widget
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants