Use getBoundingRects to add support inset MediaQuery/SafeArea when in freeform mode controls are shown. #54294

yaakovschectman · 2024-08-01T21:13:07Z

Original Title: Add FlutterViewDelegate and BoundingRect methods

Check the bounding rect for caption bar when sending viewport metrics to Flutter to account for freeform mode. Use the more recent getBoundingRects over getInsets.

Tests will need to be kept separate until Robolectric publishes a version that supports API level 35.
Roboletric tests for this pr will be part of google testing until then.
Pr for tests here https://critique.corp.google.com/cl/657302386.

flutter/flutter#146658

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

shell/platform/android/io/flutter/embedding/android/FlutterViewDelegate.java

shell/platform/android/io/flutter/embedding/android/FlutterView.java

gmackall

LGTM outside of nit

shell/platform/android/io/flutter/embedding/android/FlutterViewDelegate.java

chinmaygarde · 2024-08-15T20:02:34Z

Is this blocked on anything? Looks good to go.

reidbaker · 2024-08-15T20:55:39Z

Is this blocked on anything? Looks good to go.

It is waiting on my review. I was reviewing the internal cl which has the test but the files diverged.

flutter-dashboard · 2024-08-16T14:51:39Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

shell/platform/android/io/flutter/embedding/android/FlutterView.java

reidbaker · 2024-08-16T14:57:18Z

shell/platform/android/io/flutter/embedding/android/FlutterView.java

