Add a proof-of-concept ViewSwitcher widget

[s3thi]

I'm looking for some feedback and help with this code. I plan to use this in an app I'm building, but it might be valuable to Druid as a whole.

Short Summary

This PR contains a ViewSwitcher widget that can display one out of any number of child widgets, in the same way that Either can display one out of two widgets. I want to use this as a foundation for a sidebar and a tab-bar widget.

Longer Description

I've been working on a little app where I need a sidebar similar to the one in this screenshot:

Clicking on an item in the sidebar switches the right side of the window to a different view. The items on the left need to be dynamically generated from a field in the application state, and updated whenever it changes. The view on the right can be anything that implements Widget<T>. Each item on the left can map to an entirely different type of widget on the right.

As a preliminary step, I've built a widget that I'm calling ViewSwitcher. The code is inspired by the List and Either widgets. The argument to ViewSwitcher is a closure that is passed some data from a lens, and returns one of many different views based on that data. Something like this:

ViewSwitcher::new(|data: &(u32, AppState), _env| {
        match data.0 {
            0 => Box::new(Label::new("Simple Label").center()),
            1 => Box::new(Button::new(
                "Another Simple Button",
                |_event, _data, _env| {
                    println!("Simple button clicked!");
                },
            )),
            2 => Box::new(Button::new("Simple Button", |_event, _data, _env| {
                println!("Simple button clicked!");
            })),
            _ => Box::new(Label::new("Unknown").center()),
        }
    })

The closure is called every time the application data changes, and the view that it returns is the one that gets rendered on screen.

The ViewSwitcher needs to be wrapped in a LensWrap that gives it access to a tuple of data. The first item in the tuple needs to be the field that will be used to figure out which widget should be returned from the closure. The second item in the tuple can be the full application state or part of the application state. I've added a Tuple lens to the codebase to make this convenient.

With the Tuple lens available, you can write something like this:

ViewSwitcher::new(|data: &(u32, AppState), _env| {
        match data.0 {
            // ... return something useful
        }
    })
    .lens(Tuple::new(AppState::current_view, Id));

I'm not happy with this API at all, but I couldn't figure out anything better. I wanted the children of ViewSwitcher to have the ability to lens down to whichever bit of data they wanted from the entire application state. At the same time, I wanted the closure to be unaware of the structure of the app state and have easy access to a single field it could simply match on.

With some help, I can work on this for the next week or so and get this to a state where it can be merged into Druid. If this design completely misses the mark, then I'm hoping I can learn from the feedback and come up with something better.

When I've figured out how to build this widget, I want to start working on a Sidebar. I feel I should be able to cobble it together with a Split, List, Scroll and ViewSwitcher. At that point, someone might be able to use the same design to build a TabView.

Broken/Missing Pieces

• The child view is created from scratch every time the application data changes. I need to cache all the different children somewhere so I can reuse them and their internal state doesn't get lost.
• Keyboard events don't work in the TextBox widget. Not sure why this happens or how to fix it.
• I see tons of warnings on my console saying WARN [druid::core] old_data missing in WidgetId(XX), skipping update (where XX is a bunch of different numbers). Again, I can't figure out why this happens and what the consequences are.
• I'm not sure using the Tuple lens here makes for a great API. Would love some feedback on how to make this better.

[s3thi]

The PR contains an example of the widget in action. Run:

cargo run --example view_switcher

And click on one of the buttons to see it in action.

[futurepaul]

Looping in the tab view issue because I would imagine this should end up with a somewhat similar design.

I'm curious if it would be possible to make the API more like RadioGroup, which takes a Vec of tuples.

Something like:

            ViewSwitcher::new(vec![
                (WidgetTreeA, View::A),
                (WidgetTreeB, View::B),
                (WidgetTreeC, View::C),
            ]).lens(AppState::view),

Then you can have an enum in the AppState that represents the "current" view, and ViewSwitcher draws that view and hides the others. I don't know if this makes things harder or easier to implement, especially with the global state stuff you're trying to do.

[s3thi]

@futurepaul that's interesting. I'll try that design out in a different branch to see how well it can work. One issue I can think of is that this design would make it hard to dynamically add or remove views from the switcher.

I actually started out with a similar API a couple weeks ago. My plan was to use Commands for adding/removing widgets from the ViewSwitcher, but that didn't feel very natural or Druidic (is that a word yet?). We also didn't have widget IDs at the time, which made things a bit harder.

[cmyr]

