Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement compute max intrinsic on env scope #2382

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthewgapp
Copy link
Contributor

@matthewgapp matthewgapp commented Jun 4, 2023

Opening this non-breaking patch PR for a discussion on a breaking change to the Widget trait API.

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.

@matthewgapp
Copy link
Contributor Author

matthewgapp commented Jun 4, 2023

Moving discussion of the breaking change to #2383

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants