Migrate engine to use multi-window API #18368

gspencergoog · 2020-05-14T01:43:21Z

Description

This is a PR for trying out the multi-window API on the engine repo. The goal here is to convert from the window singleton to an API that has the concept of multiple windows. I'm not attempting to actually enable creating multiple windows here, just migrate to an API that has a concept of multiple windows/screens.

The design doc for this change is here.

The associated framework change is flutter/flutter#58696

The major changes in this PR:

In the C++/Java/ObjC side:
- moving platform-specific attributes and mutations from the Window class to a new PlatformConfiguration class.
- Adding ScreenMetrics and Screen classes so that windows can be on screens.
- Moving device_pixel_ratio to the ScreenMetrics, and adding an upper left position for windows (currently always 0, 0).
- Adds a way to update the screen metrics, similar to updating the viewport metrics.
On the Dart side:
- Move the platfom-specific attributes out of Window, and into the new PlatformDispatcher class that holds all of the platform state, so that the platform code need only update the configuration on this class.
- Create FlutterView, FlutterWindow, and SingletonFlutterWindow classes to separate out the concepts of a view (of which there may be multiple in a window), a window (of which there may be multiple on a screen, and they host views), and a window where there is only ever expected to be one (this hosts the entire API of the former Window class, and is the type of the window singleton).
- Remove the Window class entirely (it is replaced by SingletonFlutterWindow), so that we can eventually use that name as the name for the widget that creates new windows.

Things that still need to happen before review:

Update/expand ALL the docs.
Write tests for the multi-window portions
Testing/porting to Windows
Make sure that screen metric handling is consistent across platforms.
Remove the unimplemented createView/configureView/closeView API.

Related Issues

Tests

Framework master tests are expected to fail, as this is a breaking change for the framework. See [WIP] Migrate Framework to use multi-window API flutter#57178 for the necessary framework changes that will become a manual engine roll.
Other tests should pass.

Breaking Change

Adds a new member function to the embedding API, which must be implemented for embedders.

    bool SetScreenMetrics(flutter::ScreenMetrics metrics);

Renamed Engine::DefaultRouteName to Engine::InitialRouteName, and RuntimeController::DefaultRouteName to RuntimeController::InitialRouteName to match PlatformDispatcher name. Leaves window.defaultRouteName as-is.

lib/ui/window.dart

Hixie · 2020-06-11T22:26:48Z

lib/ui/window.dart

+///
+/// This class provides backward compatibility with code that was written before
+/// Flutter supported multiple top level windows. To modify or retrieve these
+/// properties, new code should use `WidgetsBinding.instance.platformDispatcher`.


If we don't want people to use an API, we shouldn't provide it. If it's only for legacy use, let's @deprecate it and sunset it.

I wouldn't do that though. I would just merge PlatformDispatcher into SingletonFlutterWindow and call it Window. The single-window case is the common case for most apps.

Would it be OK if I just remove the "backward compatibility" comments, and treat it entirely as non-legacy API?

I'd really rather not merge the classes back together. I separated them out because the functions on PlatformDispatcher don't belong conceptually to a "Window": they're all attributes of the platform configuration, and don't have to do with the concept of a window. It makes no sense to me, for instance, that alwaysUse24HourFormat is a window attribute, or that a window has an onPlatformMessage.

I think it's is incorrectly complecting things to keep them in Window, other than to keep from breaking existing code.

Also, (as I think you suggested in the doc), I'd like to use Window as the name of the widget that defines a new window, which is why I introduced SingletonFlutterWindow.

I can see why you would want to merge them again, but even if I just made window an instance of PlatformDispatcher, it would have odd things in it. For instance, it would have a list of views that included itself, and a screen accessor as well as a list of screens, and viewConfiguration accessor as well as a list of views. I can foresee people making mistakes accessing the wrong one, or just being confused.

lib/ui/hooks.dart

The goal here is to convert from the window singleton to an API that has the concept of multiple windows. I'm not attempting to actually enable creating multiple windows here, just migrate to an API that has a concept of multiple windows/screens.

…en initialization order on web.

…s unnecessarily breaking

… window class, to avoid breaking change.

stuartmorgan-g

Initial comments from skimming the native side of the changes. Figuring out how to handle embedder.h is going to be the trickiest part of the native side of this migration.

stuartmorgan-g · 2020-06-23T16:28:16Z

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

@@ -2,12 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

-#import "flutter/shell/platform/darwin/macos/framework/Headers/FlutterEngine.h"


