linebender / druid

A data-first Rust-native UI design toolkit.
https://linebender.org/druid/
Apache License 2.0
9.54k stars 569 forks source link

Notifications do not propagate to an AppDelegate event handler #1879

Open tones111 opened 3 years ago

tones111 commented 3 years ago

I'm just starting to experiment with druid and ran into some confusion when trying to handle notifications within an AppDelegate event function. It appears the top-level window eats it after printing out a couple INFO logs.

After running into this issue I came across #1388 that lists a use case of implementing list selection via a notification. I'm attempting to have a Label widget from my list notify my AppDelegate that it's been selected. The delegate will then send commands to the label to change it's background color (and tell any previously selected label to unset its background). Eventually there will be more orchestration commands to different sections of UI.

The AppDelegate seems like a good place to put this orchestration logic. Is this lack of propagation intentional or an oversight?

Thanks for all the effort going into druid. I really like the composability of the various widgets and it feels well designed! Looking forward to watching it mature.

lisael commented 3 years ago

From my understanding, an AppDelegate is somewhat "off-tree". It exists to give the user a chance to modify an event that pass through it before hitting the App, the real root of the widget tree.

What you need is a ControllerHost<W, C> (a widget that wraps another one and delegates calls to event, lifecycle and update to a user-defined Controller<T, W: Widget<T>>) at the top of your widget hierarchy.

See https://github.com/linebender/druid/blob/master/druid/src/widget/controller.rs

EDIT: that's only if you really have to use a notification. Why not a command ?

A command is queued in the context, and sent at the end of the current event phase (or at the start of the next one if the command is not submitted in event(...). It first hits the AppDelegate and then goes down the tree

A notification will have to crawl all the way up the tree to hit your handler. If no ancestor in between is interested by this command, it's not worth it.

Note that you can mix the two: submit a Selector as notification, a parent widget/controller handles it and resend it as a command for the AppDelegate handle it a the top level.

Rule of thumb: Notifications should not be abused. Their purpose is to communicate to a parent without knowing its widget_id. As soon as you know the target at the call site and are able to route a command to it, you should use a command. Here, the target is the AppDelegate, routing is as easy as MY_SELECTOR.to(Target::Global) -> use a command

Note that the explicit routing is not even required as Command implements a sensible From<Selector>. submit_command(MY_SELECTOR) works in most contexts

tones111 commented 3 years ago

Thanks for the insight. I think I had the impression commands were primarily meant for going down the tree while notifications go up. As you mentioned, since the command is targeted it makes sense that there's much less overhead to deliver. Definitely some good ideas to work with here. Thanks!

lisael commented 3 years ago

Technically speaking, commands still crawl all tree from top to bottom, as internal events. It's the wrapping WidgetPods that do forward them to their respective inner widget as plain Event::Commands if the inner widget is the target (see WidgetPod::event(...) for details). Preferring commands over notifications when applicable is more a matter of code cleanness that performance.

There probably is a performance impact, though as in case of a notification the event() method on inner widget is called whatsoever.

lisael commented 3 years ago

Also. Please take all I say with a grain of salt, I'm quite new to Druid, and there are probably some subtleties I don't know and I may have wrong assumptions about the internals. I think there is a (well known) lack of documentation, but the code of the internals is clear and easy to follow IMHO. I head there when I feel stuck, it helps a lot.

cmyr commented 3 years ago

This is correct, the AppDelegate is not part of the tree. To be honest, I hadn't even thought about the AppDelegate when I implemented notifications, and it isn't unreasonable that they could be passed there, but it really wasn't the design goal: notifications are for communicating state between a child and parent widget, and a command is a better fit if you want to get something to your AppDelegate.

lisael commented 3 years ago

On passing notifications to the AppDelegate, It wouldn't hurt, but the use-case is not clear.

Either the use wants to target the AppDelegate and a command is better, or the user is not sure that a parent will catch the notification and want to react to this event. The latter seems to denote a faulty design.

We should wait to have a convincing use case before we implement this, rather than supporting poor design upfront, IMHO.

Edit: In the catch-all use case, the user is not stuck without support in AppDelegate. They can add a controller at the top of the hierarchy. "Make the easy things easy, and the hard things possible".