Okay, I've had a chance to think about this and I think that the overall idea is good, but that maybe we can simplify it. As your example illustrates, there are essentially two parts to this problem: deciding which widget we want to show, and construction that widget as required, and I think we can separate these problems. I think that the lens might be a bit fancier than is necessary for us, though, and I might prefer just giving the widget two separate closures that influence its behaviour:

struct ViewSwitcher<T: Data> {
    active: WidgetPod<T, Box<dyn Widget<T>>>,
    active_idx: usize,
    child_selector: Box<dyn Fn(&T, &Env) -> usize>,
    child_builder: Box<dyn Fn(usize) -> Box<dyn Widget<T>>>,
}

impl<T: Data> Widget<T> for ViewSwitcher<T> {
    fn update(&mut self, ctx: &mut UpdateCtx old_data: &T, data: &T, env: &Env) {
        let child_idx = (self.child_selector)(data, env);
        if child_idx != self.active_idx {
            self.active = WidgetPod::new(self.child_builder(child_idx));
            self.active_idx = child_idx;
            // you need to do this if you add or remove a child, or things break
            ctx.children_changed();
        }
        if !old_data.same(data) {
            ctx.invalidate();
        }
    }   
}

and if we wanted to get fancy, we could use a generic 'index' type, like:

struct ViewSwitcher<T: Data, U> {
    active: WidgetPod<T, Box<dyn Widget<T>>>,
    active_idx: U,
    child_selector: Box<dyn Fn(&T, &Env) -> U>,
    child_builder: Box<dyn Fn(U) -> Box<dyn Widget<T>>>,
}

So that you could return an enum instead of a usize and maybe have some fancier logic.

Overall, though, I think what I'm sketching out is conceptually quite similar to what you're working towards.

broken/missing

• child views created from scratch: This isn't necessarily bad, although it probably depends. We don't currently have a good story for widgets that leave and then reenter the widget tree: what events should they receive? should they be addressable while they are inactive?
• keyboard events this is likely because you're missing ctx.children_changed(); this is important because events will not be correctly routed to new widgets otherwise. This is likely also why you're getting the old_data missing messages.

Let me know if that is clarifying or not, and forgive me if I've missed anything.

[s3thi]

Thanks very much for the review @cmyr! The API you suggested definitely seems cleaner. I've pushed a bunch of commits implementing those changes, and added some documentation as well. Would love for you to have a look.

If the code looks okay, would it make sense to have this be a part of Druid?

Regarding creating the children from scratch every time: the issue with doing this is that some widgets have internal state that will get reset when they're recreated. If you run the ViewSwitcher example now, you'll find that one of the views is a Split. If you resize the split pane, switch to a different view, and come back to the split, you'll find that its size has been reset to the original size. This is why it might be a good idea to reuse widgets.

Hoping to hear your thoughts on this.

[cmyr]

Cool, it looks good to me. Happy to bring this into druid; at some point we will have to think more carefully about what widgets it makes sense to ship with the framework, but right now I'm happy to err on the side of more is better?

Caching idea makes sense. The main open question, for me, is how we want to manage widgets leaving and reentering the tree; the thing that makes the most sense to me is that we have some method to take the widget out of a widgetpod (like WidgetPod::into_inner) and then when we add the widget back to the tree, we put it in a new widgetpod and it essentially believes that it is brand new, but it happens to have some custom internal state. This imposes an interesting constraint on widgets, which is that all permutations of inner state are possible starting values.

In anycase, in this world your cache is just HashMap<U, Box<dyn Widget<T>>>, and that should be pretty easy.

[cmyr]

2020 ;)

[cmyr]

minor rust idiom thing: it's considered good practice to have the generics in a type declaration be as general as possible, and then to impose restrictions in the impl block. We do need T: Data, because we use a WidgetPod and it requires T: Data, but we don't need to declare PartialEq here; we can have that only appear in the impl.

[s3thi]

I didn't even know this was possible in Rust. Under what conditions would you want to have a type with super loose generics, but then restrict them in the impl? Is there something I can Google for an explanation?

[cmyr]

I had a lot of trouble finding a good reference for this, but the basic idea is talked around here. The idea is a clear separation of data and behavior; your types are supposed to describe data, without concern for behavior, and then then the behavior is provided in impl blocks that can have different bounds, which should (generally) be only as restrictive as required.

