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

Reapply "Dynamic view sizing" (#140165) #140918

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

goderbauer
Copy link
Member

@goderbauer goderbauer commented Jan 3, 2024

This reverts commit d24c01b.

The original change was reverted because it caused some apps to get stuck on the splash screen on some phones.

An investigation determined that this was due to a rounding error. Example: The device reports a physical size of 1008.0 x 2198.0 with a dpr of 1.912500023841858. Flutter would translate that to a logical size of 527.0588169589221 x 1149.2810314243163 and use that as the input for its layout algorithm. Since the constraints here are tight, the layout algorithm would determine that the resulting logical size of the root render object must be 527.0588169589221 x 1149.2810314243163. Translating this back to physical pixels by applying the dpr resulted in a physical size of 1007.9999999999999 x 2198.0 for the frame. Android now rejected that frame because it didn't match the expected size of 1008.0 x 2198.0 and since no frame had been rendered would never take down the splash screen.

Prior to dynamically sized views, this wasn't an issue because we would hard-code the frame size to whatever the requested size was.

Changes in this PR over the original PR:

  • The issue has been fixed now by constraining the calculated physical size to the input physical constraints which makes sure that we always end up with a size that is acceptable to the operating system.
  • The ViewConfiguration was refactored to use the slightly more convenient BoxConstraints over the ViewConstraints to represent constraints. Both essentially represent the same thing, but BoxConstraints are more powerful and we avoid a couple of translations between the two by translating the ViewConstraints from the FlutterView to BoxConstraints directly when the ViewConfiguration is created.

All changes over the original PR are contained in the second commit of this PR.

Fixes b/316813075
Part of #134501.

@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Jan 3, 2024
@goderbauer goderbauer marked this pull request as ready for review January 3, 2024 21:36
@goderbauer
Copy link
Member Author

FYI @jiahaog @chingjun @ditman

@@ -450,7 +450,7 @@ void main() {
expect(children, isNot(contains(rawViewOwner)));
});

