Fix command targeting.

[xStrom]

This PR fixes commands targeted at a specific widget being recursed to children. I also moved the command targeting logic more to one place (WidgetPod::event) as previously there was some target translation going on at the WinHandler level already, which meant that the test harness command sending was broken because the harness sends the commands untranslated.

I also refactored the event recursing logic in WidgetPod::event to be more like the newer WidgetPod::lifecycle. The new approach is more flexible and also more performant because in a lot of cases there's no need to construct a new event.

[luleyleo]

This sure is a nice simplification of the event handling logic as well, thanks!
The way events are filtered is still a bit irritating though.

[luleyleo]

Why is this marked as handled...

[luleyleo]

... but not this?

[luleyleo]

So, I think I got this now:
Only commands targeted the current widget are marked as handled and will thus be ignored by nested WidgetPods while Timer events are filtered out regardless of being handled, as they are always targeted at a single widget.

This distinction is kind of irritating when reading the code, and should be documented if we stick to it.

Also I think it is weird that a widget, receiving a command targeted at it, gets that command already marked as handled. I do understand that this is currently necessary because we loose the information about whether or not the command was targeted, once we convert it to a normal Event::Command, but it still seems wrong to me.

The two possible solutions that come to my mind are either passing down both, the normal command and the internal in case it should propagate further, or adding the Target to Event::Command and always pass it down along the command.

[xStrom]

Initially I tried solving it by passing down the internal command, however that breaks a whole class of command usage. Stuff like ALL_TAKE_FOCUS_AFTER in the test case. A command where a widget wants to decide whether to handle it after it has been passed down to children. This would no longer work if we pass an internal command that generates the regular command for each widget.

Adding Target to Event::Command is also something I considered, but I went with is_handled because that is backwards compatible. I'm not commited to it though, and I'll write an extra comment with the pros and cons of doing either.

[luleyleo]

Not related to your PR but made me wonder:
Does it make sense to allow global commands to be 'handled'?
It seems wrong that global commands, which IMO are supposed to reach everywhere, can be cough by some widget, in a not necessarily deterministic way (as it depends on the window order).

[xStrom]

This touches upon the whole handled concept. A similar problem applies to all the other events as well.

Setting handled is never that deterministic, global or not. I could also see it working on a per-window basis for global commands. Either way it would probably be good to document that in the docs of Target.

[xStrom]

The problem with targeted commands is that:

• To support a whole class of commands like ALL_TAKE_FOCUS_AFTER global/window commands need to be translated from internal commands to regular commands at the root of hierarchy. Otherwise widgets can't know what their children will do in response to a command. At best you can flip the order of sending the internal and synthesized command and break ALL_TAKE_FOCUS_BEFORE instead.
• Widget targeted commands should only get sent to their target widgets. This allows widget code to just handle the command and also share the command functionality. That is to say, a widget that receives a BACKGROUND_RED command can just paint their background red and that will be the correct action regardless of how it was targeted. If widget targeted commands also go to other widgets then we would need two different commands BACKGROUND_RED_TARGETED and BACKGROUND_RED where in the case of the targeted variant the widget would have to also manually ctx.set_handled() to prevent it being propagated further.
• WidgetPod::event can't stop the propagation of a targeted Event::Command because that enum variant no longer contains the target. It can't tell a widget targeted command apart from a global one.

The easiest to program solution, which is also backwards compatible compilation wise, is the one this PR has right now. When synthesizing the Event::Command of a widget targeted command, we also set it as handled already. This prevents it from going further than its actual target widget.

The problem with this approach is that it is a rather unique solution and primarily a result of wanting to keep backwards compatibility. The uniqueness means that it can be confusing - the propagation prevention doesn't look like any other event. @Finnerale already got confused by this and he has been working on commands. It's very likely others will be even more confused by it.

An alternative solution would be to change Event::Command(cmd) to Event::Command(target, cmd). This would allow WidgetPod::event to distinguish a widget targeted command from others and stop propagation based on that, leaving handled out of it.

Another positive effect of adding target is that the widgets themselves will have more information. When they receive that BACKGROUND_RED command they will know if it's specifically targeted at them or not. If it's just them, they might also paint a golden border or something. That is - some widgets can have special functionality based on target info, while most widgets don't need to care about the target and also don't need to deal with multiple different BACKGROUND_RED commands.