(looking into this has made me realize that our WidgetPod type violates this rule, and I'm going to patch that up shortly)

[cmyr]

Would it make sense to have a "default" widget? this would make it easier to handle the case where we don't know how to handle the result of child_selector, as well as letting this be non-optional.

[s3thi]

I don't know if it's worth forcing a user to specify a default widget explicitly, since that case can be handled inside child_builder anyway. I'm working with the assumption that in most applications, the value returned by child_picker will always -- or almost always -- correspond to a valid widget inside child_builder.

If we explicitly force the user to specify a default widget, and in their application there's never really a case when child_picker returns something that child_builder cannot understand, then allocating that extra default widget might be a waste. Plus, it's extra cognitive overhead to think of a reasonable default widget when you're absolutely certain that every value returned by child_picker can be understood by child_builder.

I hope that made sense.

[cmyr]

sounds reasonable. :)

[cmyr]

'is expected' is a bit vague; it's not an option. Maybe just,

"The child_builder function creates the appropriate widget for the passed value."

[cmyr]

I appreciate this was my doing, but I would actually prefer a name other than 'selector'; that term is slightly overloaded, because we have a Selector type that is unrelated to this.

so maybe child_chooser or child_picker or widget_picker or which_widget?

[cmyr]

you could write this as, if Some(&child_id) != self.active_child_id.as_ref() { ... }, if you wanted? just an observation, no big deal either way.

[cmyr]

Another way of writing this is, if let Some(child) = self.active_child.as_mut() {. (either works, just wanted to point out the option)

[cmyr]

Because you're implementing a container widget (a widget that manages one or more other widgets, using a WidgetPod) you have certain special considerations, here.

Most importantly, you will actually receive WidgetAdded any time one of your children change; the message gets sent to each node in the tree that is an ancestor of whatever widget was actually added; so you should be defensive, and only do this initial setup if we don't have a child, or if we otherwise know that we need to do this setup.

There's another interesting thing happening here: by design, what you should be doing is calling ctx.children_changed() after adding your child, and not forwarding the current event to the new child; this will give the framework the chance to send the WidgetAdded event, which should always be the first event any new widget receives; however since we're doing this while handling WidgetAdded already, this should work as written: it's basically like the child was already here, and it will still receive WidgetAdded first and should handle registration etc correctly.

So I think this bit is fine, although in general you should not forward events to newly created widgets, and this definitely needs some more documentation.

[cmyr]

As a heads-up, when #513 lands, this will need to be fixed to correctly propagate the child's paint bounds. I would consider adding a FIXME or something here as a reminder.

[s3thi]

Thanks for the detailed review, @cmyr. I've fixed most of what you pointed out, and left comments where I don't understand something.

In general, I feel we need more documentation around creating new widgets as well as around creating container widgets (Paul Miller's blog post covers the former, but not the latter). In the early life of Druid, these might be the two most common things people want to do. I'll put these things on my list of docs I want to write someday, along with documentation for lenses.

[s3thi]

Now that I've spent some time building a "real" UI with this widget, I'm wondering why I'm separating the child_picker and child_builder at all.

My initial feeling was that the child_picker could take a complex value from the application state and transform it into something that child_builder could easily understand. This would ensure that the child_builder doesn't have to worry too much about the shape of the application state.

But now that I've spent a bit of time with this API, I'm finding that (a) my child_picker is mostly just returning a single field from the application state and (b) child_builder has to know a bit about the shape of the application state anyway, because lenses.

Would it be a better API if we simplified this further and only depended on the child_builder closure?

[cmyr]

I guess my thinking was that we don't want to be running a potentially expensive child_builder function unless it's necessary, and we also don't know exactly how the user is determining when to switch based on the data, and this mechanism feels like it can cover the general cases.

So this is mostly about reducing work; unless I'm missing something, with only child_builder we would have to run it on each update?

re: docs, please feel welcome to open issues describing anything you think is currently under documented. It's hard for me to have a good sense of these things, because I'm very familiar with the internals, so it's very helpful to have outside perspective.

[s3thi]

I was thinking that the child_builder could return an Option<Box<dyn Widget<T>>> instead of just a Box<dyn Widget<T>>. The user could return a None when they don't want the view to change, and a new Option<...> when they do.

Of course, this would make the API way more complex/confusing. The API we have now should be sufficient until we've exercised it a bit.

What should my next step be? Do we need more tests and docs before we can merge this?

Once we have the view switcher, someone could take a stab at building a tab view with this. I'm already building a sidebar widget on top of this code, but I'm not sure it's general enough for Druid. Once I have something working, I'll link it from the Zulip for discussion and review.

[cmyr]

The reason I think that returning Option<Widget> is a problem because it requires your closure to know what the current widget is?

I think that once we agree about the API this is ready to merge, just want to make sure we're making the best choice we can.

[s3thi]

I feel what we have now is the simplest from the PoV of a Druid user, until we discover new use cases.

[cmyr]

Okay two little teeny things but then I'm happy to merge this.