From 8780330699424bdf2f10dc0b52d301c9508242a6 Mon Sep 17 00:00:00 2001 From: Alex Wallen Date: Thu, 5 Jan 2023 09:58:19 -1000 Subject: [PATCH] Remove single view assumptions from `window.dart` (#38453) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Refactor `window.dart` * Remove single window assumption from platform dispatcher * Expose viewId * Remove FlutterWindow from web_ui * Refactor EngineFlutterWindow to inherit from FlutterView * Rename window property * Undo Iterable -> Map conversion * Add window and deprecate it so that the change isn't breaking * Name resolution * Revisions * Doc changes * Refactor deprecation message * Newline * Expose getViewById, hide map interface * Fix compilaiton errors * Introduce addView API * Change deprecation message * Update lib/ui/window.dart Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> * Take greg's todos * Add mutual exclusion assertion * Fix trailing whitespace lint * Use only one property to store backing view * Add doc comment to view * Document view and window parameters in the constructor of ViewConfiguration * Sync web api * Refactor assertion * Improve deprecation message * Improve window documentation * Assert one of window/view is null in copyWith ViewConfiguration * Remove EngineFlutterWindowView * Make dartdoc happy * Refactor copyWith() * Change to internal map implementation * final private refactor Co-authored-by: Michael Goderbauer * Deprecate window parameter in copyWith * Repl Window w/ View * Add tests for viewConfiguration * Make test descriptions better * Add ViewConfiguration initialization with window tests * Update lib/web_ui/lib/src/engine/window.dart Co-authored-by: David Iglesias * Refactor TODO message * Add expectation :) * Fix viewId in window.dart * Remove double deprecation access * punctuation Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> * Add kSingletonWindowID const Co-authored-by: a-wallen Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> Co-authored-by: Michael Goderbauer Co-authored-by: David Iglesias --- lib/ui/platform_dispatcher.dart | 51 ++++++++++++---- lib/ui/window.dart | 59 +++++-------------- lib/web_ui/lib/platform_dispatcher.dart | 33 +++++++++-- .../lib/src/engine/platform_dispatcher.dart | 21 ++++--- lib/web_ui/lib/src/engine/window.dart | 37 ++++-------- lib/web_ui/lib/window.dart | 11 +--- .../test/engine/platform_dispatcher_test.dart | 38 ++++++++++++ testing/dart/BUILD.gn | 1 + testing/dart/platform_dispatcher_test.dart | 42 +++++++++++++ testing/scenario_app/lib/src/scenario.dart | 2 +- 10 files changed, 187 insertions(+), 108 deletions(-) create mode 100644 testing/dart/platform_dispatcher_test.dart diff --git a/lib/ui/platform_dispatcher.dart b/lib/ui/platform_dispatcher.dart index f348a4f8db00e..b3b4de7369012 100644 --- a/lib/ui/platform_dispatcher.dart +++ b/lib/ui/platform_dispatcher.dart @@ -222,10 +222,10 @@ class PlatformDispatcher { final ViewConfiguration previousConfiguration = _viewConfigurations[id] ?? const ViewConfiguration(); if (!_views.containsKey(id)) { - _views[id] = FlutterWindow._(id, this); + _views[id] = FlutterView._(id, this); } _viewConfigurations[id] = previousConfiguration.copyWith( - window: _views[id], + view: _views[id], devicePixelRatio: devicePixelRatio, geometry: Rect.fromLTWH(0.0, 0.0, width, height), viewPadding: WindowPadding._( @@ -1289,8 +1289,18 @@ class PlatformConfiguration { /// An immutable view configuration. class ViewConfiguration { /// A const constructor for an immutable [ViewConfiguration]. + /// + /// When constructing a view configuration, supply either the [view] or the + /// [window] property, but not both since the [view] and [window] property + /// are backed by the same instance variable. const ViewConfiguration({ - this.window, + FlutterView? view, + @Deprecated(''' + Use the `view` property instead. + This change is related to adding multi-view support in Flutter. + This feature was deprecated after 3.7.0-1.2.pre. + ''') + FlutterView? window, this.devicePixelRatio = 1.0, this.geometry = Rect.zero, this.visible = false, @@ -1300,10 +1310,17 @@ class ViewConfiguration { this.padding = WindowPadding.zero, this.gestureSettings = const GestureSettings(), this.displayFeatures = const [], - }); + }) : assert(window == null || view == null), + _view = view ?? window; /// Copy this configuration with some fields replaced. ViewConfiguration copyWith({ + FlutterView? view, + @Deprecated(''' + Use the `view` property instead. + This change is related to adding multi-view support in Flutter. + This feature was deprecated after 3.7.0-1.2.pre. + ''') FlutterView? window, double? devicePixelRatio, Rect? geometry, @@ -1315,8 +1332,9 @@ class ViewConfiguration { GestureSettings? gestureSettings, List? displayFeatures, }) { + assert(view == null || window == null); return ViewConfiguration( - window: window ?? this.window, + view: view ?? window ?? _view, devicePixelRatio: devicePixelRatio ?? this.devicePixelRatio, geometry: geometry ?? this.geometry, visible: visible ?? this.visible, @@ -1329,11 +1347,22 @@ class ViewConfiguration { ); } - /// The top level view into which the view is placed and its geometry is - /// relative to. + /// The top level view for which this [ViewConfiguration]'s properties apply to. + /// + /// If this property is null, this [ViewConfiguration] is a top level view. + @Deprecated(''' + Use the `view` property instead. + This change is related to adding multi-view support in Flutter. + This feature was deprecated after 3.7.0-1.2.pre. + ''') + FlutterView? get window => _view; + + /// The top level view for which this [ViewConfiguration]'s properties apply to. /// - /// If null, then this configuration represents a top level view itself. - final FlutterView? window; + /// If this property is null, this [ViewConfiguration] is a top level view. + FlutterView? get view => _view; + + final FlutterView? _view; /// The pixel density of the output surface. final double devicePixelRatio; @@ -1427,7 +1456,7 @@ class ViewConfiguration { @override String toString() { - return '$runtimeType[window: $window, geometry: $geometry]'; + return '$runtimeType[view: $view, geometry: $geometry]'; } } @@ -1666,7 +1695,7 @@ enum AppLifecycleState { /// On Android, this corresponds to an app or the Flutter host view running /// in the foreground inactive state. Apps transition to this state when /// another activity is focused, such as a split-screen app, a phone call, - /// a picture-in-picture app, a system dialog, or another window. + /// a picture-in-picture app, a system dialog, or another view. /// /// Apps in this state should assume that they may be [paused] at any time. inactive, diff --git a/lib/ui/window.dart b/lib/ui/window.dart index 6450fad95c413..b823dd34ea215 100644 --- a/lib/ui/window.dart +++ b/lib/ui/window.dart @@ -5,8 +5,8 @@ part of dart.ui; /// A view into which a Flutter [Scene] is drawn. /// -/// Each [FlutterView] has its own layer tree that is rendered into an area -/// inside of a [FlutterWindow] whenever [render] is called with a [Scene]. +/// Each [FlutterView] has its own layer tree that is rendered +/// whenever [render] is called on it with a [Scene]. /// /// ## Insets and Padding /// @@ -51,18 +51,21 @@ part of dart.ui; /// the [viewPadding] anyway, so there is no need to account for /// that in the [padding], which is always safe to use for such /// calculations. -/// -/// See also: -/// -/// * [FlutterWindow], a special case of a [FlutterView] that is represented on -/// the platform as a separate window which can host other [FlutterView]s. -abstract class FlutterView { +class FlutterView { + FlutterView._(this.viewId, this.platformDispatcher); + + /// The opaque ID for this view. + final Object viewId; + /// The platform dispatcher that this view is registered with, and gets its /// information from. - PlatformDispatcher get platformDispatcher; + final PlatformDispatcher platformDispatcher; /// The configuration of this view. - ViewConfiguration get viewConfiguration; + ViewConfiguration get viewConfiguration { + assert(platformDispatcher._viewConfigurations.containsKey(viewId)); + return platformDispatcher._viewConfigurations[viewId]!; + } /// The number of device pixels for each logical pixel for the screen this /// view is displayed on. @@ -281,39 +284,7 @@ abstract class FlutterView { external static void _updateSemantics(SemanticsUpdate update); } -/// A top-level platform window displaying a Flutter layer tree drawn from a -/// [Scene]. -/// -/// The current list of all Flutter views for the application is available from -/// `WidgetsBinding.instance.platformDispatcher.views`. Only views that are of type -/// [FlutterWindow] are top level platform windows. -/// -/// There is also a [PlatformDispatcher.instance] singleton object in `dart:ui` -/// if `WidgetsBinding` is unavailable, but we strongly advise avoiding a static -/// reference to it. See the documentation for [PlatformDispatcher.instance] for -/// more details about why it should be avoided. -/// -/// See also: -/// -/// * [PlatformDispatcher], which manages the current list of [FlutterView] (and -/// thus [FlutterWindow]) instances. -class FlutterWindow extends FlutterView { - FlutterWindow._(this._windowId, this.platformDispatcher); - - /// The opaque ID for this view. - final Object _windowId; - - @override - final PlatformDispatcher platformDispatcher; - - @override - ViewConfiguration get viewConfiguration { - assert(platformDispatcher._viewConfigurations.containsKey(_windowId)); - return platformDispatcher._viewConfigurations[_windowId]!; - } -} - -/// A [FlutterWindow] that includes access to setting callbacks and retrieving +/// A [FlutterView] that includes access to setting callbacks and retrieving /// properties that reside on the [PlatformDispatcher]. /// /// It is the type of the global [window] singleton used by applications that @@ -328,7 +299,7 @@ class FlutterWindow extends FlutterView { /// `WidgetsBinding.instance.platformDispatcher` over a static reference to /// [window], or [PlatformDispatcher.instance]. See the documentation for /// [PlatformDispatcher.instance] for more details about this recommendation. -class SingletonFlutterWindow extends FlutterWindow { +class SingletonFlutterWindow extends FlutterView { SingletonFlutterWindow._(super.windowId, super.platformDispatcher) : super._(); diff --git a/lib/web_ui/lib/platform_dispatcher.dart b/lib/web_ui/lib/platform_dispatcher.dart index 82d991915d8ff..5d569d985264c 100644 --- a/lib/web_ui/lib/platform_dispatcher.dart +++ b/lib/web_ui/lib/platform_dispatcher.dart @@ -190,7 +190,13 @@ class PlatformConfiguration { class ViewConfiguration { const ViewConfiguration({ - this.window, + FlutterView? view, + @Deprecated(''' + Use the `view` property instead. + This change is related to adding multi-view support in Flutter. + This feature was deprecated after 3.7.0-1.2.pre. + ''') + FlutterView? window, this.devicePixelRatio = 1.0, this.geometry = Rect.zero, this.visible = false, @@ -200,10 +206,17 @@ class ViewConfiguration { this.padding = WindowPadding.zero, this.gestureSettings = const GestureSettings(), this.displayFeatures = const [], - }); + }) : assert(window == null || view == null), + _view = view ?? window; ViewConfiguration copyWith({ - FlutterWindow? window, + FlutterView? view, + @Deprecated(''' + Use the `view` property instead. + This change is related to adding multi-view support in Flutter. + This feature was deprecated after 3.7.0-1.2.pre. + ''') + FlutterView? window, double? devicePixelRatio, Rect? geometry, bool? visible, @@ -214,8 +227,9 @@ class ViewConfiguration { GestureSettings? gestureSettings, List? displayFeatures, }) { + assert(view == null || window == null); return ViewConfiguration( - window: window ?? this.window, + view: view ?? window ?? _view, devicePixelRatio: devicePixelRatio ?? this.devicePixelRatio, geometry: geometry ?? this.geometry, visible: visible ?? this.visible, @@ -228,7 +242,14 @@ class ViewConfiguration { ); } - final FlutterWindow? window; + @Deprecated(''' + Use the `view` property instead. + This change is related to adding multi-view support in Flutter. + This feature was deprecated after 3.7.0-1.2.pre. + ''') + FlutterView? get window => _view; + FlutterView? get view => _view; + final FlutterView? _view; final double devicePixelRatio; final Rect geometry; final bool visible; @@ -241,7 +262,7 @@ class ViewConfiguration { @override String toString() { - return '$runtimeType[window: $window, geometry: $geometry]'; + return '$runtimeType[view: $view, geometry: $geometry]'; } } diff --git a/lib/web_ui/lib/src/engine/platform_dispatcher.dart b/lib/web_ui/lib/src/engine/platform_dispatcher.dart index 480b367a2be3c..b34226fd4ee3e 100644 --- a/lib/web_ui/lib/src/engine/platform_dispatcher.dart +++ b/lib/web_ui/lib/src/engine/platform_dispatcher.dart @@ -136,11 +136,10 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { _onPlatformConfigurationChanged, _onPlatformConfigurationChangedZone); } - /// The current list of windows, + /// The current list of windows. @override - Iterable get views => _windows.values; - Map get windows => _windows; - final Map _windows = {}; + Iterable get views => viewData.values; + final Map viewData = {}; /// A map of opaque platform window identifiers to window configurations. /// @@ -478,10 +477,10 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { final MethodCall decoded = codec.decodeMethodCall(data); switch (decoded.method) { case 'SystemNavigator.pop': - // TODO(gspencergoog): As multi-window support expands, the pop call - // will need to include the window ID. Right now only one window is + // TODO(a-wallen): As multi-window support expands, the pop call + // will need to include the view ID. Right now only one view is // supported. - (_windows[0]! as EngineFlutterWindow) + (viewData[kSingletonViewId]! as EngineFlutterWindow) .browserHistory .exit() .then((_) { @@ -568,10 +567,10 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { return; case 'flutter/navigation': - // TODO(gspencergoog): As multi-window support expands, the navigation call - // will need to include the window ID. Right now only one window is + // TODO(a-wallen): As multi-window support expands, the navigation call + // will need to include the view ID. Right now only one view is // supported. - (_windows[0]! as EngineFlutterWindow) + (viewData[kSingletonViewId]! as EngineFlutterWindow) .handleNavigationMessage(data) .then((bool handled) { if (handled) { @@ -1160,7 +1159,7 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { @override String get defaultRouteName { return _defaultRouteName ??= - (_windows[0]! as EngineFlutterWindow).browserHistory.currentPath; + (viewData[kSingletonViewId]! as EngineFlutterWindow).browserHistory.currentPath; } /// Lazily initialized when the `defaultRouteName` getter is invoked. diff --git a/lib/web_ui/lib/src/engine/window.dart b/lib/web_ui/lib/src/engine/window.dart index 5538055f589f7..1d8e2d13bcef9 100644 --- a/lib/web_ui/lib/src/engine/window.dart +++ b/lib/web_ui/lib/src/engine/window.dart @@ -27,6 +27,9 @@ typedef _HandleMessageCallBack = Future Function(); /// When set to true, all platform messages will be printed to the console. const bool debugPrintPlatformMessages = false; +/// The view ID for a singleton flutter window. +const int kSingletonViewId = 0; + /// Whether [_customUrlStrategy] has been set or not. /// /// It is valid to set [_customUrlStrategy] to null, so we can't use a null @@ -43,11 +46,11 @@ set customUrlStrategy(UrlStrategy? strategy) { /// The Web implementation of [ui.SingletonFlutterWindow]. class EngineFlutterWindow extends ui.SingletonFlutterWindow { - EngineFlutterWindow(this._windowId, this.platformDispatcher) { + EngineFlutterWindow(this.viewId, this.platformDispatcher) { final EnginePlatformDispatcher engineDispatcher = platformDispatcher as EnginePlatformDispatcher; - engineDispatcher.windows[_windowId] = this; - engineDispatcher.windowConfigurations[_windowId] = const ui.ViewConfiguration(); + engineDispatcher.viewData[viewId] = this; + engineDispatcher.windowConfigurations[viewId] = const ui.ViewConfiguration(); if (_isUrlStrategySet) { _browserHistory = createHistoryForExistingState(_customUrlStrategy); } @@ -58,7 +61,8 @@ class EngineFlutterWindow extends ui.SingletonFlutterWindow { }); } - final Object _windowId; + @override + final Object viewId; @override final ui.PlatformDispatcher platformDispatcher; @@ -202,8 +206,8 @@ class EngineFlutterWindow extends ui.SingletonFlutterWindow { ui.ViewConfiguration get viewConfiguration { final EnginePlatformDispatcher engineDispatcher = platformDispatcher as EnginePlatformDispatcher; - assert(engineDispatcher.windowConfigurations.containsKey(_windowId)); - return engineDispatcher.windowConfigurations[_windowId] ?? + assert(engineDispatcher.windowConfigurations.containsKey(viewId)); + return engineDispatcher.windowConfigurations[viewId] ?? const ui.ViewConfiguration(); } @@ -339,32 +343,13 @@ class EngineSingletonFlutterWindow extends EngineFlutterWindow { double? _debugDevicePixelRatio; } -/// A type of [FlutterView] that can be hosted inside of a [FlutterWindow]. -class EngineFlutterWindowView extends ui.FlutterWindow { - EngineFlutterWindowView._(this._viewId, this.platformDispatcher); - - final Object _viewId; - - @override - final ui.PlatformDispatcher platformDispatcher; - - @override - ui.ViewConfiguration get viewConfiguration { - final EnginePlatformDispatcher engineDispatcher = - platformDispatcher as EnginePlatformDispatcher; - assert(engineDispatcher.windowConfigurations.containsKey(_viewId)); - return engineDispatcher.windowConfigurations[_viewId] ?? - const ui.ViewConfiguration(); - } -} - /// The window singleton. /// /// `dart:ui` window delegates to this value. However, this value has a wider /// API surface, providing Web-specific functionality that the standard /// `dart:ui` version does not. final EngineSingletonFlutterWindow window = - EngineSingletonFlutterWindow(0, EnginePlatformDispatcher.instance); + EngineSingletonFlutterWindow(kSingletonViewId, EnginePlatformDispatcher.instance); /// The Web implementation of [ui.WindowPadding]. class WindowPadding implements ui.WindowPadding { diff --git a/lib/web_ui/lib/window.dart b/lib/web_ui/lib/window.dart index ee4e0e35ec897..99e236a203614 100644 --- a/lib/web_ui/lib/window.dart +++ b/lib/web_ui/lib/window.dart @@ -7,6 +7,7 @@ part of ui; abstract class FlutterView { PlatformDispatcher get platformDispatcher; ViewConfiguration get viewConfiguration; + Object get viewId; double get devicePixelRatio => viewConfiguration.devicePixelRatio; Rect get physicalGeometry => viewConfiguration.geometry; Size get physicalSize => viewConfiguration.geometry.size; @@ -19,15 +20,7 @@ abstract class FlutterView { void updateSemantics(SemanticsUpdate update) => platformDispatcher.updateSemantics(update); } -abstract class FlutterWindow extends FlutterView { - @override - PlatformDispatcher get platformDispatcher; - - @override - ViewConfiguration get viewConfiguration; -} - -abstract class SingletonFlutterWindow extends FlutterWindow { +abstract class SingletonFlutterWindow extends FlutterView { VoidCallback? get onMetricsChanged => platformDispatcher.onMetricsChanged; set onMetricsChanged(VoidCallback? callback) { platformDispatcher.onMetricsChanged = callback; diff --git a/lib/web_ui/test/engine/platform_dispatcher_test.dart b/lib/web_ui/test/engine/platform_dispatcher_test.dart index 2568f27181e36..e304394e7e30a 100644 --- a/lib/web_ui/test/engine/platform_dispatcher_test.dart +++ b/lib/web_ui/test/engine/platform_dispatcher_test.dart @@ -11,6 +11,8 @@ import 'package:test/test.dart'; import 'package:ui/src/engine.dart'; import 'package:ui/ui.dart' as ui; +import '../matchers.dart'; + void main() { internalBootstrapBrowserTest(() => testMain); } @@ -160,6 +162,42 @@ void testMain() { expect(ui.PlatformDispatcher.instance.textScaleFactor, findBrowserTextScaleFactor()); }); }); + + test('A ViewConfiguration asserts that both window and view are not provided', () { + expect(() { + // ignore: deprecated_member_use + final EngineFlutterWindow window = EngineFlutterWindow(0, ui.PlatformDispatcher.instance); + ui.ViewConfiguration( + window: window, + view: window, + ); + }, throwsAssertionError); + }); + + test("A ViewConfiguration's view and window are backed with the same property", () { + final ui.FlutterView window = EngineFlutterWindow(0, ui.PlatformDispatcher.instance); + final ui.ViewConfiguration viewConfiguration = ui.ViewConfiguration(view: window); + // ignore: deprecated_member_use + expect(viewConfiguration.view, window); + // ignore: deprecated_member_use + expect(viewConfiguration.window, viewConfiguration.view); + }); + + test('Initialize a ViewConfiguration with a window', () { + final ui.FlutterView window = EngineFlutterWindow(0, ui.PlatformDispatcher.instance); + // ignore: deprecated_member_use + final ui.ViewConfiguration configuration = ui.ViewConfiguration(window: window); + // ignore: deprecated_member_use + expect(window, configuration.window); + }); + + test("copyWith() on a ViewConfiguration asserts that both a window aren't provided", () { + final ui.FlutterView window = EngineFlutterWindow(0, ui.PlatformDispatcher.instance); + // ignore: deprecated_member_use + expect(() { + ui.ViewConfiguration(view: window).copyWith(view: window, window: window); + }, throwsAssertionError); + }); } class MockHighContrastSupport implements HighContrastSupport { diff --git a/testing/dart/BUILD.gn b/testing/dart/BUILD.gn index 1c372c5ed9ea1..7ee975b6d384e 100644 --- a/testing/dart/BUILD.gn +++ b/testing/dart/BUILD.gn @@ -37,6 +37,7 @@ tests = [ "paragraph_test.dart", "path_test.dart", "picture_test.dart", + "platform_dispatcher_test.dart", "platform_view_test.dart", "plugin_utilities_test.dart", "semantics_test.dart", diff --git a/testing/dart/platform_dispatcher_test.dart b/testing/dart/platform_dispatcher_test.dart new file mode 100644 index 0000000000000..dc19fba3fe57f --- /dev/null +++ b/testing/dart/platform_dispatcher_test.dart @@ -0,0 +1,42 @@ +// Copyright 2022 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:ui'; + +import 'package:litetest/litetest.dart'; + +void main() { + test('A ViewConfiguration asserts that both window and view are not provided', () { + expectAssertion(() { + return ViewConfiguration( + // ignore: deprecated_member_use + window: PlatformDispatcher.instance.views.first, + view: PlatformDispatcher.instance.views.first, + ); + }); + }); + + test("A ViewConfiguration's view and window are backed with the same property", () { + final FlutterView view = PlatformDispatcher.instance.views.first; + final ViewConfiguration viewConfiguration = ViewConfiguration(view: view); + // ignore: deprecated_member_use + expect(viewConfiguration.window, view); + // ignore: deprecated_member_use + expect(viewConfiguration.window, viewConfiguration.view); + }); + + test('Initialize a ViewConfiguration with a window', () { + final FlutterView view = PlatformDispatcher.instance.views.first; + // ignore: deprecated_member_use + ViewConfiguration(window: view); + }); + + test("copyWith() on a ViewConfiguration asserts that both a window aren't provided", () { + final FlutterView view = PlatformDispatcher.instance.views.first; + expectAssertion(() { + // ignore: deprecated_member_use + return ViewConfiguration(view: view)..copyWith(view: view, window: view); + }); + }); +} diff --git a/testing/scenario_app/lib/src/scenario.dart b/testing/scenario_app/lib/src/scenario.dart index 6e66c862280e9..68c5d6506fa55 100644 --- a/testing/scenario_app/lib/src/scenario.dart +++ b/testing/scenario_app/lib/src/scenario.dart @@ -7,7 +7,7 @@ import 'dart:ui'; /// A scenario to run for testing. abstract class Scenario { - /// Creates a new scenario using a specific FlutterWindow instance. + /// Creates a new scenario using a specific FlutterView instance. Scenario(this.dispatcher); /// The window used by this scenario. May be mocked.