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

Pass Android Q insets.systemGestureInsets to Window #10413

Merged
merged 31 commits into from Aug 16, 2019

Conversation

@shihaohong
Copy link
Contributor

commented Aug 1, 2019

Description:

This passes Android Q's insets.systemGestureInsets to the Window. This becomes necessary as Flutter app developers want to take advantage of their entire screen with full gestural navigation mode coming with Q.

Edit: Since unit testing for Android Q involves a lot of unknowns and issues (see flutter/flutter#38471), we're going to add tests as a follow up to this PR once a solution is found

Issues:

Related to flutter/flutter#34678, flutter/flutter#34678

Todos:

shihaohong added 3 commits Jul 31, 2019
Not able to properly retrieve systemGestureInsets yet
shihaohong added 2 commits Aug 1, 2019
Copy link
Member

left a comment

Have a few queries that would be great to have clarified in docstrings:

How do the view and gesture insets interact? Does it matter if the system puts gestures recognizers vs window decorations in that area? Not sure it does. An argument could be made that the Flutter application author should not put interactive controls in the gesture insets but is safe to do so within view insets. But the view insets have been shown to be accommodations for status bars and such (which are gesture sensitive). If there is no difference, then the gesture insets could be specified in the current view insets.

@shihaohong

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

The main reason they're kept separate is that I feel that the view and gesture insets feel like orthogonal use cases.

For example, there will be system gesture insets on the vertical edges of the screen. App developers may want to paint window decorations on there. However, for widgets with gesture detectors, they may want to simply avoid the system gesture recognizer using the gesture inset.

Android Q also provides an API to override some of their system gesture insets if they needed to do so (I haven't gotten around to exposing this API yet). In this case, if both system view and system gesture insets are added together, it's inconvenient for the app developer if they'd like to add a decoration/gesture detector in the area where the system gesture was removed, since they can no longer distinguish between system view insets and system gesture insets.

Copy link
Member

left a comment

lgtm with nit about the names of the variables.

@@ -59,6 +63,10 @@ struct ViewportMetrics {
double physical_view_inset_left = 0;
double physical_view_inset_front = kUnsetDepth;
double physical_view_inset_back = kUnsetDepth;
double system_gesture_inset_top = 0;

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 1, 2019

Member

Keeping with the naming convention used above, these would be physical_system_gesture_inset_* or physical_gesture_inset_*. I think we should stick to that scheme for consistency. It is also clearer about the fact that those coordinates do not take into account the device pixel ratio.

Copy link
Contributor

left a comment

LGTM

@@ -666,6 +666,24 @@ class Window {
WindowPadding get viewPadding => _viewPadding;
WindowPadding _viewPadding = WindowPadding.zero;

/// The number of physical pixels on each side of the display rectangle into
/// which the application can render, but over which the operating system
/// will likely place system gestures.

This comment has been minimized.

Copy link
@HansMuller

HansMuller Aug 1, 2019

Contributor

but over which the operating system will likely place system gestures => but where the operating system will consume input gestures for the sake of system navigation.

/// screen, where swiping inwards from the edges takes users backward
/// through the history of screens they previously visited.
///
/// When this changes, [onMetricsChanged] is called.

This comment has been minimized.

Copy link
@HansMuller

HansMuller Aug 1, 2019

Contributor

this => this property

@shihaohong shihaohong referenced this pull request Aug 6, 2019
@shihaohong shihaohong requested a review from dnfield Aug 6, 2019
DEPS Outdated
@@ -459,7 +459,7 @@ deps = {
'packages': [
{
'package': 'flutter/android/sdk/platforms',
'version': 'version:28r6'
'version': 'version:29r1'

This comment has been minimized.

Copy link
@dnfield

dnfield Aug 6, 2019

Member

How sure are we that this is safe? This means that now all our builds will go against the 29 SDK. Is it stable enough yet?

This comment has been minimized.

Copy link
@shihaohong

shihaohong Aug 6, 2019

Author Contributor

As mentioned in the buildroot PR (posting here for posterity as well), this isn't certain, but it'd be ideal to support system gestural navigation as soon as possible so that Flutter devs can catch up to this update. Originally, I was using reflection to handle this particular case, but a lint recommended against doing so.

On one hand, we could move forward with using reflection, and once v29 of the Android SDK is stable, roll v29 into the engine and then remove the use of reflection.

This comment has been minimized.

Copy link
@dnfield

dnfield Aug 6, 2019

Member

Ok. Can we roll th eother SDK packages with this one? Especially build tools.

@dnfield dnfield requested a review from matthew-carroll Aug 6, 2019
@dnfield

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@matthew-carroll - can you take a look at this from the Android embedding perspective? It looks like this only updates the old embedding but not the new.

/// will likely place system UI, such as the keyboard, that fully obscures
/// any content.
/// which the application can render, but where the operating system will
/// consume input gestures for the sake of system navigation.

This comment has been minimized.

Copy link
@matthew-carroll

matthew-carroll Aug 6, 2019

Contributor

It's not clear to me based on this description what the difference is between viewInsets and systemGestureInsets. I'm seeing nearly identical descriptions.

This comment has been minimized.

Copy link
@shihaohong

shihaohong Aug 7, 2019

Author Contributor

I accidentally changed the viewInsets API doc to what I was trying to update systemGestureInsets to. It should be clearer once I've removed this change.

@@ -63,6 +67,10 @@ struct ViewportMetrics {
double physical_view_inset_left = 0;
double physical_view_inset_front = kUnsetDepth;
double physical_view_inset_back = kUnsetDepth;
double physical_system_gesture_inset_top = 0;

This comment has been minimized.

Copy link
@matthew-carroll

matthew-carroll Aug 6, 2019

Contributor

Are there any implications of defining gesture insets in such a central location? I'm guessing that this definition of viewport metrics is used for all platforms, right? If so, would gesture insets ever make sense on Windows, Linux, Mac, and other non-touch surfaces?

This comment has been minimized.

Copy link
@shihaohong

shihaohong Aug 6, 2019

Author Contributor

The main reason I decided to define gesture insets this way is that I could foresee other platforms using system gesture navigations as well. I'll need to see how iOS currently handles this, but I know that swiping up from the bottom of the screen on later iPhones is analogous to pressing the home button on older models of the phone.

@@ -100,6 +100,10 @@
int physicalViewInsetRight = 0;
int physicalViewInsetBottom = 0;
int physicalViewInsetLeft = 0;
int systemGestureInsetTop = 0;

This comment has been minimized.

Copy link
@matthew-carroll

matthew-carroll Aug 6, 2019

Contributor

All changes to this old FlutterView probably need to be made to the new FlutterView, too. Please see io/flutter/embedding/android/FlutterView.java

Also, it would probably be a good idea to introduce a few FlutterView tests around this definition using robolectric.

This comment has been minimized.

Copy link
@shihaohong

shihaohong Aug 8, 2019

Author Contributor

I started exploring the use of roboelectric, but I'm not very familiar with Flutter's android embedding. Do you have any pointers to a high-level overview of how the embedding works and how to get started writing the test suite for a FlutterViewTest.java?

@shihaohong

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Just a heads up that we're currently working on implementing Java tests and the required changes to our unit testing framework, Robolectric, before we can proceed with this PR. See flutter/flutter#38471 for more information.

@shihaohong shihaohong changed the title Pass Android Q Gestural Navigation Insets Pass Android Q insets.systemGestureInsets to Window Aug 15, 2019
@shihaohong

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

After discussion on the state of Robolectric unit testing with Android Q, we've decided to proceed with landing this PR without tests with the intent of adding them once support for Android Q Robolectric testing has been fixed.

I've created a separate issue (flutter/flutter#38626) to track introducing these tests once Robolectric has been upgraded to properly test Android Q.

The PR should be ready for additional review.

cc/ @mklim @matthew-carroll @HansMuller @cbracken

@shihaohong shihaohong removed the request for review from jonahwilliams Aug 15, 2019
@shihaohong shihaohong requested review from matthew-carroll and dnfield Aug 15, 2019
@@ -680,6 +680,24 @@ class Window {
WindowPadding get viewPadding => _viewPadding;
WindowPadding _viewPadding = WindowPadding.zero;

/// The number of physical pixels on each side of the display rectangle into

This comment has been minimized.

Copy link
@matthew-carroll

matthew-carroll Aug 15, 2019

Contributor

Not necessary for this PR, but can we follow up with a diagram of the various paddings and insets and put it on the wiki, or on the website, and then maybe link to it from here?

This comment has been minimized.

Copy link
@shihaohong

shihaohong Aug 15, 2019

Author Contributor

I created an issue (flutter/flutter#38649). some work already exists in the MediaQueryData API docs, we could elaborate on the new insets there and add links to this particular resource once everything has been solidified.

@@ -312,11 +313,21 @@ public final WindowInsets onApplyWindowInsets(@NonNull WindowInsets insets) {
viewportMetrics.viewInsetBottom = insets.getSystemWindowInsetBottom();
viewportMetrics.viewInsetLeft = 0;

if (Build.VERSION.SDK_INT >= 29) {

This comment has been minimized.

Copy link
@matthew-carroll

matthew-carroll Aug 15, 2019

Contributor

Please use the named constant rather than 29. There's another property within Build that provides it. You should be able to find an example somewhere else in the Android embedding if you look.

This comment has been minimized.

Copy link
@shihaohong

shihaohong Aug 15, 2019

Author Contributor

I remember that this caused an issue with the pre-submit checks for a reason I did not really understand. I updated it to use the named constant and will let you know if the problem resurfaces.

This comment has been minimized.

Copy link
@shihaohong

shihaohong Aug 16, 2019

Author Contributor

This has been resolved by suppressing the lint and adding a comment for how the code properly guards for the right SDK version.

@@ -574,6 +579,14 @@ public final WindowInsets onApplyWindowInsets(WindowInsets insets) {
mMetrics.physicalViewInsetBottom =
navigationBarHidden ? calculateBottomKeyboardInset(insets) : insets.getSystemWindowInsetBottom();
mMetrics.physicalViewInsetLeft = 0;

if (Build.VERSION.SDK_INT >= 29) {

This comment has been minimized.

Copy link
@matthew-carroll

matthew-carroll Aug 15, 2019

Contributor

Same comment here about the number 29

@@ -4,6 +4,10 @@

package io.flutter;

import io.flutter.SmokeTest;

This comment has been minimized.

Copy link
@matthew-carroll

matthew-carroll Aug 15, 2019

Contributor

If we're not contributing any tests then we should probably revert this file.

@shihaohong shihaohong requested review from matthew-carroll and mklim Aug 16, 2019
@mklim
mklim approved these changes Aug 16, 2019
Copy link
Member

left a comment

LGTM

@@ -292,10 +294,14 @@ protected void onSizeChanged(int width, int height, int oldWidth, int oldHeight)
*
* This callback is not present in API < 20, which means lower API devices will see
* the wider than expected padding when the status and navigation bars are hidden.
* The annotations to suppress "InlinedApi" and "NewApi" lints prevent lint warnings

This comment has been minimized.

Copy link
@mklim

mklim Aug 16, 2019

Member

really minor nit: This is more of an implementation detail and not really documentation to callers of this method about what it should do. It makes more sense to move it to around the @SuppressLint annotation and comment with // instead of Javadoc (/** */).

@@ -312,11 +318,21 @@ public final WindowInsets onApplyWindowInsets(@NonNull WindowInsets insets) {
viewportMetrics.viewInsetBottom = insets.getSystemWindowInsetBottom();
viewportMetrics.viewInsetLeft = 0;

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {

This comment has been minimized.

Copy link
@mklim

mklim Aug 16, 2019

Member

Do you think it be worth it to still add a test for the non Q case in this PR where these are set to zero? It would just basically be verifying the current behavior and making sure that these lines were guarded and didn't cause problems on lower SDK versions. So some but not a huge amount of value from them.

This comment has been minimized.

Copy link
@shihaohong

shihaohong Aug 16, 2019

Author Contributor

I don't know the root cause of the problem, but I believe that because the TargetApi is set to 20 and the tests only use up to SDK 16 at runtime, the onApplyWindowInsets method gets completely ignored by the test framework (I tried putting print statements in the method and they weren't being called). So, even this simple case might be blocked on upgrading Robolectric :\

This comment has been minimized.

Copy link
@mklim

mklim Aug 16, 2019

Member

Ah, right. The entire method here requires at least API 20, and our setup only includes API 16 right now.

If you wanted to test just the non-Q parts of this method, you should be able to bring in API 21 without any trouble. Robolectric has supported it since 3.0 (we're on 3.8). It's 29/Q specifically that required upgrading to 4.3 and is currently blocked. But doing this has some extra overhead where it would require bringing in another robolectric android package for the right SDK (like robolectric.android-all:5.0.0_r2-robolectric-0) and adding it to CIPD as described here. So at that point I'm not sure if it's worth the hassle. There may be some unknown unknowns that cause it to take extra time.

mMetrics.physicalHeight, mMetrics.physicalPaddingTop, mMetrics.physicalPaddingRight,
mMetrics.physicalPaddingBottom, mMetrics.physicalPaddingLeft, mMetrics.physicalViewInsetTop,
mMetrics.physicalViewInsetRight, mMetrics.physicalViewInsetBottom, mMetrics.physicalViewInsetLeft);
mNativeView.getFlutterJNI().setViewportMetrics(

This comment has been minimized.

Copy link
@mklim

mklim Aug 16, 2019

Member

This is an aside that's out of scope for this PR, but reading this all of these methods that take 20 identical primitives in a very specific order and then handle them in subtle ways seem a little risky. It would be super trivial to accidentally get the order wrong in one of the many places these are called. In Dart this can be sort of addressed trivially with named params, but in Java/C++ there aren't really great options. Could define a struct/class that has these as named members and then pass that around, but that's probably overkill.

This comment has been minimized.

Copy link
@shihaohong

shihaohong Aug 16, 2019

Author Contributor

Right, this crossed my mind while making this change. I had at times passed in the wrong number of params and had to rebuild because there were so many of them and they were all identical. Coming up with a cleaner way to handle this will probably help, especially if we see a world where the number of params continue to grow.

@shihaohong shihaohong merged commit 014ab76 into flutter:master Aug 16, 2019
13 checks passed
13 checks passed
WIP Ready for review
Details
build_and_test_android_unopt_debug Task Summary
Details
build_and_test_android_unopt_debug
Details
build_and_test_linux_unopt_debug Task Summary
Details
build_and_test_linux_unopt_debug
Details
build_windows_opt_debug Task Summary
Details
build_windows_opt_debug
Details
build_windows_unopt_debug Task Summary
Details
build_windows_unopt_debug
Details
cla/google All necessary CLAs are signed
format_and_dart_test Task Summary
Details
format_and_dart_test
Details
luci-engine
Details
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 16, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Aug 17, 2019
git@github.com:flutter/engine.git/compare/e5f9132b347c...4d5c38e215e9

git log e5f9132b347c..4d5c38e215e9 --no-merges --oneline
2019-08-16 bkonyi@google.com Roll src/third_party/dart a3b579d5c3..2a3b844b41 (5 commits) (flutter/engine#11060)
2019-08-16 skia-flutter-autoroll@skia.org Roll src/third_party/skia df54f37a5dc1..237a95fe7b28 (2 commits) (flutter/engine#11058)
2019-08-16 chinmaygarde@google.com Roll buildroot to 5a33d6ab to pickup changes to toolchain version tracking. (flutter/engine#11061)
2019-08-16 jason-simmons@users.noreply.github.com Sort the Skia typefaces in a font style set into a consistent order (flutter/engine#11056)
2019-08-16 50503328+flar@users.noreply.github.com Add ccls config files to .gitignore (flutter/engine#11046)
2019-08-16 shihaohong@google.com Pass Android Q insets.systemGestureInsets to Window (flutter/engine#10413)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff (stuartmorgan@google.com), and stop
the roller if necessary.
@HansMuller HansMuller referenced this pull request Sep 25, 2019
15 of 15 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.