The downside of adding the target is that it's another breaking change. However perhaps we shouldn't be too concerned about that at this stage and focus more on getting things right.

When doing this PR originally I was on the fence about which way to go. I went with handled because it was easier to do and less of a waste when redone. Now that I've thought about this a bit more and seen the confusion it caused, I'm leaning towards changing Event::Command to also contain the target.

@cmyr you've used commands a bunch, do you think adding target to Event::Command is reasonable?

[cmyr]

Okay sorry to leave this hanging.

I think that this problem, in combination with some of the issues raised in the @Finnerale's work with selectors, suggests (to me) that command should include a target.

I think this means that Command itself gets a target, although it's possible that this goes in Event::Command. This will require some exploration.

I don't think I totally understand the ALL_TAKE_FOCUS_BEFORE/ALL_TAKE_FOCUS_AFTER example; if you wanted to walk me through the goals and concerns there I might be able to provide better input.

If we add a target to Event::Command, do we get rid of InternalEvent::TargetedCommand? Would it be an option to always send the original targeted command along, after sending the Command (in the current system?)

[xStrom]

The main point of ALL_TAKE_FOCUS_BEFORE / ALL_TAKE_FOCUS_AFTER is that it allows predictable ordering and overriding of behavior. All widgets will handle this event - they will request focus. With the before variant the widgets request focus before they send the even down to their children, and with the after variant they request focus after they've passed the event down to their children. This means that with the before command the deepest child makes the last focus request and wins, and with the after command it's the root parent.

This is all very nice and it works right now. The point I was making is that this only works if this is a regular Event::Command (as opposed to internal) at the root already and then widgets keep passing this event down.

This is noteworthy because the propagation could also be solved by having an internal event go down which then WidgetPod will synthesize the regular command at each step. Regular commands would never be propagated down by WidgetPod. This breaks the before/after stuff, because no matter the order that the widget chooses to request focus vs. passing down, the passed down command will not propagate further, because only the internal one will.

Thus, global/window commands need to be Event::Command for the root widget already and that exact same event must propagate down. (This is also how it is today.)

---

Even if we add the target to Event::Command then InternalEvent::TargetedCommand needs to still be there so that only the targeted widget receives it. The benefit of having the target in Event::Command is that WidgetPod recognizes that it was targeted to a specific widget and does not propagate it further -- and that the widget itself will be able to tell a targeted command apart from a window/global command.

---

Would it be an option to always send the original targeted command along, after sending the Command (in the current system?)

I'm not sure I understand your question.

---

More generally on the topic of targeting. If an event is targeted it should be routed via internal events and then synthesized for the specific target widget, and further propagation down would be blocked. Some events like the timer already work like this and I think more should, including a command targeted at a specific widget, which is what this PR is all about.

The major benefit of this precision targeting is a large reduction in boilerplate. Widgets don't have to ask the question whether an event was meant for them. If they receive an event - it's meant for them. Widgets also don't need logic to decide whether they should pass down an event to their children. They should just always do that and WidgetPod will only propagate events that make sense.

This means that widgets can check for event details and then do their reactive work. There's no need to have every widget decide about applicability to themselves or their children.

The only downside I can think of is that a group of WidgetPods might want to react to a single targeted event and don't want to use Data, but that seems somewhat contrived. Even so, I think a better solution than just re-introducing all the event applicability checking boilerplate would be to send a command targeted at those other WidgetPods.

[luleyleo]

I think adding a target directly to Command could work out well. As @xStrom described, we would still use InternalEvent::RouteCommand to get widget-targeted commands in place and in WidgetPod we just filter out all events targeted at a different widget.

This should give widgets complete control over when commands are propagated and automatically filters out widget-targeted commands.

Adding the target to Command instead of Event::Command would allow for a nice API change as well, as submit_command would no longer require passing a Target and could just set the Command internal target. Commands could then have a Command::to(mut self, Target) -> Self function to set the target. Then a command could be submitted like this:

ctx.submit_command(SELECTOR.with(payload).to(Target::Widget(id)))

[cmyr]

Okay, have we made a decision here? What can I do to help?

[cmyr]

I'm going to close this for now, just to clean up a bit; happy to pick it up again if there's further interest.