Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@goderbauer
Copy link
Member

@goderbauer goderbauer commented Nov 15, 2023

Towards flutter/flutter#134501.

This PR makes the following changes to the public dart:ui API:

  • It adds the FlutterView.pysicalConstraints property that describes max and min width and height for a view. The framework is allowed to size the FlutterView to any Size that meets these constraints.
  • It adds an optional size argument to FlutterView.render. The framework provides the chosen Size that meets the aforementioned constraints to the render method. If the FlutterView.pysicalConstraints are tight (minHeight == maxHeight and minWidth == maxWidth) the argument is optional to remain backwards compatible. In all other cases, a Size must be provided.
  • It adds a ViewConstraints class, which is basically the dart:ui version of BoxConstraints (This is similar to how we have ViewPadding in dart:ui to mirror EdgeInsets from the framework). It describes the constraints of a FlutterView, i.e. it powers the FlutterView.pysicalConstraints property.

This change does not wire anything up to the embedders. For now, FlutterView.pysicalConstraints just returns tight constraints for the embedder-provided size of the view (FlutterView.physicalSize) and the size provided to FlutterView.render is ignored (after it is checked that it meets the constrains).

This PR enables the framework to implement the new dynamic view sizing and embedders to separately expose the new functionality to their clients.

Presubmits will fail until flutter/flutter#138565 is submitted to the framework.

DO NOT SUBMIT until flutter/flutter#138648 is ready.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Nov 15, 2023
@goderbauer goderbauer marked this pull request as ready for review November 16, 2023 20:37
@goderbauer
Copy link
Member Author

Quick note: The framework tests are failing until flutter/flutter#138565 is submitted to the framework to prepare for this PR.

final double devicePixelRatio;

/// The size requested for the view in logical pixels.
/// The size requested for the view in physical pixels.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did we find out this should be physical instead of logical? Is it a bug fix or a behavior change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value was always in physical pixels, it was just documented wrong. Luckily, it is a doc comment on a private class, so the doc was never actually public.

Copy link
Member Author

@goderbauer goderbauer Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backup evidence that this has always been in physical pixels and we just documented it wrong: This value is returned by the physicalSize getter on FlutterView:

Size get physicalSize => _viewConfiguration.size;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found Physical vs Logical pixels is painful in the web, especially if one trusts the docs :P

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

(Here's the changes I did to get web support)

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

});

/// Creates view constraints that is respected only by the given size.
ViewConstraints.tight(Size size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this be const?

Copy link
Member Author

@goderbauer goderbauer Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size.width and size.height are invalid constant values, according to Dart (In theory, they could be getters that do some crazy computation using information only available at runtime.). Since they are used in the initializer list below, the constructor cannot be const, sadly.

(Same applies to the existing BoxConstraints.tight constructor)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, right, makes sense. Too bad there's no way to make a final property not be overridable with a getter.

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 27, 2023
Towards #134501.

Required to roll flutter/engine#48090 into the framework.

**DO NOT SUBMIT until #138648 and flutter/engine#48090 are ready.**
@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 27, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 27, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 27, 2023

auto label is removed for flutter/engine/48090, due to - The status or check suite Linux Framework Smoke Tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

///
/// At startup, the constraints for the view may not be known before Dart code
/// runs. If this value is observed early in the application lifecycle, it may
/// report constraints with all dimensions set to zero.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does "constraints with all dimensions set to zero" mean the resulting size must be 0? I don't know if this sentence needs to be said, because the sentence alone feels important on the surface but it's really just a temporary state. And the constraints are less like "known" but more like "set" by the engine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it means the only size allowed will be zero. It matches what was previously documented on physicalSize.

Comment on lines +155 to +156
/// This value does not take into account any on-screen keyboards or other
/// system UI. If the constraints are tight, the [padding] and [viewInsets]
Copy link
Contributor

@dkwingsmt dkwingsmt Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does "not take into account" mean the size include or exclude these system UI? I think "include/exclude" will be clearer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is using the same terms we use on physicalSize to document the same thing). I find include/exclude more confusing because it depends on your perspective whether the padding is included or excluded.

Comment on lines +158 to +159
/// be obscured by system UI. If the constraints are loose, this information
/// is not known upfront.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "this information is not known"? Shall we just assert that only tight constraints can have non-zero padding and insets?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically describing platform limitations. No platform currently makes it possible to know this information upfront, but in theory a platform could provide it.

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 27, 2023
Towards #134501.

