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

[dart:ui] Introduce PlatformDispatcher.implicitView #39553

Merged
merged 2 commits into from
Feb 17, 2023

Conversation

loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented Feb 11, 2023

This introduces PlatformDispatcher.implicitView (see flutter/flutter#120306).

This is a low-level primitive for the framework's bootstrapping. Most code, including the framework after bootstrapping, will use View.of(context) instead of this new API. This new primitive will let us deprecate the window global.

Goals:

  1. Enable multi-window. The PlatformDispatcher.implicitView is nullable. If null, the app must create a window to get a view it can draw into.
  2. Backwards compatibility. For "single window" apps, PlatformDispatcher.instance.implicitView should behave as similar to window as possible.
    1. The PlatformDispatcher.instance.implicitView.viewId should be 0.
    2. The PlatformDispatcher.instance.implicitView must be available synchronously at root isolate startup. This allows the framework to determine if it can make single window assumptions at startup.
    3. The PlatformDispatcher.instance.implicitView reference must not change after startup: if it is null at startup, it must always be null; if it is non-null at startup, it must always be non-null. If "single window" app enters headless mode, the implicit view must remain non-null.

In the future, the embedder will control whether an implicit view is created: mobile & legacy desktop apps will have an implicit view, multi-window desktop apps won't have an implicit view. This requires updating the engine's embedder API and is out-of-scope for this change. For now, all apps will have an implicit view.

Part of flutter/flutter#120306

Manual test

Manual test plan...

Modify: https://github.com/flutter/flutter/blob/master/examples/layers/raw/hello_world.dart

diff --git a/examples/layers/raw/hello_world.dart b/examples/layers/raw/hello_world.dart
index 20a53b0c7c..0940ebc49f 100644
--- a/examples/layers/raw/hello_world.dart
+++ b/examples/layers/raw/hello_world.dart
@@ -8,8 +8,9 @@
 import 'dart:ui' as ui;

 void beginFrame(Duration timeStamp) {
-  final double devicePixelRatio = ui.window.devicePixelRatio;
-  final ui.Size logicalSize = ui.window.physicalSize / devicePixelRatio;
+  ui.FlutterView? window = ui.PlatformDispatcher.instance.implicitView;
+  final double devicePixelRatio = window!.devicePixelRatio;
+  final ui.Size logicalSize = window!.physicalSize / devicePixelRatio;

   final ui.ParagraphBuilder paragraphBuilder = ui.ParagraphBuilder(
     ui.ParagraphStyle(textDirection: ui.TextDirection.ltr),
@@ -35,7 +36,7 @@ void beginFrame(Duration timeStamp) {
     ..addPicture(ui.Offset.zero, picture)
     ..pop();

-  ui.window.render(sceneBuilder.build());
+  window!.render(sceneBuilder.build());
 }

 // This function is the primary entry point to your application. The engine

✅ Verify that Windows works as expected:

flutter run .\raw\hello_world.dart -d windows --local-engine host_debug_unopt

✅ Verify that Web works as expected:

flutter run .\raw\hello_world.dart --local-engine host_debug_unopt -d chrome

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 Hixie said 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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added embedder Related to the embedder API platform-web Code specifically for the web engine labels Feb 11, 2023
@loic-sharma loic-sharma force-pushed the dart-ui-implicit-view branch from 2d84ee5 to 054ec84 Compare February 14, 2023 21:35
@loic-sharma loic-sharma marked this pull request as ready for review February 14, 2023 22:51
@ditman
Copy link
Member

ditman commented Feb 14, 2023

web_ui bits look good! Thanks!

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

///
/// While the [FlutterView]'s properties may change, this reference is
/// guaranteed to never change.
///
Copy link
Member

Choose a reason for hiding this comment

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

Should we add some kind of a warning here that applications and packages should not rely on this property being set and instead acquire the view via other means (e.g. View.of(context) is appropriate for most use cases)? This property is basically just interesting for low-level app bootstrapping.

Copy link
Member

Choose a reason for hiding this comment

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

(I guess you're sort of hinting at this with the "see also" section)

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you take another look? I updated this comment to push people towards View.of instead. The new comment is less clear as to when an implicit view is provided by the platform though...

Copy link
Member

Choose a reason for hiding this comment

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

I took a stab below at how I would write the documentation for this property. Feel free to copy it as a whole or bits and pieces that you like or ignore it completely.


The implicit [FlutterView] provided by the platform, if any.

If the platform provides an implicit view, it can be used to bootstrap the framework. This is common for platforms designed for single-view applications (e.g. on mobile devices with a single display).

Applications and libraries must not rely on this property being set as it may be null depending on the engine's configuration. Instead, consider using [View.of] to lookup the [FlutterView] the current [BuildContext] is drawing into.

While the properties on the referenced [FlutterView] may change, the reference itself is guaranteed to never change over the lifetime of the application: If this property is null at startup, it will remain so throughout the entire lifetime of the application. If it points to a specific [FlutterView], it will continue to point to the same view until the application is shut down (although the engine may replace or remove the underlying backing surface of the view at its discretion).

See also:

  • [PlatformDisptacher.views] for a list of all [FlutterView]s provided by the platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's perfect, will update!

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated! FYI, I added [View.of] to the "See also" section. It's a little repetitive, but I want to nudge folks towards View.of as much as possible :)

Copy link
Member

Choose a reason for hiding this comment

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

Added just a couple more suggestions above but this is looking pretty great!

FlutterView? get implicitView => _implicitViewEnabled() ? _views[0] : null;

@Native<Handle Function()>(symbol: 'PlatformConfigurationNativeApi::ImplicitViewEnabled')
external static bool _implicitViewEnabled();
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if this maybe should just return the implicit view ID (or null) to avoid that we have to hard-code the magic number here and in platform_configuration.cc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had this approach originally but I switched to a boolean instead as we don't have any scenarios where the implicit view will have a non-zero ID. I don't feel strongly either ways though, so let me know if you have a preference.

Copy link
Member

Choose a reason for hiding this comment

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

No preference, really. It's all private and internal anyways, so easy to change if it has to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I'll leave this open so others can chime in :)

lib/ui/platform_dispatcher.dart Outdated Show resolved Hide resolved
shell/common/engine.cc Show resolved Hide resolved
Copy link
Contributor

@yaakovschectman yaakovschectman left a comment

Choose a reason for hiding this comment

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

LGTM with one question

/// See also:
///
/// * [View.of], for accessing the current view.
FlutterView? get implicitView => _implicitViewEnabled() ? _views[0] : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will/should this always return the 0th view even in multi-window cases?

Copy link
Member Author

@loic-sharma loic-sharma Feb 15, 2023

Choose a reason for hiding this comment

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

Yup, if there is an implicit view it is guaranteed to have view ID 0. Note that this is a map and not a list. Furthermore, the implicit view & underlying view exist even if the app transitions to headless mode - this matches today's window behavior.

"Modern" desktop apps won't have an implicit view. These are apps that use the new runner templates that don't exist yet. For these future apps, view ID 0 won't have any special meaning.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

Overall looks good - just a few C++ and doc nits.

@loic-sharma loic-sharma force-pushed the dart-ui-implicit-view branch from 9d95b31 to a3326dc Compare February 15, 2023 22:51
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@@ -45,8 +45,15 @@ void executableNameNotNull() {
notifyStringValue(Platform.executable);
}

@pragma('vm:entry-point')
void implicitViewNotNull() {
notifyBoolValue(PlatformDispatcher.instance.implicitView != null);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this test do? To ensure that PlatformDispatcher.instance.implicitView isn't null? But this is only because we don't have an option to control it right? Should we add a TODO or at least comment to explain that this test might (should) be broken once we allow non-implicitView?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a fixture to check whether the implicit view is non-null. The real test is EmbedderTest.ImplicitViewNotNull.

But this is only because we don't have an option to control it right?

Yup that's correct.

Should we add a TODO or at least comment to explain that this test might (should) be broken once we allow non-implicitView?

Good idea, added a TODO to EmbedderTest.ImplicitViewNotNull.

@@ -41,6 +41,12 @@ void testMain() {
await window.resetHistory();
});

test('EnginePlatformDispatcher.instance.implicitView should be non-null', () async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

Copy link
Member Author

@loic-sharma loic-sharma Feb 16, 2023

Choose a reason for hiding this comment

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

From my understanding, only desktop platforms will be able to create windows (and therefore views). All other platforms - mobile and web - will have an implicit view instead. In other words, this test will be correct for the foreseeable future.

Side note: in theory we could allow all embedders to opt-out of the implicit view for add-to-app scenarios. We don't have any concrete plans there though... /cc @goderbauer

Copy link
Contributor

Choose a reason for hiding this comment

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

All other platforms - mobile and web - will have an implicit view instead. In other words, this test will be correct for the foreseeable future.

I think we'd rather not assume that (and we don't have to, right?). The user might very possibly attach multiple views from the native code, and the framework will handle them. And with that, the user might even choose not to use the implicit view.

Copy link
Member

Choose a reason for hiding this comment

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

All other platforms - mobile and web - will have an implicit view instead.

Web will (possibly) want to render to multiple different target DIVs at some point, so I guess we're going to have multiple "views" too?

Copy link
Member Author

@loic-sharma loic-sharma Feb 16, 2023

Choose a reason for hiding this comment

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

All other platforms - mobile and web - will have an implicit view instead.

Web will (possibly) want to render to multiple different target DIVs at some point, so I guess we're going to have multiple "views" too?

To clarify there's two semi-related things here:

  1. Multi-view: all platforms, including web, will have multi-view support from the framework side
  2. Implicit view: this replaces the window global

Having an implicit view does not block multi-view. It's just the replacement for the window global. For now, we don't have plans to move web off the implicit view as that's not required for supporting multi-window on desktop.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification @loic-sharma (expect a quick call from me soon-ish though, I want to start redoing stuff in the web engine so the multi-view lands more easily!)

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

@loic-sharma loic-sharma force-pushed the dart-ui-implicit-view branch from d0a9927 to 1e1d41a Compare February 16, 2023 21:40
@loic-sharma loic-sharma merged commit 946b291 into flutter:main Feb 17, 2023
@loic-sharma loic-sharma deleted the dart-ui-implicit-view branch February 17, 2023 00:32
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 17, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 17, 2023
sourcegraph-bot pushed a commit to sgtest/megarepo that referenced this pull request Feb 17, 2023
…ew` (flutter/engine#39553) (#120939)

Commit: e29a79975375f98118b256cd75b5a260a04bfe17
auto-submit bot pushed a commit to flutter/plugins that referenced this pull request Feb 17, 2023
* f85438bfa c8b1d2ffa Roll Fuchsia Mac SDK from YpQKlqmyn8r_snx06... to xl9Y8o-9FDyvPogki... (flutter/engine#39675) (flutter/flutter#120887)

* 174a562a6 d699b4a Roll Flutter from e3471f0 to df41e58f6f4e (83 revisions) (#7184) (flutter/flutter#120888)

* 170539f83 Roll Flutter Engine from c8b1d2ffaec8 to 0d8d93306822 (2 revisions) (flutter/flutter#120891)

* df98689c9 2be7253c9 Roll Fuchsia Linux SDK from q7u2WyX2BSRBIzyTW... to yT4JLKTCWWwbRwB0l... (flutter/engine#39679) (flutter/flutter#120898)

* cacef57b6 [flutter_tools] Skip over "Resolving dependencies..." text in integration tests (flutter/flutter#120077)

* 34102ca3b Migrate channels to pkg:integration _test (flutter/flutter#120833)

* df13ea248 Roll Flutter Engine from 2be7253c9b10 to e4cb80e22ee1 (2 revisions) (flutter/flutter#120903)

* a2e65f7c3 Roll Flutter Engine from e4cb80e22ee1 to 4a90fbcd6901 (2 revisions) (flutter/flutter#120911)

* e00241a06 Enable Windows plugin tests (flutter/flutter#119345)

* 09ad9f3cd Document ScrollPhysics invariant requiring ballistic motion (flutter/flutter#120400)

* 6029de2fb Update switch template (flutter/flutter#120919)

* 229d70ea3 Roll Flutter Engine from 4a90fbcd6901 to bddfc1c4dcaa (5 revisions) (flutter/flutter#120920)

* 206c6ae99 roll packages (flutter/flutter#120922)

* 9fcaaebb5 Roll Flutter Engine from bddfc1c4dcaa to 6602fc753525 (3 revisions) (flutter/flutter#120928)

* 00c0a07fa Increase Linux docs_test timeout (flutter/flutter#120899)

* e29a79975 946b29198 [dart:ui] Introduce `PlatformDispatcher.implicitView` (flutter/engine#39553) (flutter/flutter#120939)

* 081cd5776 650db7a72 [macOS] Eliminate mirrors support (flutter/engine#39694) (flutter/flutter#120943)

* 875e48c69 52a4fb4c5 Roll Skia from b1800a8b9595 to d0df677ffd5e (13 revisions) (flutter/engine#39699) (flutter/flutter#120947)

* 78d058f46 6e92c0c28 Roll Fuchsia Mac SDK from xl9Y8o-9FDyvPogki... to haDvcC5VzWVdQs9Rs... (flutter/engine#39700) (flutter/flutter#120950)

* 298d8c76b Revert "Remove references to Observatory (#118577)" (flutter/flutter#120929)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Feb 18, 2023
* df98689 2be7253c9 Roll Fuchsia Linux SDK from q7u2WyX2BSRBIzyTW... to yT4JLKTCWWwbRwB0l... (flutter/engine#39679) (flutter/flutter#120898)

* cacef57 [flutter_tools] Skip over "Resolving dependencies..." text in integration tests (flutter/flutter#120077)

* 34102ca Migrate channels to pkg:integration _test (flutter/flutter#120833)

* df13ea2 Roll Flutter Engine from 2be7253c9b10 to e4cb80e22ee1 (2 revisions) (flutter/flutter#120903)

* a2e65f7 Roll Flutter Engine from e4cb80e22ee1 to 4a90fbcd6901 (2 revisions) (flutter/flutter#120911)

* e00241a Enable Windows plugin tests (flutter/flutter#119345)

* 09ad9f3 Document ScrollPhysics invariant requiring ballistic motion (flutter/flutter#120400)

* 6029de2 Update switch template (flutter/flutter#120919)

* 229d70e Roll Flutter Engine from 4a90fbcd6901 to bddfc1c4dcaa (5 revisions) (flutter/flutter#120920)

* 206c6ae roll packages (flutter/flutter#120922)

* 9fcaaeb Roll Flutter Engine from bddfc1c4dcaa to 6602fc753525 (3 revisions) (flutter/flutter#120928)

* 00c0a07 Increase Linux docs_test timeout (flutter/flutter#120899)

* e29a799 946b29198 [dart:ui] Introduce `PlatformDispatcher.implicitView` (flutter/engine#39553) (flutter/flutter#120939)

* 081cd57 650db7a72 [macOS] Eliminate mirrors support (flutter/engine#39694) (flutter/flutter#120943)

* 875e48c 52a4fb4c5 Roll Skia from b1800a8b9595 to d0df677ffd5e (13 revisions) (flutter/engine#39699) (flutter/flutter#120947)

* 78d058f 6e92c0c28 Roll Fuchsia Mac SDK from xl9Y8o-9FDyvPogki... to haDvcC5VzWVdQs9Rs... (flutter/engine#39700) (flutter/flutter#120950)

* 298d8c7 Revert "Remove references to Observatory (#118577)" (flutter/flutter#120929)

* 674254c Always use the testbed in web_test.dart so `environment` is populated. (flutter/flutter#120984)

* c4d40cc Modify the updateChildren method deep copy _children (flutter/flutter#120773)

* 9367641 clean up (flutter/flutter#120934)

* 51712b9 Roll Plugins from d699b4a to 8f3419b (7 revisions) (flutter/flutter#120993)

* c3587c6 Add `InheritedTheme` support  to `ScrollbarTheme` (flutter/flutter#120970)

* 08b409a Roll Flutter Engine from 6e92c0c28410 to bd37a3992b50 (16 revisions) (flutter/flutter#121004)

* f785136 [web] Temporarily disable a line boundary test (flutter/flutter#121005)

* 9fe5567 Print sub process that failed to run in tool (flutter/flutter#120999)

* 6205c11 Remove "note that" in our documentation (as per style guide) (flutter/flutter#120842)

* 1daa0be Fix scrollable to clear inner semantics node if it does not use two p… (flutter/flutter#120996)

* 7f19b74 0a27673d7 Roll Skia from 02890036028e to 0e444e355607 (9 revisions) (flutter/engine#39723) (flutter/flutter#121008)

* 48d2dfc e7fde3f72 [web] Make glassPaneElement and glassPaneShadow non-nullable (flutter/engine#39692) (flutter/flutter#121009)

* 6104505 2b2780185 Roll Skia from 0e444e355607 to 4b79e398dfe0 (5 revisions) (flutter/engine#39725) (flutter/flutter#121016)

* f99f472 Remove the deprecated accentColor from ThemeData (flutter/flutter#120932)

* 2b4c960 Remove more references to dart:ui.window (flutter/flutter#120994)

* 0fa6527 Roll Flutter Engine from 2b2780185dd5 to a37e27b77008 (2 revisions) (flutter/flutter#121020)

* 9281114 Roll Flutter Engine from a37e27b77008 to 2fdce9a96367 (2 revisions) (flutter/flutter#121023)

* 4dd555d Roll Flutter Engine from 2fdce9a96367 to 9a3c3e462fce (3 revisions) (flutter/flutter#121025)

* 66dce65 Roll Flutter Engine from 9a3c3e462fce to 3777ed51774f (2 revisions) (flutter/flutter#121029)

* a5b53a6 a9db42c3e Roll Skia from 733a19f6a625 to 2f05923f825e (3 revisions) (flutter/engine#39744) (flutter/flutter#121030)

* 0be7c3f Roll Flutter Engine from a9db42c3edc2 to c22c64812243 (2 revisions) (flutter/flutter#121041)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: desktop embedder Related to the embedder API platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants