Require widget to implement compute_max_intrinsic

[matthewgapp]

We should require widget to implement compute_max_intrinsic (remove the default implementation) so that ancestor calculations are correct.

I want to preface that by "correct", I mean that child implementations of compute_max_intrinsic are faithfully called. I could be misunderstanding the purpose or spirit of this method which would make my suggestion wrong.

A little bit of context: I've built a special scroll element that has its own implementation of compute_max_intrinsic (I do this so that I can call compute_max_intrinsic on the child instead of deferring to the default implementation (which calls layout on the scroll element). The motivation for my component-specific implementation isn't super important to this discussion, though. What's important is that certain widgets may have a good reason to implement their own version of compute_max_intrinsics.

Because the default implementation calls layout (instead of compute_max_instrinsic), the compute_max_intrinsic call chain is broken whenever an intermediate child uses the blanket implementation. This sucks and makes the compute_max_intrinsic method pretty useless in ancestor components when descendants have their own implementations.

This PR adds the necessary implementation to the env_scope component as a partial patch for my use case. But a better fix would remove the default implementation in favor of implementors being required to implement the compute_max_intrinsic (and thus call the correct method on children). This pattern is already leveraged for event, lifecycle, update, layout, and paint.

Naturally, removing the default implementation would be breaking so thought I'd bring it up here. But at the moment, the default implementation makes compute max intrinsic pretty useless/broken since any intermediate components that use the default implementation wipe out any widget-specific implementations downstream.

On a positive note, the breaking change would be pretty mechanical to fix, though. Just go through and call compute_max_intrinsic on every non-leaf widget and call layout on every leaf. This reflects how Flutter implements its intrinsic calculation

[matthewgapp]

@sjoshid @jneem since you authored and reviewed, respectively, the PR that introduced this method, would you have thoughts here?

[sjoshid]

Hmmm. What can't you override the blanket impl? Like, many other widgets.

[matthewgapp]

Hmmm. What can't you override the blanket impl? Like, many other widgets.

You can override it, but because it's not required, the widget authors may forget to implement it properly (ie, call the compute_max_intrinsic on the child), making any container widget that relies on the default implementation break the call chain and result in weird, hard-to-debug behavior. Container widgets should always propagate the calculation to its child/children but atm this isn't required.

[sjoshid]

All out-of-the-box containers handle it correctly. If it's a custom container, the onus is on the author of that container to override correctly. Druid, at the end of the day, is an opinioned framework.

Besides, if you remove the blanket impl, every widget out-of-the-box or not, must have an impl. What should be this default impl?

[sjoshid]

Also, I havent used Druid in a long time. If @jneem thinks what you propose makes sense, I have no objection. :)

[matthewgapp]

All out-of-the-box containers handle it correctly. If it's a custom container, the onus is on the author of that container to override correctly. Druid, at the end of the day, is an opinioned framework.

Yeah, maybe it's just a matter of educating authors and fixing up the widgets in /widget that don't properly propagate the calculation. Any widget that contains a child widget or a child WidgetPod should call the compute_max_intrinsic. Taking a glance at /widget, it appears most don't properly call compute_max_instrinsic but instead rely on the blanket implementation. So they should be fixed up.

[jneem]

I'm a bit conflicted here. I'm not sure it's reasonable to assume that compute_max_intrinsic will always be propagated recursively. The way druid allows custom implementation of its traits makes it hard to have firm rules like this. And going back to the original PR that introduced it, I'm pretty sure it was an explicit choice not to impose a new required method on all custom widgets.

That said, I'm 100% on board with adding the propagation to EnvScope and whatever other built-in widgets it makes sense for.