Implement overlay for Component

[nicksenger]

I saw the note about this method in Component so figure perhaps something like this has already been considered (or there are other plans for overlay), but here is something I came up with that seems to work in the limited tests I've performed.

The idea is to use a second self-referential struct to cache the cached element's overlay content, then return an overlay element that accesses the cache for each operation similar to how it's being done for the original element.

There's a couple potential problems with doing this:

• In each widget method for the original content we must call into_heads to access the element. I don't know what the performance implications of this are.
• Since we need to drop the cached overlay to access the original element, the returned overlay element may try to access the cached overlay content after it has already been dropped. I've tried to do the right thing in those cases, but I'm not sure what impact this could have on the UI in practice.

[hecrj]

I saw the note about this method in Component so figure perhaps something like this has already been considered

Yes! I originally tried to implement another self-referential Overlay struct:

#[self_referencing]
struct Overlay<'a, 'b, Message, Renderer, Event> {
    state: &'b mut Option<State<'a, Message, Renderer, Event>>,

    #[borrows(mut state)]
    #[covariant]
    overlay: Option<overlay::Element<'this, Event, Renderer>>,

    message: PhantomData<Message>,
}

So we can implement Widget::overlay as follows:

fn overlay(
    &mut self,
    layout: Layout<'_>,
) -> Option<overlay::Element<'_, Message, Renderer>> {
    let overlay = OverlayBuilder {
        state: &mut self.state,
        overlay_builder: |state| {
            state
                .as_mut()
                .unwrap()
                .with_element_mut(|element| element.overlay(layout))
        },
        message: PhantomData,
    }
    .build();

    if overlay.borrow_overlay().is_some() {
        Some(overlay::Element::new(layout.position(), Box::new(overlay)))
    } else {
        None
    }
}

But unfortunately, I don't think ouroboros can handle two lifetimes in a head field.

I did not think about relying on RefCell for internal mutations... That's pretty nifty! Let me see if I can simplify the code a bit...

[hecrj]

I have removed some of the duplication in 48b2264 by creating a couple of helper methods.

Hopefully I didn't break anything! Let me know what you think.

[nicksenger]

I have removed some of the duplication in 48b2264 by creating a couple of helper methods.

Hopefully I didn't break anything! Let me know what you think.

I really like the with_* helper methods, they make this much easier to follow!

There is an edge case though where if overlay.on_event() causes a state mutation which results in the original element's overlay method now returning None, the application will panic because hash_layout or on_event may still be invoked on Overlay, but there's no longer any cached overlay content for it to access after the cache gets rebuilt.

I made an attempt to fix this by deferring the processing of the overlay's events by component.update(..) until on_event is invoked on the original element in aa09bd4. This avoids rebuilding the cache in Overlay::on_event, so there should no longer be a risk of the overlay vanishing while it's still being operated on. It does change the order of operations a bit for components relative to other widgets in some cases though, so let me know whether or not you think this is a viable workaround.

[hecrj]

It does change the order of operations a bit for components relative to other widgets in some cases though, so let me know whether or not you think this is a viable workaround.

I'm not sure it will work properly. There is no guarantee that Widget::on_event will be called after Overlay::on_event. Therefore, this approach could produce weird behavior (overlays not closing until you move the mouse, etc.).

I think the best approach for now is to wrap the overlay::Element in an Option and simply drop overlay events if it turns out to be closed during the processing of an event batch. This is what you first implemented and I incorrectly removed, right? 😅

[nicksenger]

I'm not sure it will work properly. There is no guarantee that Widget::on_event will be called after Overlay::on_event. Therefore, this approach could produce weird behavior (overlays not closing until you move the mouse, etc.).

That's a good point. It implicitly depends on the order in which methods are being called from UserInterface::update currently, which isn't really part of its contract.

I think the best approach for now is to wrap the overlay::Element in an Option and simply drop overlay events if it turns out to be closed during the processing of an event batch. This is what you first implemented and I incorrectly removed, right? 😅

Yeah this is what I tried to do originally, but I was a bit concerned that returning an empty layout node from Overlay::layout in this case could cause problems. Thinking about it more though, since we won't be drawing anything from the overlay anyways, it shouldn't do any harm right? I've restored this approach in 44c0d75

[hecrj]

Thinking about it more though, since we won't be drawing anything from the overlay anyways, it shouldn't do any harm right?

It shouldn't be a problem. The layout nodes of each widget are opaque to iced and, as such, cannot be a part of any external contract.

[hecrj]

Thank you for this!

It's great to be able to make it work even when the current overlay implementation hinders composability!

Let's ship! 🎉