Lifecycle issues on dynamic widget change

[raphlinus]

This is a crash when using master Druid with the Crochet prototype. The lifecycle problems were happening before, but only giving warnings. Now text layout is highly dependent on getting lifecycle right. Though it's observed in Crochet, I'm pretty sure it's a problem in Druid, and will probably bite us any time we're trying to reconfigure children dynamically.

The following println-debugging trace should give a clue what's going wrong. It's from a simplified version of the "counter" example, stripped down to just be a label. I can post a gist of the example, and a diff adding the println, if it'd be useful.

anywidget lifecycle WidgetAdded
anywidget event WindowConnected
children changed
anywidget layout
anywidget layout
label layout
anywidget lifecycle Size(4.0W×16.0H)
label lifecycle Size(4.0W×16.0H)
anywidget lifecycle Size(1427.0W×726.0H)
anywidget lifecycle Internal(RouteWidgetAdded)
anywidget lifecycle WidgetAdded
label lifecycle WidgetAdded
anywidget paint
anywidget paint
label paint
thread 'main' panicked at 'TextLayout::draw called without rebuilding layout object. Text was 'Foo'', C:\Users\raph\github\druid\druid\src\text\layout.rs:291:9

The "children changed" log is when the child widget (a label) gets added to the (modified) flex container; it's this line in particular.

The underlying problem seems pretty simple: it's doing the WidgetAdded lifecycle after the layout, which seems pretty clearly wrong to me. I think this problem may be hidden in other examples because there might be an intervening update or something, but I also wouldn't be surprised if it were possible to trigger in extant code even outside Crochet.

I haven't investigated the lifecycle ordering logic carefully yet. It feels like a bit of a haunted graveyard. But I am willing to dig in.

(PS: yes, the × symbol is mojibake. Because it's 2020 and all software is broken)

[luleyleo]

This sounds like #1127?
If that is the problem, I've investigated this some time ago and I think I have a patch for this in one of my branches. I'll refresh my memory about this tomorrow and see what I got.

[raphlinus]

Yes, on quick skim it seems likely that's the same problem. It would be sweet if there's a patch for it.

[luleyleo]

I found the branch and updated it, I'll have to fix some of out tests though because they do not even honor WidgetAdded...
For that I'll have to get into that test harness stuff so I probably won't get to finish it today.

[luleyleo]

So after 4 to 5 hours of madness I believe I found the reason for this.

First of, if I am correct, this is purely a Crochet issue and not really Druid's fault.

What I believe causes this, is that Crochet's mutate method does not use the correct EventCtx for the widgets, but instead propagates down the one from the root widget. If you take a look at the WidgetPod::event method:

druid/druid/src/core.rs

Lines 704 to 719 in ecd0a24

	if recurse {
	let mut inner_ctx = EventCtx {
	cursor: ctx.cursor,
	state: ctx.state,
	widget_state: &mut self.state,
	is_handled: false,
	is_root: false,
	};
	let inner_event = modified_event.as_ref().unwrap_or(event);
	inner_ctx.widget_state.has_active = false;
	self.inner.event(&mut inner_ctx, &inner_event, data, env);
	inner_ctx.widget_state.has_active |= inner_ctx.widget_state.is_active;
	ctx.is_handled |= inner_ctx.is_handled;
	}

you can see, that is uses an inner_ctx for the child. Because we don't do this children_changed never becomes true for any widgets other than the root, which means that this part of WidgetPod::lifecycle always gets skipped:

druid/druid/src/core.rs

Lines 736 to 751 in ecd0a24

	InternalLifeCycle::RouteWidgetAdded => {
	// if this is called either we were just created, in
	// which case we need to change lifecycle event to
	// WidgetAdded or in case we were already created
	// we just pass this event down
	if self.old_data.is_none() {
	self.lifecycle(ctx, &LifeCycle::WidgetAdded, data, env);
	return;
	} else {
	if self.state.children_changed {
	self.state.children.clear();
	self.state.focus_chain.clear();
	}
	self.state.children_changed
	}
	}

And thus WidgetAdded never gets propagated anywhere but to the root.

So far I have no idea how we could do this correctly, because we have no way of constructing such a "child context" from Crochet.

[raphlinus]

Thanks, this is a very useful analysis, and not what I was expecting. I haven't thought about it too deeply yet, but it seems like the right path forward might be to add a method to Druid to create an appropriate child context.

[luleyleo]

Closed by #1352 and raphlinus/crochet#19 🎉