linebender / linebender/druid

Require widget to implement compute_max_intrinsic

Open
#2,383 7 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
9.7k
Forks
565
PR merge metrics
No merged PRs in 30d

Description

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](https://github.com/linebender/druid/pull/2382) 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](https://api.flutter.dev/flutter/rendering/RenderFractionallySizedOverflowBox/computeMaxIntrinsicWidth.html#:~:text=has%20usage%20examples.-,Implementation,-%40override%0Adouble%20computeMaxIntrinsicWidth)

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by locating the Widget::compute_max_intrinsic method and reviewing the env_scope implementation added in PR 2382. Inspect the event, lifecycle, update, layout, and paint patterns, then audit non-leaf and leaf widgets as described. Done means every required widget implementation preserves child intrinsic calls and leaf widgets use layout.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
frontend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.