xilem icon indicating copy to clipboard operation
xilem copied to clipboard

Design new `Widget` trait

Open raphlinus opened this issue 2 years ago • 2 comments

An important step in building out the widget tree is to get the trait right. Further iteration is possible, but getting it basically right at this time will save hassle later.

The high-level approach is to start with the Druid Widget trait and make some changes.

  • The data argument goes away (as does being generic on T). This is a natural consequence of the Xilem architecture.

  • The env argument goes away, I think. This is actually a much bigger question, as we rely on env to retrieve style data, so probably deserves some thought about what will replace that.

  • The paint method evolves to generate a piet-gpu scene fragment rather than drawing into a Piet render context.

  • The trait also needs to update the accessibility tree. In the prototype Druid integration for AccessKit, there an accessibility() method that generates the node, but that's based on generating the entire tree every time rather than doing incremental update. This needs to be worked out in detail.

As discussed in today's office hours, we will not be making significant changes to layout at this time, so those methods will basically be the same as Druid's existing Flutter-like mechanism.

The update method

Right now the update method isn't doing much, and if #6 gets adopted, it will do even less. In current Druid, one of its other roles is to respond to env changes (if theme or styling changes), but that may well go away as well.

A potential role may be to update the accessibility tree, but likely that should be a separate method, as it should be lazy (only called when a screen reader is connected).

So one of the questions which needs to be decided is whether to keep the update method or remove it. An excellent way to help resolve it is to go through the existing use cases in Druid and see if any really motivate keeping it.

The lifecycle method

We can also consider combining lifecycle and event, as the motivation for having them separate may no longer be compelling. I consider this a lower priority, as I'm sure it's fine either way, but again if we do decide to change this from Druid, it would be less work to do it now than later.

raphlinus avatar Nov 23 '22 21:11 raphlinus

One other thing to discuss now: damage regions. Because of limitations in WebGPU, it is difficult to plumb that down to the compositor, but I also know from experience that if we don't enforce it, the logic will bit-rot. My current thinking is to render the whole scene, but use the damage region to blit only the changed parts to the final render texture for presentation. (see this Zulip thread for lots more detail about that blit)

There is one fairly subtle consideration about damage regions that is new. If a container culls some of its children because they don't intersect the damage region, then the resulting scene fragment is not valid with a different damage region. I see two approaches:

  • The simplest thing to do is always append children regardless of the damage region, so the resulting scene is valid for the whole viewport. By definition, the children outside the damage region won't be invalidated, so their fragments are retained. Under the assumption that append is cheap (and early stages of coarse rasterization), this makes sense, and avoids the risk of over-invalidating because of changes of damage region.
  • Assuming we want to trim the scene in cases of small updates, probably the best approach is for containers to set a flag on PaintCx indicating that their fragment is only valid for the given damage region. Then in a subsequent paint cycle, if the new damage region covers any area not covered by the old one, the container's fragment is invalidated and paint is called on it.

I'm honestly not sure which of these two approaches is best. The second is more complex and puts a greater burden on widget implementations, but may be more efficient.

raphlinus avatar Nov 25 '22 18:11 raphlinus

Should we maybe open an issue for long-term exploration of layout mechanisms? So that when the time is ripe, we can try to improve upon the current Flutter style.

ghost avatar Nov 27 '22 10:11 ghost

I'm in favor of a somewhat more focused issue that specifically queries what layout ideas we can import from SwiftUI (including the fragment implemented in the previous xilem prototype). To my mind, that's primarily GeometryReader and named alignments.

Another question I'm running into is how (and whether) to adapt the prepare_paint method from the xilem prototype. That was primarily for the virtualized scrolling experiment (it would be called on a scrolling list view when the scroll offset changed, and could request widgets to be materialized), but I can also imagine it being used for damage region invalidation.

In Druid, request_paint() is mostly called from the update() method when the data changes (or from event and lifecycle when the widget appearance needs to change in response to those). Most widgets just invalidate their entire paint_rect, but the possibility exists for a widget to do finer grained invalidation. To preserve the Druid mechanisms, we would need to keep update more or less as-is so that there would be an update context in which to request invalidation. Having update be the authoritative source of truth might involve reverting #6, where some of that responsibility is moved upstream into the view.

Another possible direction is to have invalidation move into a new prepare_paint method, which might be compatible with update going away. I need to think about this more.

My inclination for now is to preserve Druid mechanisms as much as possible, and propose changes later. However, now is an opportunity to rethink things, which I don't want to entirely pass up.

raphlinus avatar Dec 05 '22 23:12 raphlinus

