-
Notifications
You must be signed in to change notification settings - Fork 6k
Use getBoundingRects to add support inset MediaQuery/SafeArea when in freeform mode controls are shown. #54294
Changes from 10 commits
34a0d12
de75b74
21c6111
7dc6abd
ee464bd
91abf9b
ec13d36
243486c
64f7bac
49475fb
9a428c5
e0d38da
a0a1cbc
87cd394
4cbc7a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1449,6 +1449,13 @@ public void removeFlutterEngineAttachmentListener( | |
| .send(); | ||
| } | ||
|
|
||
| private FlutterViewDelegate delegate = new FlutterViewDelegate(); | ||
|
|
||
| @VisibleForTesting | ||
| public void setDelegate(FlutterViewDelegate delegate) { | ||
|
reidbaker marked this conversation as resolved.
Outdated
|
||
| this.delegate = delegate; | ||
| } | ||
|
|
||
| private void sendViewportMetricsToFlutter() { | ||
|
reidbaker marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just tested this, and it looks good. I've moved it to |
||
| if (!isAttachedToFlutterEngine()) { | ||
| Log.w( | ||
|
|
@@ -1460,6 +1467,21 @@ private void sendViewportMetricsToFlutter() { | |
|
|
||
| viewportMetrics.devicePixelRatio = getResources().getDisplayMetrics().density; | ||
| viewportMetrics.physicalTouchSlop = ViewConfiguration.get(getContext()).getScaledTouchSlop(); | ||
|
|
||
|
reidbaker marked this conversation as resolved.
|
||
| if (Build.VERSION.SDK_INT >= API_LEVELS.API_35) { | ||
|
reidbaker marked this conversation as resolved.
Outdated
|
||
| List<Rect> boundingRects = delegate.getCaptionBarInsets(getContext()); | ||
| if (boundingRects != null && boundingRects.size() == 1) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it safe to ignore the boundingRects at positions 2 and beyond?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| // The value getCaptionBarInset returns is only the bounding rects of the caption bar. | ||
| // When assigning the new value of viewPaddingTop, the maximum is taken with its old value | ||
| // to ensure that any previous top padding that is greater than that from the caption bar | ||
| // is not destroyed by this operation. | ||
| viewportMetrics.viewPaddingTop = | ||
| Math.max(boundingRects.get(0).bottom, viewportMetrics.viewPaddingTop); | ||
| } | ||
| } else { | ||
| Log.w(TAG, "API level " + Build.VERSION.SDK_INT + " is too low to query bounding rects."); | ||
| } | ||
|
|
||
| flutterEngine.getRenderer().setViewportMetrics(viewportMetrics); | ||
| } | ||
|
|
||
|
|
@@ -1485,6 +1507,15 @@ public void setVisibility(int visibility) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Allow access to the viewport metrics so that tests can set them to be valid with nonzero | ||
| * dimensions. | ||
| */ | ||
| @VisibleForTesting | ||
| public FlutterRenderer.ViewportMetrics getViewportMetrics() { | ||
| return viewportMetrics; | ||
| } | ||
|
|
||
| /** | ||
| * Listener that is notified when a {@link io.flutter.embedding.engine.FlutterEngine} is attached | ||
| * to/detached from a given {@code FlutterView}. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| package io.flutter.embedding.android; | ||
|
|
||
| import android.app.Activity; | ||
| import android.content.Context; | ||
| import android.graphics.Rect; | ||
| import android.view.Window; | ||
| import android.view.WindowInsets; | ||
| import androidx.annotation.RequiresApi; | ||
| import androidx.annotation.VisibleForTesting; | ||
| import io.flutter.Build; | ||
| import io.flutter.util.ViewUtils; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| /** A delegate class that performs the task of retrieving the bounding rect values. */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there anything you can add here to let future maintainers know when logic should go into FlutterViewDelegate vs sendViewportMetricsToFlutter |
||
| public class FlutterViewDelegate { | ||
| /** | ||
| * Return the WindowInsets object for the provided Context. Returns null if there is no associated | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you document when 2 different context objects might have different windowInsets?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why this detail is relevant documentation to this method? I'm not sure I see the throughline.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| * activity, the window of the associated activity is null, or the root window insets of the | ||
| * activity's window is null. | ||
| */ | ||
| @RequiresApi(api = Build.API_LEVELS.API_23) | ||
| @VisibleForTesting | ||
| public WindowInsets getWindowInsets(Context context) { | ||
| Activity activity = ViewUtils.getActivity(context); | ||
| if (activity == null) { | ||
| return null; | ||
| } | ||
| Window window = activity.getWindow(); | ||
| if (window == null) { | ||
| return null; | ||
| } | ||
| return window.getDecorView().getRootWindowInsets(); | ||
| } | ||
|
|
||
| @RequiresApi(api = Build.API_LEVELS.API_35) | ||
|
reidbaker marked this conversation as resolved.
|
||
| @VisibleForTesting | ||
|
reidbaker marked this conversation as resolved.
Outdated
|
||
| public List<Rect> getCaptionBarInsets(Context context) { | ||
| WindowInsets insets = getWindowInsets(context); | ||
|
reidbaker marked this conversation as resolved.
|
||
| if (insets == null) { | ||
| return Collections.emptyList(); | ||
| } | ||
| return insets.getBoundingRects(WindowInsets.Type.captionBar()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should not be considered blocking.