Skip to content

Add DisplayFeatures and DisplayCutout to viewport metrics#1

Open
andreidiaconu wants to merge 31 commits intomasterfrom
foldable_support
Open

Add DisplayFeatures and DisplayCutout to viewport metrics#1
andreidiaconu wants to merge 31 commits intomasterfrom
foldable_support

Conversation

@andreidiaconu
Copy link
Owner

@andreidiaconu andreidiaconu commented Jan 18, 2021

This PR is opened for early feedback on foldable support

tggmbi
tggmbi suggested changes Feb 3, 2021
View reviewed changes
Copy link

@tggmbi tggmbi left a comment

Choose a reason for hiding this comment

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

at this stage, this should be based on alpha 02 of androidx.window:window
any reason you are using 01?

@andreidiaconu
Copy link
Owner Author

andreidiaconu commented Feb 3, 2021 via email

@andreidiaconu
Copy link
Owner Author

andreidiaconu commented Feb 3, 2021 via email

GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
GaryQian
GaryQian reviewed Feb 3, 2021
View reviewed changes
@GaryQian
Copy link

GaryQian commented Feb 3, 2021

This also needs tests!

@andreidiaconu
Copy link
Owner Author

andreidiaconu commented Feb 4, 2021

@GaryQian Thank you for the thorough review. Just wanted to mention that the TODOs and lack of tests were next on my list and not something intended to stay.

xster
xster reviewed Feb 8, 2021
View reviewed changes
Copy link

@xster xster left a comment

Choose a reason for hiding this comment

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

Looks like a great start! Mostly small nit comments. One question around the initialization timing.

shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java
* We do not know this type of display feature yet. This can happen if WindowManager is updated
* with new types.
*/
UNKNOWN(0),
Copy link

@xster xster Feb 8, 2021

Choose a reason for hiding this comment

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

Or this could happen when the window is not yet attached when setWindowManagerDisplayFeatures is called via onLayout right?

I wonder if we should delay the sending of the very first viewport metrics until a window is attached. Worth testing whether during initialization, we get a quick succession of { window size XY }, then very quickly after, { window size XY, hinge at rect Z }, causing the Flutter application listening to the mediaquery to rapidly build 2 different layouts (wasting the first one).

Copy link
Owner Author

@andreidiaconu andreidiaconu Feb 9, 2021

Choose a reason for hiding this comment

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

Will test and see. If there is a delay, I don't think it's large enough to warrant both calls.

@andreidiaconu
Copy link
Owner Author

andreidiaconu commented Feb 9, 2021

Looks like a great start! Mostly small nit comments. One question around the initialization timing.

@xster I would say they are more than nit comments - Documentation can be a lot better. Thank you for taking the time.

goderbauer
goderbauer reviewed Feb 17, 2021
View reviewed changes
lib/ui/platform_dispatcher.dart
final DisplayFeatureType type;

/// Posture of display feature, which is populated only for folds and hinges.
/// For cutouts, this is [DisplayFeatureState.unknown]
Copy link

@goderbauer goderbauer Feb 17, 2021

Choose a reason for hiding this comment

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

To get better doc linking (here and elsewhere):

Suggested change
/// For cutouts, this is [DisplayFeatureState.unknown]
/// For [DisplayFeatureType.cutout]s, this is [DisplayFeatureState.unknown].

Copy link

@goderbauer goderbauer Feb 17, 2021

Choose a reason for hiding this comment

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

Also seems odd that it is "unknown" for cutouts. Isn't it more like: "This doesn't make sense for cutouts"?

Copy link
Owner Author

@andreidiaconu andreidiaconu Feb 18, 2021

Choose a reason for hiding this comment

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

Also seems odd that it is "unknown" for cutouts. Isn't it more like: "This doesn't make sense for cutouts"?

WindowManager display features and cutouts are very similar in a lot of aspects, but indeed, this property is not used for cutouts yet. This was the simplest way to still have the posture for "fold" and "hinge".

@xster
Copy link

xster commented Mar 5, 2021

@andreidiaconu is this PR ready for another round of reviews?

@andreidiaconu
Copy link
Owner Author

andreidiaconu commented Mar 5, 2021

@xster Yes, with the mention that I've opened an official PR on the engine repo: flutter#24756

Also, tests are still in progress, my apologies for taking longer with them.

