Android 10+ View.setSystemGestureExclusionRects #11441

shihaohong · 2019-08-26T17:11:31Z

Description

Adds a platform channel that allows the Flutter framework to set system gesture exclusion rects for Android Q+.

The corresponding framework branch used to test this behavior is https://github.com/shihaohong/flutter/tree/set-exclusion-rects.

Demo

Demo (exclude top-left corner of app):

Sample code:

                SystemGestureExclusionRects.setSystemGestureExclusionRects([
                  {
                    "top": 0,
                    "right": 500,
                    "bottom": 500,
                    "left": 0,
                  },
                ]);

Related Issues

Related to flutter/flutter#32748

Testing

Testing is blocked for Android SDK Q+ (29+). Will track for follow up updates to unit tests on flutter/flutter#38626

…-rects

mklim · 2019-08-26T20:53:18Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformChannel.java

            result.success(null);
            break;
+          case "SystemGestures.setSystemGestureExclusionRects":
+            if (arguments instanceof JSONArray) {


subjective, optional: Consider early returning here with a guard clause instead to reduce nesting.

https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html

mklim · 2019-08-26T20:57:13Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformChannel.java

+              JSONArray inputRects = (JSONArray) arguments;
+              ArrayList<Rect> decodedRects = new ArrayList<Rect>();
+              for (int i = 0; i < inputRects.length(); i++) {
+                JSONObject rect = inputRects.getJSONObject(i);


subjective, optional: pretty much every other method here has a decodeFoo helper method for parsing the JSON arguments, I think that would probably also make sense here and make it a little easier to read.

Quick question about this, I originally attempted to do that, but ran into the issue where I would to do a try...catch block in the helper method as well to catch JSONExceptions. In the event that happens, do I simple return null and handle it in the original function to send a result.error? (see my previous commit)

I'd declare the helper method as throws JSONException instead of trying to have it handle the exceptions itself. Then I'd catch the exceptions and convert them to result.error in the top level here. That seems like a better separation of concerns to me because this is the level where we handle result and passing the output into it normally, so it makes sense to me to catch any parsing exceptions and convert it into an error for result here. decodeSystemUiOverlays and decodeAppSwitcherDescription are doing this already too.

Please disregard my original comment. It compiles just fine with the decodeRects helper method. I think I originally had a nested try/catch block and it was complaining about that instead.

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformChannel.java

mklim · 2019-08-26T20:58:14Z

shell/platform/android/io/flutter/plugin/platform/PlatformPlugin.java

    }
+
+    private void setSystemGestureExclusionRects(ArrayList<Rect> rects) {
+        if (Build.VERSION.SDK_INT >= 29) {


optional, subjective: This could also be a guard clause.

mklim · 2019-08-26T20:59:15Z

shell/platform/android/io/flutter/plugin/platform/PlatformPlugin.java

        clipboard.setPrimaryClip(clip);
    }
+
+    private void setSystemGestureExclusionRects(ArrayList<Rect> rects) {


I think the rest of the PR besides this method could be tested, even with the SDK limitations. Even though this is never called, I think you could check that the arguments are parsed safely in PlatformChannel. What do you think?

That makes sense! I'll give it a shot

…ests

PieterAelse · 2019-08-27T12:57:52Z

There's a limit of 200dp for exclusion height: https://android-developers.googleblog.com/2019/08/final-beta-update-official-android-q.html

We've made further refinements to Gesture Navigation in Beta 6 based on user feedback. First, to ensure reliable and consistent operation, there's a 200dp vertical app exclusion limit for the Back gesture.

Will there be anything on the Flutter side for this? I don't think Google is throwing an exception or anything, but the least thing possible is mention it in the Flutter method documentation.
Also, as far as I can see the 500 from the example is pixels and not dp?

shihaohong · 2019-08-27T15:33:59Z

Hi @PieterAelse, I'm aware of both points that there's a limit of 200dp and that my example uses pixels instead of dp. I will document the limit and also include the device pixel ratio as an argument when I create the framework-side PR

…latform channel

mklim

LGTM. Thanks for adding tests!

mklim · 2019-08-29T20:02:42Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformChannel.java

+  @VisibleForTesting
+  protected class PlatformMethodCallHandler implements MethodChannel.MethodCallHandler {
+    @Override
+    public void onMethodCall(@NonNull MethodCall call, @NonNull MethodChannel.Result result) {


I wish the diff were better here. I spot checked it, assuming that nothing in this was changed from the original besides the new method call.

mklim · 2019-08-29T20:03:17Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformChannel.java

+              break;
+            }
+
+            Log.v("Got here", arguments.toString());


This log probably isn't useful and should be removed, even at the verbose level.

This was an artifact from when I was testing my tests. I'll remove it!

mklim · 2019-08-29T20:03:58Z

shell/platform/android/test/README.md

   This makes sure the test is actually executed at run time.
 4. Write your test.
-5. Build and run with `testing/run_tests.py [--type=java] [filter=<test_class_name>]`.
+5. Build and run with `testing/run_tests.py [--type=java] [--java-filter=<test_class_name>]`.


Thanks for updating this!

matthew-carroll · 2019-08-29T21:07:49Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformChannel.java

  @Nullable
  private PlatformMessageHandler platformMessageHandler;
-
-  private final MethodChannel.MethodCallHandler parsingMethodCallHandler = new MethodChannel.MethodCallHandler() {


@shihaohong why was this moved?

matthew-carroll · 2019-08-29T21:09:37Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformChannel.java

+   * decodes incoming arguments, if needed, into a format that is necessary for the API.
+   */
+  @VisibleForTesting
+  protected class PlatformMethodCallHandler implements MethodChannel.MethodCallHandler {


I'd like for us to avoid non-static inner classes. I think in this case I'd like for us to revert the movement of this handler definition back to where it was. But in future cases where we do define inner classes, they should be static to avoid the possibility of accidentally leaking references to the outer class.

I had a discussion with @mklim and we felt that it was difficult to read an inline class definition within another class. I'll revert this in a subsequent PR

matthew-carroll · 2019-08-29T21:11:21Z

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java

+@RunWith(RobolectricTestRunner.class)
+public class PlatformChannelTest {
+    @Test
+    public void setSystemExclusionRectsSendsSuccessMessageToFramework() throws JSONException {


Please name all tests as itDoesSomethingWhenSomethingHappens or as close to that as possible. This helps ensure that we writing unit tests here instead of integration tests.

matthew-carroll · 2019-08-29T21:12:05Z

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java

+        );
+    }
+
+    private class ResultsMock implements Result {


Please make all inner classes static to avoid hidden references that can cause leaks and complications.

matthew-carroll · 2019-08-29T21:13:02Z

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java

+public class PlatformChannelTest {
+    @Test
+    public void setSystemExclusionRectsSendsSuccessMessageToFramework() throws JSONException {
+        DartExecutor dartExecutor = mock(DartExecutor.class);


Please denote with comments in each test the part that is setup vs execution vs verification so that developers can more easily check the part they're interested in.

matthew-carroll · 2019-08-29T21:14:20Z

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java

+        int bottom = 250;
+        int left = 0;
+
+        ResultsMock resultsMock = mock(ResultsMock.class);


Can't you just mock(Result.class) instead of introducing ResultsMock? Or does Mockito not allow that?

matthew-carroll

I just got back today from a trip so I couldn't review this before merging, but I added a few comments with requests for change.

shihaohong · 2019-08-29T21:40:37Z

@matthew-carroll How should I address your review suggestions? Should I make a separate PR addressing them?

matthew-carroll · 2019-08-29T21:41:41Z

Yeah, I think that would be fine.

…ne#11441)

[email protected]:flutter/engine.git/compare/9a17d8e45197...101e03c git log 9a17d8e..101e03c --no-merges --oneline 2019-08-29 [email protected] Return a JSON value for the Skia channel (flutter/engine#11717) 2019-08-29 [email protected] Android 10+ View.setSystemGestureExclusionRects (flutter/engine#11441) 2019-08-29 [email protected] Roll src/third_party/dart 35382f9b14..05c28c6115 (flutter/engine#11702) 2019-08-29 [email protected] Roll src/third_party/skia ca8b07cf8a59..3783375c4d41 (2 commits) (flutter/engine#11712) 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] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

…ne#11441)

* 37c5510 Roll src/third_party/skia ca8b07cf8a59..3783375c4d41 (2 commits) (flutter/engine#11712) * 74a3c73 Roll src/third_party/dart 35382f9b14..05c28c6115 (flutter/engine#11702) * 7024722 Android 10+ View.setSystemGestureExclusionRects (flutter/engine#11441) * 101e03c Return a JSON value for the Skia channel (flutter/engine#11717) * dce8a34 Revert "Notify framework to clear input connection when app is backgrounded (#35054) (#9498)" (flutter/engine#11720) * 1b1f781 Quote the font family name whenever setting the font-family property. (flutter/engine#11722) * 260f88d Roll src/third_party/skia 3783375c4d41..165ca3f85b7a (8 commits) (flutter/engine#11725) * e620012 Roll src/third_party/dart 05c28c6115..1573f4e877 (14 commits) * 81e010f last flutter web sync: cc38319841 (flutter/engine#11732) * 1235439 Roll src/third_party/dart 1573f4e877..fd48ea3432 (16 commits) * cf5d2a6 Add wasm to sky_engine (flutter/engine#11736) * 118b5f3 Roll src/third_party/dart fd48ea3432..a873bc5db3 (7 commits) * a7d59aa Roll src/third_party/skia 165ca3f85b7a..ba0a2c7ad992 (1 commits) (flutter/engine#11745) * 8401ce9 Roll src/third_party/dart a873bc5db3..35df96a2e2 (1 commits) * ff3dffa Roll src/third_party/skia ba0a2c7ad992..7409b73fa546 (3 commits) (flutter/engine#11756) * 70626c0 Roll src/third_party/skia 7409b73fa546..575699569e91 (1 commits) (flutter/engine#11766) * 292d844 Roll src/third_party/dart 35df96a2e2..d4342b9021 (4 commits)

…from #11441 (#11804) * Improve variable naming and javadoc for setSystemGestureExclusionRects * Remove variables from setSystemGestureExclusionRects tests * Split test for two behaviors into two separate tests for setSystemGestureExclusionRects success case

shihaohong added 5 commits August 23, 2019 16:04

(WIP) setting up setSystemGestureExclusionRects platform channel

7b7fa25

Merge branch 'master' of github.com:flutter/engine into set-exclusion…

361eb40

…-rects

SystemGesture.setSystemGestureExclusionRects

5245ad3

Cleanup Log.v and unnecessary imports

b37fa16

Add Javadoc

c155aec

shihaohong added the platform-android label Aug 26, 2019

shihaohong requested review from matthew-carroll and mklim August 26, 2019 17:11

googlebot added the cla: yes label Aug 26, 2019

Reinclude accidentally removed log

53e858b

mklim reviewed Aug 26, 2019

View reviewed changes

shihaohong added 4 commits August 26, 2019 14:36

Use guard clauses, refactor decodeRects to its own function

8dc8646

Add missing return statement to guard clause

e4dac7c

Replace return with break statement

3df584b

(WIP) - adding PlatformChannelTest.java systemGestureExclusionRects t…

676192e

…ests

(WIP) - PlatformChannel exploration

21577fb

shihaohong added the Work in progress (WIP) Not ready (yet) for review! label Aug 27, 2019

shihaohong added 4 commits August 29, 2019 11:35

(WIP) - Added first happy path test for systemGestureExclusionRects p…

87ddf16

…latform channel

Added unit tests for setSystemGestureInsets platform channel

13617ee

Code clean-up

cdd4e6a

Add docs

773a7e9

shihaohong requested a review from mklim August 29, 2019 19:58

shihaohong removed the Work in progress (WIP) Not ready (yet) for review! label Aug 29, 2019

Fix android/BUILD.gn ordering

2dca160

mklim approved these changes Aug 29, 2019

View reviewed changes

shihaohong added 2 commits August 29, 2019 13:24

Remove remnant Log.v calls

57a3bf5

Fix test naming in PlatformChannelTest.java

fc972ba

shihaohong merged commit 7024722 into flutter:master Aug 29, 2019

shihaohong deleted the set-exclusion-rects branch August 29, 2019 21:00

matthew-carroll reviewed Aug 29, 2019

View reviewed changes

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

7024722 Android 10+ View.setSystemGestureExclusionRects (flutter/engi…

60c61ca

…ne#11441)

This was referenced Aug 29, 2019

Roll engine 9a17d8e45197..101e03c1c841 (4 commits) flutter/flutter#39529

Closed

Roll engine 9a17d8e45197..101e03c1c841 (4 commits) flutter/flutter#39534

Merged

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

7024722 Android 10+ View.setSystemGestureExclusionRects (flutter/engi…

ff3412f

…ne#11441)

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

7024722 Android 10+ View.setSystemGestureExclusionRects (flutter/engi…

4bb71f4

…ne#11441)

engine-flutter-autoroll mentioned this pull request Aug 30, 2019

Roll engine 9a17d8e45197..1235439830ae (10 commits) flutter/flutter#39551

Closed

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

7024722 Android 10+ View.setSystemGestureExclusionRects (flutter/engi…

e3188a1

…ne#11441)

engine-flutter-autoroll mentioned this pull request Aug 30, 2019

Roll engine 9a17d8e45197..1235439830ae (10 commits) flutter/flutter#39552

Closed

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

7024722 Android 10+ View.setSystemGestureExclusionRects (flutter/engi…

8399894

…ne#11441)

engine-flutter-autoroll mentioned this pull request Aug 30, 2019

Roll engine 9a17d8e45197..292d844de357 (17 commits) flutter/flutter#39578

Merged

This was referenced Aug 31, 2019

Incorporate View.setSystemGestureExclusionRects code review feedback from #11441 #11804

Merged

Document Android embedding testing approaches flutter/flutter#39604

Closed

HansMuller mentioned this pull request Sep 25, 2019

Planned Material Library Features Jul-Sept 2019 flutter/flutter#35088

Closed

15 tasks

shihaohong mentioned this pull request Apr 9, 2020

Revert getSystemGestureExclusionRects and setSystemGestureExclusionRects #17613

Merged

vanlooverenkoen mentioned this pull request Feb 23, 2021

Android swipe gesture support icapps/flutter-template#157

Open

Android 10+ View.setSystemGestureExclusionRects #11441

Android 10+ View.setSystemGestureExclusionRects #11441

Uh oh!

Conversation

shihaohong commented Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Demo

Related Issues

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shihaohong Aug 26, 2019 • 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!

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!

PieterAelse commented Aug 27, 2019

Uh oh!

shihaohong commented Aug 27, 2019

Uh oh!

mklim left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthew-carroll left a comment

Choose a reason for hiding this comment

Uh oh!

shihaohong commented Aug 29, 2019

Uh oh!

matthew-carroll commented Aug 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shihaohong commented Aug 26, 2019 •

edited

Loading

shihaohong Aug 26, 2019 •

edited

Loading