I had some ideas about the Widget trait. Maybe some of them are useful.

I have no idea whether Flutter or SwitfUI style layout is the better fit for Xilem. But either way, i am in favor of a preferred_size() method in the Widget trait. Without it, i often had to hardcode sizes for widgets to make them usable.

I also think we should combine lifecycle and event. The only remaining reason for keeping them apart, is that event has capabilities which lifecycle hasn't, like set_handled() and set_active(). Maybe there could be an even more fine grained solution, like one context per event type? Something like this:

fn event(&mut self, cx: &mut EventCx) { //the event is part of the cx
    match cx.event() {
        RawEvent::MouseDown(mouse_event: &MouseEvent, cx: &mut MouseCx) => { // the MouseCx contains all methods of EventCx + some mouse specific ones
            cx.set_active(true);
            cx.set_handled();
        }
        RawEvent::MouseUp(mouse_event: &MouseEvent, cx: &mut MouseCx) => {
            if cx.is_active() && cx.is_hot() {
                cx.set_active(false);
                // do something
            }
        }
        RawEvent::BuildFocusChain(cx: &mut FocusChainCx) => {
            cx.register_for_focus();
        }
    }

    cx.set_active(true); // this would be a compiler error
    let active = cx.is_active(); // while this would be ok
}

In this case you could merge lifecycle, update and prepare_paint all into event.

xarvic avatar Dec 06 '22 16:12 xarvic

Hmm, I'm realizing this is a bit deeper water than I originally planned. Let me gather my thoughts a bit.

First, I do think event and lifecycle can probably be combined. I know we had a rationale for separating them, but suspect that is not quite as relevant now that the Widget trait isn't meant to support any application architecture.

Second, I'd like to put tweaks to the layout protocol aside. I do think there is scope for improvement, but for the most part it feels separate. Flutter has a bunch of extra mechanisms (implicit size etc) on top of the core BoxConstraints mechanism, and we can sort through how much of that we want to copy verbatim, and how much we want to change.

Third, I'm skeptical that prepare_paint should be unified with event, though it's not completely out of the question. My idea of event is that it generally targets a single widget, while prepare_paint would be called on pretty much any widget that has requested paint (in response to an update, which is basically equivalent to a mutation by the view logic, or in response to an event), and also when it becomes visible, definitely when scrolling but probably not exclusive to that.

My inclination here is to do a first cut that stays very close to the original Druid code, then consider changes from that. The closer we stay to the original trait, the less cognitive load there is to adapt widgets. That said, this is also an opportunity to make improvements without having to make big refactoring patches.

raphlinus avatar Dec 06 '22 22:12 raphlinus

As discussed in office hours, in terms of the events, I think separating it out when there are separate logic paths (like broadcast vs to one widget vs commands) would probably be easy to port, and would be conceptually easier when creating a new widget. New people, when creating widgets that contain widgets, may not realize the different possibilities and just propagate them blindly otherwise.

And in terms of the other functions, I think it's most important to make it efficient in the case of large scroll lists, where the behavior of hidden widgets matters, but must be cheap.

In my use case, a chat app, I need to process a lot of text fields in a scroll area, which is very computationally expensive when it comes to layout and sometimes painting.

jaredoconnell avatar Dec 07 '22 17:12 jaredoconnell

My idea of event is that it generally targets a single widget.

This is true for druid's event method, but if we combine event and lifecycle, this no longer holds. Many lifecycle events target multiple widgets. In my opinion the thing that unifies event and lifecycle is, that there is no need for interaction between child and parent to propagate the events correctly and therefore we can send them all in one method.

I think it would be ideal to have methods for everything, every widget has to do and put the rest into event. Every widget has to do layout, paint it self, generate accessibility information and depending on the model we choose maybe call ctx.request_paint() in update or prepare_paint.

As discussed in office hours, in terms of the events, I think separating it out when there are separate logic paths (like broadcast vs to one widget vs commands) would probably be easy to port, and would be conceptually easier when creating a new widget. New people, when creating widgets that contain widgets, may not realize the different possibilities and just propagate them blindly otherwise.

@jaredoconnell, could you explain the benefit of having seperate methods? I unfortunately did not have time to come to the office hours. So i hope i don't sound ignorant, but my understanding is, that this is actually the thing which made druid so elegant. You can just send your events to your children and their WidgetPod will do the right thing.

xarvic avatar Dec 10 '22 11:12 xarvic

Closing this as the immediate issue is solved by #26. There will be another issue soon for followup.

raphlinus avatar Jan 21 '23 20:01 raphlinus