testWidgetsWithLeakTracking('RenderView does not use size of child if constraints are tight', (WidgetTester tester) async {
testWidgets('RenderView does not use size of child if constraints are tight', (WidgetTester tester) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no leak tracking?

Copy link
Member Author

Choose a reason for hiding this comment

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

Between reverting and relanding this change the testWidgetsWithLeakTracking method was removed in #140239. It's functionality is now folded into testWidgets directly.

@@ -3,7 +3,7 @@
// found in the LICENSE file.

import 'dart:io' show Platform;
import 'dart:ui' as ui show FlutterView, Scene, SceneBuilder, SemanticsUpdate, ViewConstraints;
import 'dart:ui' as ui show FlutterView, Scene, SceneBuilder, SemanticsUpdate;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the ViewConfiguration doesn't use ViewConstraints anymore, do we need the distinction still? It doesn't appear to be used anywhere in the framework (although it is used in the engine).

Copy link
Member Author

@goderbauer goderbauer Jan 3, 2024

Choose a reason for hiding this comment

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

We still need it for the type used by FlutterView.physicalConstraints in the engine since the engine doesn't have access to the BoxConstraints type. This is similar to how the engine uses ViewPadding that the framework then translates to EdgeInsets at its earliest convenience because the second one is more powerful.

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

@goderbauer
Copy link
Member Author

Waiting for confirmation on b/316813075 before merging this.

@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 9, 2024
@goderbauer goderbauer merged commit 4534a24 into flutter:master Jan 9, 2024
67 checks passed
@goderbauer goderbauer deleted the dynamicView2 branch January 9, 2024 22:10
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 10, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jan 10, 2024
flutter/flutter@126302d...b840a60

2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3269fd84460d to b361a60ae224 (1 revision) (flutter/flutter#141271)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from a5d446da5495 to 3269fd84460d (1 revision) (flutter/flutter#141264)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3ccf66bed335 to a5d446da5495 (2 revisions) (flutter/flutter#141252)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from e57e418c02ae to 3ccf66bed335 (1 revision) (flutter/flutter#141241)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7e6f3d847e01 to e57e418c02ae (1 revision) (flutter/flutter#141240)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1cf2e0a603c7 to 7e6f3d847e01 (1 revision) (flutter/flutter#141237)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 32bbf8be8d2c to 1cf2e0a603c7 (1 revision) (flutter/flutter#141232)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5b9d2132b7cd to 32bbf8be8d2c (1 revision) (flutter/flutter#141229)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 941f268fc8bb to 5b9d2132b7cd (1 revision) (flutter/flutter#141228)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 542fea9edae4 to 941f268fc8bb (3 revisions) (flutter/flutter#141224)
2024-01-10 Michal-MK@users.noreply.github.com `NestedScrollView`'s outer scrollable jumping with `BouncingScrollPhysics` due to `double` precision errors (flutter/flutter#138319)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8c1501f3956d to 542fea9edae4 (1 revision) (flutter/flutter#141217)
2024-01-10 polinach@google.com Fix or except leaks. (flutter/flutter#141081)
2024-01-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 693af0c699c5 to 8c1501f3956d (1 revision) (flutter/flutter#141215)
2024-01-09 cbobbe@zulip.com TextStyle: In copyWith, stop ignoring debugLabel when receiver has none (flutter/flutter#141141)
2024-01-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from a35e3b026e1d to 693af0c699c5 (5 revisions) (flutter/flutter#141209)
2024-01-09 stuartmorgan@google.com Replace deprecated `exists` in podhelper.rb (flutter/flutter#141169)
2024-01-09 kevmoo@users.noreply.github.com Correctly handle null case in ProcessText.queryTextActions (flutter/flutter#141205)
2024-01-09 polinach@google.com Add environment variable to leak tracking bots. (flutter/flutter#141137)
2024-01-09 goderbauer@google.com Reapply "Dynamic view sizing" (#140165) (flutter/flutter#140918)
2024-01-09 89970141+SharbelOkzan@users.noreply.github.com Introduce new Form validation method  (flutter/flutter#135578)
2024-01-09 tessertaha@gmail.com Update `RouteObserver` example and fix an error thrown (flutter/flutter#141166)
2024-01-09 polinach@google.com Upgrade leak_tracker. (flutter/flutter#141153)
2024-01-09 chillers@google.com [ci.yaml] Do not run packaging test on presubmit (flutter/flutter#141192)
2024-01-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 036b39fa47fa to a35e3b026e1d (6 revisions) (flutter/flutter#141191)
2024-01-09 15619084+vashworth@users.noreply.github.com Run tests on iOS 16 or iOS 17 (flutter/flutter#141178)
2024-01-09 polinach@google.com Remove conditions that depend on order. (flutter/flutter#141183)
2024-01-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from b3c8597df0e2 to 036b39fa47fa (1 revision) (flutter/flutter#141179)
2024-01-09 148634283+nikkivirtuoso@users.noreply.github.com resolved the issue of indeterminate CircularProgressIndicator.adaptive on Darwin  (flutter/flutter#140947)

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

To file a bug in Packages: 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
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…r#5855)

flutter/flutter@126302d...b840a60

2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3269fd84460d to b361a60ae224 (1 revision) (flutter/flutter#141271)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from a5d446da5495 to 3269fd84460d (1 revision) (flutter/flutter#141264)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3ccf66bed335 to a5d446da5495 (2 revisions) (flutter/flutter#141252)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from e57e418c02ae to 3ccf66bed335 (1 revision) (flutter/flutter#141241)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7e6f3d847e01 to e57e418c02ae (1 revision) (flutter/flutter#141240)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1cf2e0a603c7 to 7e6f3d847e01 (1 revision) (flutter/flutter#141237)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 32bbf8be8d2c to 1cf2e0a603c7 (1 revision) (flutter/flutter#141232)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5b9d2132b7cd to 32bbf8be8d2c (1 revision) (flutter/flutter#141229)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 941f268fc8bb to 5b9d2132b7cd (1 revision) (flutter/flutter#141228)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 542fea9edae4 to 941f268fc8bb (3 revisions) (flutter/flutter#141224)
2024-01-10 Michal-MK@users.noreply.github.com `NestedScrollView`'s outer scrollable jumping with `BouncingScrollPhysics` due to `double` precision errors (flutter/flutter#138319)
2024-01-10 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8c1501f3956d to 542fea9edae4 (1 revision) (flutter/flutter#141217)
2024-01-10 polinach@google.com Fix or except leaks. (flutter/flutter#141081)
2024-01-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 693af0c699c5 to 8c1501f3956d (1 revision) (flutter/flutter#141215)
2024-01-09 cbobbe@zulip.com TextStyle: In copyWith, stop ignoring debugLabel when receiver has none (flutter/flutter#141141)
2024-01-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from a35e3b026e1d to 693af0c699c5 (5 revisions) (flutter/flutter#141209)
2024-01-09 stuartmorgan@google.com Replace deprecated `exists` in podhelper.rb (flutter/flutter#141169)
2024-01-09 kevmoo@users.noreply.github.com Correctly handle null case in ProcessText.queryTextActions (flutter/flutter#141205)
2024-01-09 polinach@google.com Add environment variable to leak tracking bots. (flutter/flutter#141137)
2024-01-09 goderbauer@google.com Reapply "Dynamic view sizing" (#140165) (flutter/flutter#140918)
2024-01-09 89970141+SharbelOkzan@users.noreply.github.com Introduce new Form validation method  (flutter/flutter#135578)
2024-01-09 tessertaha@gmail.com Update `RouteObserver` example and fix an error thrown (flutter/flutter#141166)
2024-01-09 polinach@google.com Upgrade leak_tracker. (flutter/flutter#141153)
2024-01-09 chillers@google.com [ci.yaml] Do not run packaging test on presubmit (flutter/flutter#141192)
2024-01-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 036b39fa47fa to a35e3b026e1d (6 revisions) (flutter/flutter#141191)
2024-01-09 15619084+vashworth@users.noreply.github.com Run tests on iOS 16 or iOS 17 (flutter/flutter#141178)
2024-01-09 polinach@google.com Remove conditions that depend on order. (flutter/flutter#141183)
2024-01-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from b3c8597df0e2 to 036b39fa47fa (1 revision) (flutter/flutter#141179)
2024-01-09 148634283+nikkivirtuoso@users.noreply.github.com resolved the issue of indeterminate CircularProgressIndicator.adaptive on Darwin  (flutter/flutter#140947)

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

To file a bug in Packages: 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: tests "flutter test", flutter_test, or one of our tests autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants