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.

Sign up for GitHub

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

Remove single view assumptions from window.dart #38453

Merged
merged 46 commits into from
Jan 5, 2023
Merged

Remove single view assumptions from window.dart #38453

Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
5f9cbb0
Refactor `window.dart`
Dec 21, 2022
1f5376a
Remove single window assumption from platform dispatcher
Dec 21, 2022
def8a49
Expose viewId
Dec 21, 2022
ee1d3a3
Remove FlutterWindow from web_ui
Dec 22, 2022
59a1b34
Refactor EngineFlutterWindow to inherit from FlutterView
Dec 22, 2022
559ab5c
Rename window property
Dec 22, 2022
3ef5f40
Undo Iterable -> Map conversion
Dec 22, 2022
368ec15
Add window and deprecate it so that the change isn't breaking
Dec 22, 2022
570cc18
Name resolution
Dec 22, 2022
109c396
Revisions
Dec 22, 2022
b08531d
Doc changes
Dec 22, 2022
d8425c2
Refactor deprecation message
Jan 3, 2023
6a8fd2e
Newline
Jan 3, 2023
4c659b1
Expose getViewById, hide map interface
Jan 3, 2023
48832da
Fix compilaiton errors
Jan 3, 2023
9504d93
Introduce addView API
Jan 3, 2023
bdb74d2
Change deprecation message
Jan 3, 2023
4453c99
Update lib/ui/window.dart
a-wallen Jan 3, 2023
5103c3a
Take greg's todos
Jan 3, 2023
184e658
Add mutual exclusion assertion
Jan 3, 2023
864a0ee
Fix trailing whitespace lint
Jan 3, 2023
0ae4700
Use only one property to store backing view
Dec 22, 2022
56e558c
Add doc comment to view
Dec 22, 2022
278f724
Document view and window parameters in the constructor of ViewConfigu…
Dec 22, 2022
ce6db62
Sync web api
Dec 22, 2022
23be98b
Refactor assertion
Dec 22, 2022
40922b5
Improve deprecation message
Dec 22, 2022
75d1cbe
Improve window documentation
Dec 22, 2022
e8df09f
Assert one of window/view is null in copyWith ViewConfiguration
Dec 22, 2022
0209cf6
Remove EngineFlutterWindowView
Dec 22, 2022
ec25975
Make dartdoc happy
Dec 22, 2022
8199e72
Refactor copyWith()
Dec 22, 2022
b3164c8
Change to internal map implementation
Dec 22, 2022
d42d9ec
final private refactor
a-wallen Jan 4, 2023
5c2e0a9
Deprecate window parameter in copyWith
Dec 22, 2022
12d4871
Repl Window w/ View
Dec 22, 2022
15ebe59
Add tests for viewConfiguration
Dec 22, 2022
1f71e01
Make test descriptions better
Dec 22, 2022
6d531ba
Add ViewConfiguration initialization with window tests
Dec 22, 2022
3a70482
Update lib/web_ui/lib/src/engine/window.dart
a-wallen Jan 5, 2023
2a4f807
Refactor TODO message
Dec 22, 2022
9f4ac26
Add expectation :)
Dec 22, 2022
b2b410e
Fix viewId in window.dart
Dec 22, 2022
4bbd09d
Remove double deprecation access
Dec 22, 2022
82defe4
punctuation
a-wallen Jan 5, 2023
8fa153b
Add kSingletonWindowID const
Dec 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 35 additions & 11 deletions lib/ui/platform_dispatcher.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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._(
Expand Down Expand Up @@ -1289,8 +1289,18 @@ class PlatformConfiguration {
/// An immutable view configuration.
class ViewConfiguration {
/// A const constructor for an immutable [ViewConfiguration].
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
///
/// 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('''
goderbauer marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Ultimately, it's up to you, but I would probably do the rename (with deprecation) in a separate PR that's dedicated to just that. That may make it easier to land these two changes and potentially roll back if there are issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a good idea. I'd like to address all the feedback and get an LGTM before I cherry-pick the commits from this PR into another one.

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,
Expand All @@ -1300,10 +1310,12 @@ class ViewConfiguration {
this.padding = WindowPadding.zero,
this.gestureSettings = const GestureSettings(),
this.displayFeatures = const <DisplayFeature>[],
});
}) : assert(window == null || view == null),
_view = view ?? window;

/// Copy this configuration with some fields replaced.
ViewConfiguration copyWith({
FlutterView? view,
Copy link
Member

Choose a reason for hiding this comment

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

Deprecate the window parameter in the copyWith functions as well? That one will also need to be removed at some point, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

5c2e0a9

FlutterView? window,
Copy link
Member

Choose a reason for hiding this comment

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

copy with should do something with the window property instead of silently ignoring it.

double? devicePixelRatio,
Rect? geometry,
Expand All @@ -1315,8 +1327,9 @@ class ViewConfiguration {
GestureSettings? gestureSettings,
List<DisplayFeature>? displayFeatures,
}) {
assert(view == null || window == null);
return ViewConfiguration(
Copy link
Member

Choose a reason for hiding this comment

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

should probably assert that at least one of window, view is null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added 7d2ad3e.

window: window ?? this.window,
view: view ?? window ?? _view,
devicePixelRatio: devicePixelRatio ?? this.devicePixelRatio,
geometry: geometry ?? this.geometry,
visible: visible ?? this.visible,
Expand All @@ -1329,11 +1342,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;
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved

final FlutterView? _view;

/// The pixel density of the output surface.
final double devicePixelRatio;
Expand Down Expand Up @@ -1427,7 +1451,7 @@ class ViewConfiguration {

@override
String toString() {
return '$runtimeType[window: $window, geometry: $geometry]';
return '$runtimeType[view: $view, geometry: $geometry]';
}
}

Expand Down Expand Up @@ -1666,7 +1690,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,
Expand Down
60 changes: 16 additions & 44 deletions lib/ui/window.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
///
Expand Down Expand Up @@ -51,18 +51,22 @@ 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;
Copy link
Member

Choose a reason for hiding this comment

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

move the private field below the getter. Our style is to have the following order: getter, private field, setter. The setter may be optional, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing.

Is that documented somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

I actually couldn't find this in the style guide, but it is something we try to follow. We should add it.

dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
Object get viewId => _viewId;
a-wallen marked this conversation as resolved.
Show resolved Hide resolved

/// 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.
Expand Down Expand Up @@ -281,39 +285,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
Expand All @@ -328,7 +300,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 {
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
SingletonFlutterWindow._(super.windowId, super.platformDispatcher)
: super._();

Expand Down
28 changes: 22 additions & 6 deletions lib/web_ui/lib/platform_dispatcher.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -200,10 +206,12 @@ class ViewConfiguration {
this.padding = WindowPadding.zero,
this.gestureSettings = const GestureSettings(),
this.displayFeatures = const <DisplayFeature>[],
});
}) : assert(window == null || view == null),
_view = view ?? window;

ViewConfiguration copyWith({
FlutterWindow? window,
FlutterView? view,
FlutterView? window,
Copy link
Member

Choose a reason for hiding this comment

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

Deprecate window param?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

5c2e0a9

double? devicePixelRatio,
Rect? geometry,
bool? visible,
Expand All @@ -214,8 +222,9 @@ class ViewConfiguration {
GestureSettings? gestureSettings,
List<DisplayFeature>? displayFeatures,
}) {
assert(view == null || window == null);
return ViewConfiguration(
Copy link
Member

Choose a reason for hiding this comment

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

assert here that at most of view, window is not-null.

window: window ?? this.window,
view: view ?? window ?? _view,
devicePixelRatio: devicePixelRatio ?? this.devicePixelRatio,
geometry: geometry ?? this.geometry,
visible: visible ?? this.visible,
Expand All @@ -228,7 +237,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;
Expand All @@ -241,7 +257,7 @@ class ViewConfiguration {

@override
String toString() {
return '$runtimeType[window: $window, geometry: $geometry]';
return '$runtimeType[view: $view, geometry: $geometry]';
}
}

Expand Down
15 changes: 7 additions & 8 deletions lib/web_ui/lib/src/engine/platform_dispatcher.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,8 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher {

/// The current list of windows,
a-wallen marked this conversation as resolved.
Show resolved Hide resolved
@override
Iterable<ui.FlutterView> get views => _windows.values;
Map<Object, ui.FlutterWindow> get windows => _windows;
final Map<Object, ui.FlutterWindow> _windows = <Object, ui.FlutterWindow>{};
Iterable<ui.FlutterView> get views => viewData.values;
final Map<Object, ui.FlutterView> viewData = <Object, ui.FlutterView>{};
ditman marked this conversation as resolved.
Show resolved Hide resolved

/// A map of opaque platform window identifiers to window configurations.
///
Expand Down Expand Up @@ -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
// TODO(a-wallen): As multi-window support expands, the pop call
// will need to include the window ID. Right now only one window is
a-wallen marked this conversation as resolved.
Show resolved Hide resolved
// supported.
(_windows[0]! as EngineFlutterWindow)
(viewData[0]! as EngineFlutterWindow)
Copy link
Contributor

Choose a reason for hiding this comment

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

0 seems to have a special meaning here that's not documented.

The code looks like it's trying to access the "first" element in a list, but that's not what's happening. viewData (and previously _windows) is a map, and the key 0 is basically a special hard-coded key that's shared between here and:

engine/lib/web_ui/lib/src/engine/window.dart

Lines 366 to 367 in bb40152

final EngineSingletonFlutterWindow window =
EngineSingletonFlutterWindow(0, EnginePlatformDispatcher.instance);

I suggest we make that explicit through a constant (e.g. const int kSingletonViewId = 0), and stop using literal 0 elsewhere.

(I know this existed before your PR, but I wanted to recommend it since you are working on this part of the code anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

8fa153b

.browserHistory
.exit()
.then((_) {
Expand Down Expand Up @@ -568,10 +567,10 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher {
return;

case 'flutter/navigation':
// TODO(gspencergoog): As multi-window support expands, the navigation call
// TODO(a-wallen): As multi-window support expands, the navigation call
// will need to include the window ID. Right now only one window is
// supported.
(_windows[0]! as EngineFlutterWindow)
(viewData[0]! as EngineFlutterWindow)
.handleNavigationMessage(data)
.then((bool handled) {
if (handled) {
Expand Down Expand Up @@ -1160,7 +1159,7 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher {
@override
String get defaultRouteName {
return _defaultRouteName ??=
(_windows[0]! as EngineFlutterWindow).browserHistory.currentPath;
(viewData[0]! as EngineFlutterWindow).browserHistory.currentPath;
}

/// Lazily initialized when the `defaultRouteName` getter is invoked.
Expand Down
34 changes: 9 additions & 25 deletions lib/web_ui/lib/src/engine/window.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,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);
}
Expand All @@ -58,7 +58,10 @@ class EngineFlutterWindow extends ui.SingletonFlutterWindow {
});
}

final Object _windowId;
final Object _viewId;

@override
Object get viewId => _viewId;
a-wallen marked this conversation as resolved.
Show resolved Hide resolved

@override
final ui.PlatformDispatcher platformDispatcher;
Expand Down Expand Up @@ -202,8 +205,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();
}

Expand Down Expand Up @@ -339,25 +342,6 @@ class EngineSingletonFlutterWindow extends EngineFlutterWindow {
double? _debugDevicePixelRatio;
}

/// A type of [FlutterView] that can be hosted inside of a [FlutterWindow].
class EngineFlutterWindowView extends ui.FlutterWindow {
ditman marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
11 changes: 2 additions & 9 deletions lib/web_ui/lib/window.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down