Required to roll flutter/engine#48090 into the framework.

Two new subclasses of FlutterView were added recently for testing (in #138849) that I missed in my previous PR (#138565).
goderbauer and others added 10 commits November 27, 2023 11:53
Co-authored-by: David Iglesias <ditman@gmail.com>
Co-authored-by: David Iglesias <ditman@gmail.com>
Co-authored-by: David Iglesias <ditman@gmail.com>
@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 27, 2023
@auto-submit auto-submit bot merged commit 2be7191 into flutter:main Nov 27, 2023
@goderbauer goderbauer deleted the dynamic-view-sizing branch November 27, 2023 23:03
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 27, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 27, 2023
…139103)

flutter/engine@0a098bd...2be7191

2023-11-27 goderbauer@google.com Dynamic view sizing [dart:ui] (flutter/engine#48090)
2023-11-27 dnfield@google.com Roll shaderc to google/shaderc@37e2553 (flutter/engine#48415)
2023-11-27 skia-flutter-autoroll@skia.org Roll Skia from 4c964f8c4738 to 600986ba305d (6 revisions) (flutter/engine#48419)
2023-11-27 1961493+harryterkelsen@users.noreply.github.com [canvaskit] Enable multiview rendering (flutter/engine#48301)
2023-11-27 30870216+gaaclarke@users.noreply.github.com [Impeller] updated gaussian blur tests to use device private textures (flutter/engine#48417)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 29, 2023
Towards #134501.

This change is based on flutter/engine#48090. It changes the `RenderView` to be dynamically sized based on its content if the `FlutterView` it is configured with allows it (i.e. the `FlutterView` has loose `FlutterView.physicalConstraints`). For that, it uses those `physicalConstraints` as input to the layout algorithm by passing them on to its child (after translating them to logical constraints via the device pixel ratio). The resulting `Size` that the `RenderView` would like to be is then communicated back to the engine by passing it to the `FlutterView.render` call.

Tests will fail until flutter/engine#48090 has rolled into the framework.
caseycrogers pushed a commit to caseycrogers/flutter that referenced this pull request Dec 29, 2023
Towards flutter#134501.

Required to roll flutter/engine#48090 into the framework.

**DO NOT SUBMIT until flutter#138648 and flutter/engine#48090 are ready.**
caseycrogers pushed a commit to caseycrogers/flutter that referenced this pull request Dec 29, 2023
Towards flutter#134501.

Required to roll flutter/engine#48090 into the framework.

Two new subclasses of FlutterView were added recently for testing (in flutter#138849) that I missed in my previous PR (flutter#138565).
caseycrogers pushed a commit to caseycrogers/flutter that referenced this pull request Dec 29, 2023
…lutter#139103)

flutter/engine@0a098bd...2be7191

2023-11-27 goderbauer@google.com Dynamic view sizing [dart:ui] (flutter/engine#48090)
2023-11-27 dnfield@google.com Roll shaderc to google/shaderc@37e2553 (flutter/engine#48415)
2023-11-27 skia-flutter-autoroll@skia.org Roll Skia from 4c964f8c4738 to 600986ba305d (6 revisions) (flutter/engine#48419)
2023-11-27 1961493+harryterkelsen@users.noreply.github.com [canvaskit] Enable multiview rendering (flutter/engine#48301)
2023-11-27 30870216+gaaclarke@users.noreply.github.com [Impeller] updated gaussian blur tests to use device private textures (flutter/engine#48417)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
caseycrogers pushed a commit to caseycrogers/flutter that referenced this pull request Dec 29, 2023
Towards flutter#134501.

This change is based on flutter/engine#48090. It changes the `RenderView` to be dynamically sized based on its content if the `FlutterView` it is configured with allows it (i.e. the `FlutterView` has loose `FlutterView.physicalConstraints`). For that, it uses those `physicalConstraints` as input to the layout algorithm by passing them on to its child (after translating them to logical constraints via the device pixel ratio). The resulting `Size` that the `RenderView` would like to be is then communicated back to the engine by passing it to the `FlutterView.render` call.

Tests will fail until flutter/engine#48090 has rolled into the framework.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants