-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Deprecate single semantics tree assumption from platform dispatcher #36675
Conversation
ccc5c13
to
033f9df
Compare
lib/ui/platform_dispatcher.dart
Outdated
In a multi-view world, the platform dispatcher can no longer provide apis | ||
to update semantics since each view will host its own semantics tree. | ||
|
||
Semantics updates must be passed to an individual flutter view. To update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "flutter view" -> FlutterView since its an actual class
lib/ui/window.dart
Outdated
@@ -266,6 +266,19 @@ abstract class FlutterView { | |||
|
|||
@FfiNative<Void Function(Pointer<Void>)>('PlatformConfigurationNativeApi::Render') | |||
external static void _render(Scene scene); | |||
|
|||
/// Change the retained semantics data about this platform dispatcher. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: replace "platform dispatcher" with flutter view?
lib/ui/window.dart
Outdated
|
||
/// Change the retained semantics data about this platform dispatcher. | ||
/// | ||
/// If [semanticsEnabled] is true, the user has requested that this function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit" semanticsEnabled is a property on the PlatformDispatcher, not the view, right? So, this should probable change to [PlatformDispatcher.semanticsEnabled]
.
lib/ui/window.dart
Outdated
/// Change the retained semantics data about this platform dispatcher. | ||
/// | ||
/// If [semanticsEnabled] is true, the user has requested that this function | ||
/// be called whenever the semantic content of this platform dispatcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
platform dispatcher -> flutter view (or maybe just view)
lib/ui/window.dart
Outdated
/// be called whenever the semantic content of this platform dispatcher | ||
/// changes. | ||
/// | ||
/// In either case, this function disposes the given update, which means the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe remove "in either case" as its just empty prose?
lib/ui/window.dart
Outdated
@override | ||
@Deprecated(''' | ||
A singleton flutter window no longer manages semantics trees. In a multi-view | ||
world, each flutter view must manage its own semantics tree. | ||
|
||
Call updateSemantics() from FlutterView instead. | ||
''') | ||
void updateSemantics(SemanticsUpdate update) => platformDispatcher.updateSemantics(update); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"SingletonFlutterWindow" extends FlutterView, so updateSemantics
continues to be available. Instead of deprecating it, I think you just want to remove this override altogether, so it uses FlutterView.updateSemantics
from the base class instead of forwarding to the platform dispatcher.
lib/ui/platform_dispatcher.dart
Outdated
@@ -758,6 +758,14 @@ class PlatformDispatcher { | |||
/// | |||
/// In either case, this function disposes the given update, which means the | |||
/// semantics update cannot be used further. | |||
@Deprecated(''' | |||
In a multi-view world, the platform dispatcher can no longer provide apis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indent these lines by two so they are indented more than the @Deprecated
line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(here and for all other @Deprecarted
instances below as well.)
lib/web_ui/lib/window.dart
Outdated
@override | ||
@Deprecated(''' | ||
A singleton flutter window no longer manages semantics trees. In a multi-view | ||
world, each flutter view must manage its own semantics tree. | ||
|
||
Call updateSemantics() from FlutterView instead. | ||
''') | ||
void updateSemantics(SemanticsUpdate update) => platformDispatcher.updateSemantics(update); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as for the other SingletonFlutterWindow, just remove this orverride.
for (final FlutterView view in dispatcher.views) { | ||
view.updateSemantics(semanticsUpdate); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the other tests you online dispatched it to the first view, here its going to all views? Why this difference?
FWIW, choosing the first one everywhere is probably good enough.
3e93250
to
88c65fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -83,6 +83,14 @@ abstract class PlatformDispatcher { | |||
VoidCallback? get onAccessibilityFeaturesChanged; | |||
set onAccessibilityFeaturesChanged(VoidCallback? callback); | |||
|
|||
@Deprecated(''' | |||
In a multi-view world, the platform dispatcher can no longer provide apis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indent by two more
@@ -698,6 +698,14 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { | |||
/// In either case, this function disposes the given update, which means the | |||
/// semantics update cannot be used further. | |||
@override | |||
@Deprecated(''' | |||
In a multi-view world, the platform dispatcher can no longer provide apis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indentation
88c65fa
to
b382bf6
Compare
In an effort to move from a single-rooted flutter view to multiple views in a flutter app,
dart::ui
should be able to dispatch semantics tree updates to selected views. This PR deprecates semantics APIs that assume one flutter view per app.This patch will be succeeded by framework changes that migrate from the deprecated APIs. After, the deprecated APIs will be removed in a subsequent PR.
This is a partial fix to flutter/flutter#112221. that issue will be fully fixed when all 3 PRs are landed.
Pre-launch Checklist
writing and running engine tests.
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.