+
+    if (Build.VERSION.SDK_INT >= API_LEVELS.API_35) {
+      List<Rect> boundingRects = delegate.getCaptionBarInsets(getContext());
+      if (boundingRects != null && boundingRects.size() == 1) {


Why is it safe to ignore the boundingRects at positions 2 and beyond?

I had assumed the caption bar would have only one rect. If this is false, what would you suppose we do if there are more, considering that the viewport metrics describe only a bounding rect for the view? Get the maximum to use for the padding value?

We can sometimes work from assumptions. I am asking what evidence justifies ignoring that the api returns a list. I dont know if bounding recs are a single value or list. That is your responsibility as the author of a change.

When/if we would get multiple recs would help us decide what to do? Assuming we dont have that information then I would think the largest value is probably the one to choose.

reidbaker · 2024-08-16T14:58:58Z

shell/platform/android/io/flutter/embedding/android/FlutterViewDelegate.java

+import java.util.Collections;
+import java.util.List;
+
+/** A delegate class that performs the task of retrieving the bounding rect values. */


Is there anything you can add here to let future maintainers know when logic should go into FlutterViewDelegate vs sendViewportMetricsToFlutter

reidbaker · 2024-08-16T14:59:43Z

shell/platform/android/io/flutter/embedding/android/FlutterViewDelegate.java

+/** A delegate class that performs the task of retrieving the bounding rect values. */
+public class FlutterViewDelegate {
+  /**
+   * Return the WindowInsets object for the provided Context. Returns null if there is no associated


Can you document when 2 different context objects might have different windowInsets?

Can you explain why this detail is relevant documentation to this method? I'm not sure I see the throughline.

The dart docs is a hint for callers and for maintainers to help understand the behavior of the code documented. The existing dart doc, as written, helps a little bit by describing the null behavior but it could be more helpful if it describe when context might change. That is because the caller can know for their use case of FlutterViewDelegate if it matters which context to pass.

For example if the android documentation indicates that window insets will always be the same for an app regardless of which of the apps context is used it indicated to the caller that it might be safe in situations when launching another activity to use the application context. However if insets might change across activities maybe because of their window flags then it is important to use one FlutterViewDelate per activity.

shell/platform/android/io/flutter/embedding/android/FlutterViewDelegate.java

shell/platform/android/io/flutter/embedding/android/FlutterView.java

reidbaker · 2024-08-19T15:00:33Z

shell/platform/android/io/flutter/embedding/android/FlutterView.java

+  public void setDelegate(@NonNull FlutterViewDelegate delegate) {
+    this.delegate = delegate;
+  }
+
  private void sendViewportMetricsToFlutter() {


Looking at the design of this class I am seeing a pattern of the flutterView having methods that trigger on different android lifecycle events, Then ViewportMetrics being updated then at the end of that triggering method calling sendViewportMetricsToFlutter.

Looking at this code we do the calculation every send. When I was searching internally I did not find documentation of a lifecycle method to trigger on so maybe this is moot. If this code moved to onApplyWindowInsets would it still work?

I just tested this, and it looks good. I've moved it to onApplyWindowInsets after the existing calculations.

reidbaker · 2024-08-19T15:04:49Z

shell/platform/android/io/flutter/embedding/android/FlutterView.java

Digging into go/customizable-window-headers I found APPEARANCE_TRANSPARENT_CAPTION_BAR_BACKGROUND Can you test an app with that flag set vs the default and come up with a recommendation on if we should apply transparent caption bar by default or not?

This comment should not be considered blocking.

reidbaker · 2024-08-19T18:43:04Z

shell/platform/android/io/flutter/embedding/android/FlutterViewDelegate.java

+   * Return the WindowInsets object for the provided Context. A Context will only have a window if
+   * it is an instance of Activity. If context does not have a window, or it is not an activity,
+   * this method will return null. Otherwise, this method will return the WindowInsets for the
+   * provided activity's window.


Great documentation

shell/platform/android/io/flutter/embedding/android/FlutterViewDelegate.java

…ea when in freeform mode controls are shown. (flutter/engine#54294)

…153781) flutter/engine@e10b075...d6bc4dc 2024-08-20 [email protected] Use getBoundingRects to add support inset MediaQuery/SafeArea when in freeform mode controls are shown. (flutter/engine#54294) 2024-08-20 [email protected] Manual roll Dart SDK from c5264a1bd1d2 to c22bf5aedbcf (2 revisions) (flutter/engine#54646) 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 [email protected],[email protected],[email protected] 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

…lutter#153781) flutter/engine@e10b075...d6bc4dc 2024-08-20 [email protected] Use getBoundingRects to add support inset MediaQuery/SafeArea when in freeform mode controls are shown. (flutter/engine#54294) 2024-08-20 [email protected] Manual roll Dart SDK from c5264a1bd1d2 to c22bf5aedbcf (2 revisions) (flutter/engine#54646) 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 [email protected],[email protected],[email protected] 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

mengyanshou · 2025-01-09T10:36:16Z

@yaakovschectman Please take a look, this submission affects all Futter3.27.X In all Xiaomi Android 15 devices, in the suspended window mode, all apps built by Futter3.27.X will be white screen

reidbaker · 2025-01-09T14:38:18Z

@mengyanshou yaakov no longer contributes to flutter it is unlikely you will get a response. Please file an issue so that the triage process can start.

yaakovschectman added 3 commits August 1, 2024 17:08

Add FlutterViewDelegate

34a0d12

Use max

de75b74

Remove debug logs

21c6111

yaakovschectman requested review from matanlurey, reidbaker, jonahwilliams and Nayuta403 August 1, 2024 21:13

yaakovschectman self-assigned this Aug 1, 2024

github-actions bot added the platform-android label Aug 1, 2024

yaakovschectman marked this pull request as ready for review August 2, 2024 16:30

yaakovschectman added 4 commits August 5, 2024 14:49

Feedback

7dc6abd

FlutterViewDelegate to own class

ee464bd

Formatting

91abf9b

Remove mistake lien

ec13d36

gmackall reviewed Aug 6, 2024

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterViewDelegate.java Outdated Show resolved Hide resolved

shell/platform/android/io/flutter/embedding/android/FlutterView.java Outdated Show resolved Hide resolved

PR Feedback

243486c

gmackall approved these changes Aug 6, 2024

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterViewDelegate.java Outdated Show resolved Hide resolved

yaakovschectman added 3 commits August 6, 2024 17:32

Test/PR feedback

64f7bac

Explanatory comment

49475fb

Delegate maximizing viewport metric padding

9a428c5

reidbaker changed the title ~~Add FlutterViewDelegate and BoundingRect methods~~ Use getBoundingRects to add support inset MediaQuery/SafeArea when in freeform mode controls are shown. Aug 16, 2024

reidbaker suggested changes Aug 16, 2024

View reviewed changes

yaakovschectman added 2 commits August 16, 2024 13:52

Format

e0d38da

PR feedback

a0a1cbc

reidbaker reviewed Aug 19, 2024

View reviewed changes

Comments

87cd394

reidbaker reviewed Aug 19, 2024

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterViewDelegate.java Outdated Show resolved Hide resolved

reidbaker reviewed Aug 19, 2024

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterViewDelegate.java Outdated Show resolved Hide resolved

Move caption bar logic

4cbc7a4

yaakovschectman requested a review from reidbaker August 19, 2024 20:00

reidbaker approved these changes Aug 20, 2024

View reviewed changes

yaakovschectman merged commit d6bc4dc into flutter:main Aug 20, 2024
28 checks passed

engine-flutter-autoroll mentioned this pull request Aug 20, 2024

Roll Flutter Engine from e10b07598091 to d6bc4dc6e59d (2 revisions) flutter/flutter#153781

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 20, 2024

d6bc4dc6e Use getBoundingRects to add support inset MediaQuery/SafeAr…

bc6b4ab

…ea when in freeform mode controls are shown. (flutter/engine#54294)

yaakovschectman mentioned this pull request Aug 21, 2024

Remove log message about API level #54687

Closed

8 tasks

lycstar mentioned this pull request Jan 3, 2025

Incorrect MediaQuery padding in small window (freeform) mode on xiaomi device(Xiaomi 14, HyperOS2.0.7.0, Android15, Api35) causing abnormal layout on Flutter 3.27.X flutter/flutter#161086

Open

Use getBoundingRects to add support inset MediaQuery/SafeArea when in freeform mode controls are shown. #54294

Use getBoundingRects to add support inset MediaQuery/SafeArea when in freeform mode controls are shown. #54294

Uh oh!

Conversation

yaakovschectman commented Aug 1, 2024 • edited by reidbaker Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

gmackall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chinmaygarde commented Aug 15, 2024

Uh oh!

reidbaker commented Aug 15, 2024

Uh oh!

flutter-dashboard bot commented Aug 16, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reidbaker Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mengyanshou commented Jan 9, 2025

Uh oh!

reidbaker commented Jan 9, 2025

Uh oh!

Uh oh!

yaakovschectman commented Aug 1, 2024 •

edited by reidbaker

Loading

reidbaker Aug 16, 2024 •

edited

Loading