andreidiaconu pushed a commit that referenced this pull request Jun 23, 2021
@cbracken
Revert "Sped up the objc standard message codec (flutter#25998)" (flu…
08d9bc0 
…tter#26117)

This appears to have triggered reproducible failures in channels_integration_test_ios:

    [   +4 ms] 00:01 [32m+0[0m: channel suite step through[0m
    [+3744 ms] Unsupported value: Sun Mar 11 07:16:42 2018 of type __NSTaggedDate
    [        ] *** Assertion failure in void WriteValue(CFMutableDataRef, id)(), FlutterStandardCodec.mm:340
    [   +2 ms] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Unsupported value for standard codec.'
    [        ] *** First throw call stack:
    [        ] (0x19d5bd9d8 0x1b1940b54 0x19d4cc50c 0x19e815238 0x1050031ec 0x104823f80 0x105003aac 0x1050009bc 0x104824e9c 0x105000b4c 0x104d0cc98 0x10501b398 0x104fb3c94 0x104fb72c4 0x19d53e3e0 0x19d53dfe4 0x19d53d4c4 0x19d537850 0x19d536ba0 0x1b429c598 0x19fe282f4 0x19fe2d874 0x1048257fc 0x19d215568)
    [        ] libc++abi.dylib: terminating with uncaught exception of type NSException
    [  +65 ms] Process 541 stopped
    [        ] * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
    [        ]     frame #0: 0x00000001c93e584c libsystem_kernel.dylib`__pthread_kill + 8
    [        ] libsystem_kernel.dylib`__pthread_kill:
    [        ] -> 0x1c93e584c <+8>:  b.lo   0x1c93e5868               ; <+36>
    [        ]    0x1c93e5850 <+12>: stp    x29, x30, [sp, #-0x10]!
    [        ]    0x1c93e5854 <+16>: mov    x29, sp
    [        ]    0x1c93e5858 <+20>: bl     0x1c93c2f5c               ; cerror_nocancel
    [        ] Target 0: (Runner) stopped.

Example builds:
* https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/828/overview
* https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/829/overview
* https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/830/overview
* https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/831/overview

Example Log:
* https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8847377106855056784/+/u/run_channels_integration_test_ios/stdout

This reverts commit 99021da.
@andreidiaconu
Update androidx window to beta01
de52872
@andreidiaconu
Remove unused import
3dc358c
@andreidiaconu
Merge branch 'master' of github.com:flutter/engine into foldable_support
3b58160 
# Conflicts:
#	lib/ui/fixtures/ui_test.dart
#	lib/ui/hooks.dart
#	lib/ui/platform_dispatcher.dart
#	lib/ui/window/viewport_metrics.cc
#	lib/ui/window/viewport_metrics.h
#	lib/ui/window/window.cc
#	lib/web_ui/lib/src/ui/platform_dispatcher.dart
#	shell/common/shell_test.cc
#	shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java
#	shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java
#	shell/platform/android/io/flutter/view/FlutterView.java
#	shell/platform/android/platform_view_android_jni_impl.cc
#	shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java
#	shell/platform/fuchsia/flutter/platform_view.cc
#	testing/scenario_app/android/app/build.gradle
@andreidiaconu
Remove window_hook_integration_test.dart
5452a01
@andreidiaconu
Fix tests
0e6fe2a
@andreidiaconu
Merge branch 'master' of github.com:flutter/engine into foldable_support
3174f12 
# Conflicts:
#	shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java
@andreidiaconu
Update android_embedding_dependencies version to include androidx.window
888ee95
@andreidiaconu
Add androidx.window to test_runner build
d22920e
@andreidiaconu
Add empty display features arrays to fuchsia platform view
3fd0699
@andreidiaconu
Fix tests using final class mocking
58d3eb2
@andreidiaconu
Add missing license header
e12f54a
@andreidiaconu
Update licenses_golden
afce662
@andreidiaconu
Merge branch 'master' of github.com:flutter/engine into foldable_support
89fe749 
# Conflicts:
#	shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers

@xster xster xster left review comments

@goderbauer goderbauer goderbauer left review comments

@GaryQian GaryQian GaryQian left review comments

@tggmbi tggmbi tggmbi requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@andreidiaconu @GaryQian @xster @tggmbi @goderbauer