The primary header(s) should be first. This was intentional, and your change violates the style guide.

Thanks, I know that, I'm not really sure why I reordered it like that.

stuartmorgan-g · 2020-06-23T16:29:49Z

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

@@ -298,13 +297,21 @@ - (void)updateWindowMetrics {
  CGSize scaledSize = [view convertRectToBacking:view.bounds].size;
  double pixelRatio = view.bounds.size.width == 0 ? 1 : scaledSize.width / view.bounds.size.width;

-  const FlutterWindowMetricsEvent event = {
-      .struct_size = sizeof(event),
+  // The screen is currently always the same size as the window.


Can we reference an issue that describes a transition plan for this? It seems weird to intentionally send incorrect information without more detail about the plan.

I filed flutter/flutter#60131 and referenced it in the comment.

I'm going to split out the screen stuff into a separate PR and land that before the rest of the changes here, to make it easier to review, and then I can even get the right screen information on each platform.

stuartmorgan-g · 2020-06-23T16:32:41Z

shell/platform/embedder/embedder.h

@@ -336,9 +336,18 @@ typedef struct {
  size_t width;
  /// Physical height of the window.
  size_t height;
+} FlutterWindowMetricsEvent;


This is a breaking change, which we don't generally allow in embedder.h. If we're changing the structs, we need new structs and new APIs using them, and the old path needs to do something reasonable (presumably translate under the hood to the new structs then call through).

OK. I can just duplicate the information about the pixel ratio when I send the window events, and send additional window events when the screen DPI changes (which shouldn't be often). I moved it because I didn't want it to be possible to have them not be in agreement with the screen information and I didn't want it to be ambiguous as to which one is authoritative.

But not breaking the ABI is obviously more important.

stuartmorgan-g · 2020-06-23T16:38:19Z

shell/platform/embedder/embedder.h

+FLUTTER_EXPORT
+FlutterEngineResult FlutterEngineSendScreenMetricsEvent(
+    FLUTTER_API_SYMBOL(FlutterEngine) engine,
+    const FlutterScreenMetricsEvent* event);


Since we don't make breaking changes to embedder.h, we should be as future-proof as possible in any new API we're adding. That means we shouldn't add an API that assumes a single screen when we know we're going to need to allow a list of screens. Instead we should design it for the future case, and document that it currently only uses the first screen.

Actually, the window and screen events were meant to have an additional id field so that they would be identified, I just forgot to add that here in the embedder implementation. (if you look at screen.h, you'll see what I mean). I've wired that through now, so it isn't limited to one screen.

stuartmorgan-g · 2020-06-23T16:38:56Z

shell/platform/linux/fl_engine.cc

@@ -3,13 +3,13 @@
 // found in the LICENSE file.

 #include "flutter/shell/platform/linux/public/flutter_linux/fl_engine.h"
-#include "flutter/shell/platform/linux/fl_engine_private.h"


This is a primary header, and should be first.

goderbauer

Some comments from skimming through the dart side for the first time.

goderbauer · 2020-06-23T18:41:13Z

lib/ui/platform_dispatcher.dart

+
+  /// The current platform configuration.
+  ///
+  /// If values in this configuration change, [onMetricsChanged] will be called.


Will onMetricsChanged be called in addition to onPlatformConfigurationChanged? Or is this a typo?

goderbauer · 2020-06-23T18:42:15Z

lib/ui/platform_dispatcher.dart

+  PlatformConfiguration get configuration => _configuration;
+  PlatformConfiguration _configuration = const PlatformConfiguration();
+
+  /// Called when the platform configuration changes.


Similarly to onMetricsChanged, maybe document what this callback is used for?

goderbauer · 2020-06-23T18:47:11Z

lib/ui/platform_dispatcher.dart

+  /// A const constructor for an immutable [ViewConfiguration].
+  const ViewConfiguration(
+    this.screen,
+  { this.window,


nit: formatting, put the { on the previous line?

goderbauer · 2020-06-23T18:52:57Z

lib/ui/platform_dispatcher.dart

+  /// This is the screen that the upper left corner of the view appears on.
+  final Screen screen;
+
+  /// The top level view into which the view is placed and its geometry is


This doc comment confused me a little but because it only talks about views and the property is named "window"...

gspencergoog · 2020-07-01T23:16:41Z

I'm going to split out all the screen-related changes into another PR that doesn't include Dart changes, so that can be reviewed separately, so I'm going to close this PR and reopen when I have something based on a submitted set of screen changes. This thing is just too huuge to review.

gspencergoog · 2020-07-01T23:17:05Z

The screens PR is #19425

0xZOne · 2020-07-02T15:01:42Z

Hello @gspencergoog, when will it be released ? are there any plans? Thanks!

gspencergoog · 2020-07-06T15:39:56Z

@0xZOne There are plans, but no concrete release date yet. There's a lot still to do before it will be ready, and we're somewhat resource constrained at the moment.

xuzhongpeng · 2022-03-13T16:29:13Z

@0xZOne There are plans, but no concrete release date yet. There's a lot still to do before it will be ready, and we're somewhat resource constrained at the moment.

Amazing.Are there any plans now? If not how can I compile it myself.

gspencergoog · 2022-03-14T15:39:47Z

There are plans, and we are working on it, but as yet no working prototypes you can compile.

gspencergoog added the Work in progress (WIP) Not ready (yet) for review! label May 14, 2020

googlebot added the cla: yes label May 14, 2020

gspencergoog force-pushed the multi_window branch 4 times, most recently from 3e43ee5 to 333a973 Compare May 14, 2020 01:56

gspencergoog mentioned this pull request May 14, 2020

[WIP] Migrate Framework to use multi-window API flutter/flutter#57178

Closed

1 task

gspencergoog force-pushed the multi_window branch 9 times, most recently from 0c9b84b to fe6e3a1 Compare May 20, 2020 17:24

gspencergoog force-pushed the multi_window branch 9 times, most recently from f0ce7f8 to df307a0 Compare June 2, 2020 16:44

gspencergoog force-pushed the multi_window branch 4 times, most recently from 6103c26 to 2f8e327 Compare June 4, 2020 16:10

gspencergoog mentioned this pull request Jun 4, 2020

[WIP] Migrate Framework to use multi-window API flutter/flutter#58696

Closed

1 task

Hixie reviewed Jun 11, 2020

View reviewed changes

lib/ui/window.dart Outdated Show resolved Hide resolved

Hixie reviewed Jun 11, 2020

View reviewed changes

lib/ui/hooks.dart Outdated Show resolved Hide resolved

gspencergoog force-pushed the multi_window branch from c70c663 to 5da9a1b Compare June 17, 2020 20:01

gspencergoog added 12 commits June 22, 2020 16:20

Migrate engine to use multi-window API

4e9f691

The goal here is to convert from the window singleton to an API that has the concept of multiple windows. I'm not attempting to actually enable creating multiple windows here, just migrate to an API that has a concept of multiple windows/screens.

Add a test

ca2021c

Fix some more tests

b04bfcd

Sync to head, added lots of NNBD hints, and fixed a problem with scre…

df4a4a8

…en initialization order on web.

Cleaning up

2eb9710

Merge changes

b3f65ee

Remove more future APIs

c1bc11e

Reverting rename of defaultRouteName => initialRouteName because it i…

c781a03

…s unnecessarily breaking

Rename defaultRouteName to initialRouteName for everything except for…

51f6ef1

… window class, to avoid breaking change.

Add screen accessor and testing.

e6be29a

Review Changes

282e7ae

Merge Changes

25634c6

stuartmorgan-g suggested changes Jun 23, 2020

View reviewed changes

goderbauer reviewed Jun 23, 2020

View reviewed changes

Fix bad merge, and other issues

a235ea6

gspencergoog force-pushed the multi_window branch from 5da9a1b to a235ea6 Compare June 23, 2020 20:10

gspencergoog mentioned this pull request Jun 23, 2020

☂️ Migration to multi-window API flutter/flutter#60131

Closed

6 tasks

Wire screen and window id through the embedding.

f670d07

gspencergoog closed this Jul 1, 2020

This comment was marked as off-topic.

Sign in to view

Migrate engine to use multi-window API #18368

Migrate engine to use multi-window API #18368

Uh oh!

Conversation

gspencergoog commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Breaking Change

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g 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!

goderbauer 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!

gspencergoog commented Jul 1, 2020

Uh oh!

gspencergoog commented Jul 1, 2020

Uh oh!

0xZOne commented Jul 2, 2020

Uh oh!

gspencergoog commented Jul 6, 2020

Uh oh!

xuzhongpeng commented Mar 13, 2022

Uh oh!

gspencergoog commented Mar 14, 2022

Uh oh!

This comment was marked as off-topic.

Uh oh!

gspencergoog commented May 14, 2020 •

edited

Loading