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
Introduce the PipelineOwner tree #122231
Introduce the PipelineOwner tree #122231
Conversation
@@ -859,6 +859,20 @@ class _LocalSemanticsHandle implements SemanticsHandle { | |||
/// are visible on screen. You can create other pipeline owners to manage | |||
/// off-screen objects, which can flush their pipelines independently of the | |||
/// on-screen render objects. | |||
/// | |||
/// [PipelineOwner]s can be organized in a tree to manage multiple render trees, |
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.
/// [PipelineOwner]s can be organized in a tree to manage multiple render trees, | |
/// [PipelineOwner]s are organized in a tree to manage multiple render trees, |
Aren't they always arranged this way (even a single main view can have subviews).
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.
I wanted to express here that the PipelineOwner class can also be used stand-alone. It fully functions if it is not part of a tree, for example for the use case mentioned above where you create another pipeline owner to manage off-screen objects. You are not required to turn them into a tree or attach them to any (existing) tree. Of course, one could argue, that just having a single node (the root) is also a form of tree...
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.
Oh, OK, ignore my comment then.
Of course, one could argue, that just having a single node (the root) is also a form of tree...
We don't need to be as pedantic as all that. :-)
@@ -945,6 +967,7 @@ class PipelineOwner { | |||
/// always returns false. | |||
bool get debugDoingLayout => _debugDoingLayout; | |||
bool _debugDoingLayout = false; | |||
bool _debugDoingChildrenLayout = false; |
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.
bool _debugDoingChildrenLayout = false; | |
bool _debugDoingChildLayout = false; |
...because it just reads better. I'd either say "I'm doing child layout" or "I'm laying out children", but not "I'm doing children layout".
/// [semanticsOwner] field will revert to null, and the previous owner will be | ||
/// disposed. | ||
/// An owner is created by [ensureSemantics] or when the [PipelineManifold] to | ||
/// which this owner is connected to has [PipelineManifold.enableSemantics] |
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.
/// which this owner is connected to has [PipelineManifold.enableSemantics] | |
/// which this owner is connected has [PipelineManifold.enableSemantics] |
_updateSemanticsOwner(); | ||
|
||
// If onNeedVisualUpdate is specified, it has already been called when the node was dirtied in the first place. | ||
if (onNeedVisualUpdate == null && (_nodesNeedingLayout.isNotEmpty || _nodesNeedingCompositingBitsUpdate.isNotEmpty || _nodesNeedingPaint.isNotEmpty || _nodesNeedingSemantics.isNotEmpty)) { |
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.
Maybe wrap this line
/// Adds `child` to this [PipelineOwner]. | ||
/// | ||
/// During the phases of frame production (see [RendererBinding.drawFrame]), | ||
/// the parent [PipelineOwner] will complete a phase first for the nodes it |
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.
/// the parent [PipelineOwner] will complete a phase first for the nodes it | |
/// the parent [PipelineOwner] will complete a phase for the nodes it |
/// | ||
/// No children may be removed after the [PipelineOwner] has started calling | ||
/// [flushLayout] on any of its children until the end of the current frame. | ||
void dropChild(PipelineOwner child) { |
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.
Why use adopt/drop? They're not really opposites of each other. Why not addChild
/removeChild
? If you want to use adopt, then the opposite is probably "abandon". Which feels a bit bleak. :-)
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.
That's the terminology we use for every other tree node class in the framework:
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.
Sigh. Okay, consistency is important, but it's too bad that they aren't opposites.
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.
Agreed!
/// | ||
/// [PipelineOwner]s can register listeners with the [PipelineManifold] to be | ||
/// informed when certain values provided by the [PipelineManifold] change. | ||
abstract class PipelineManifold implements Listenable { |
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.
I really like that name.
/// particular binding implementation. | ||
/// | ||
/// The root of the [PipelineOwner] tree is attached to a [PipelineManifold] | ||
/// by passing it to [PipelineOwner.attach]. Children are attached to the same |
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.
/// by passing it to [PipelineOwner.attach]. Children are attached to the same | |
/// by passing the manifold to [PipelineOwner.attach]. Children are attached to the same |
/// implementations typically use to back this property. | ||
bool get semanticsEnabled; | ||
|
||
/// Called be a [PipelineOwner] connected to this [PipelineManifold] when a |
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.
/// Called be a [PipelineOwner] connected to this [PipelineManifold] when a | |
/// Called by a [PipelineOwner] connected to this [PipelineManifold] when a |
bool get semanticsEnabled; | ||
|
||
/// Called be a [PipelineOwner] connected to this [PipelineManifold] when a | ||
/// render object associated with that pipeline owner wishes to update its |
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.
maybe link RenderObject?
/// render object associated with that pipeline owner wishes to update its | |
/// [RenderObject] associated with that pipeline owner wishes to update its |
0889d78
to
e1220e7
Compare
448e790
to
fb46684
Compare
All comments are addressed and all failing checks should pass now. PTAL @gspencergoog. |
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.
83448f5
to
a514117
Compare
It looks like this PR is causing failures in the tree: https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20web_canvaskit_tests_7_last/7125/overview |
Roll Flutter Engine from de724e263264 to b4297f3cc886 (7 revisions) (flutter#121991) Roll Flutter Engine from de724e263264 to b4297f3cc886 (7 revisions) 54b989378 Roll Skia from 5ce2918379b4 to 6ab9a7f46467 (1 revision) (flutter/engine#40081) (flutter#121995) Roll Flutter Engine from b4297f3cc886 to 54b989378249 (1 revision) f8ec13dad Roll Skia from 6ab9a7f46467 to ab12a43ea332 (4 revisions) (flutter/engine#40082) (flutter#122000) Roll Flutter Engine from 54b989378249 to f8ec13dade7c (1 revision) 197b6dae1 [Impeller] Round glyph locations per-run to fix per-glyph jitter (flutter/engine#40073) (flutter#122002) Roll Flutter Engine from f8ec13dade7c to 197b6dae1c31 (1 revision) Use `dart pub` instead of `dart __deprecated pub` (flutter#121605) Bottom appbar/sheet shadow property (flutter#121406) * add shadowColor property * add to bottom app bar * add test * update m2/m3 diffs * reorder debug test * finalize * remove crswap * update doc comments * add m2 shadow back * add const * update docs * update docs * comment replies * make param non-null * indentation fix * doc fix b6db201ea Roll Skia from ab12a43ea332 to d43c1424807e (2 revisions) (flutter/engine#40084) (flutter#122018) Roll Flutter Engine from 197b6dae1c31 to b6db201ea32f (1 revision) Fix license page title color issues (flutter#121872) [tool] Proposal to multiple defines for --dart-define-from-file (flutter#120878) [tool] Proposal to multiple defines for --dart-define-from-file Add missing properties to `ListTileTheme.merge` (flutter#121975) Add missing properties to `ListTileTheme.merge` Removes single window assumptions from `flutter_test` (flutter#121549) Removes single window assumptions from `flutter_test` Roll Packages from ca7205b9c541 to 789e3a72c9d4 (7 revisions) (flutter#122026) Roll Packages from ca7205b9c541 to 789e3a72c9d4 (7 revisions) Revert "Removes single window assumptions from `flutter_test` (flutter#121549)" (flutter#122037) This reverts commit f2dd19d. e2bd89f97 Roll Skia from d43c1424807e to 7e1756b42f94 (1 revision) (flutter/engine#40087) (flutter#122033) Roll Flutter Engine from b6db201ea32f to e2bd89f97e7f (1 revision) Reland "Speed up first asset load by using the binary-formatted asset manifest for image resolution (flutter#121322) Reland "Speed up first asset load by using the binary-formatted asset manifest for image resolution Update test font (flutter#121306) Update test font Add backward compatibility in proxied_devices. (flutter#122040) Add backward compatibility in proxied_devices. pin flutter_plugin_android_lifecycle and roll other pub deps (flutter#122043) pin flutter_plugin_android_lifecycle and roll other pub deps Revert "Update test font (flutter#121306)" (flutter#122053) This reverts commit 9f69a36. Adds vmservices for getting iOS build options (flutter#121736) Adds vmservices for getting iOS build options Add ZoomPageTransitionsBuilder.allowSnapshotting (flutter#122019) Add ZoomPageTransitionsBuilder.allowSnapshotting [web] delete unhelpful image loading e2e test (flutter#122059) [web] delete unhelpful image loading e2e test Fix PlatformMenuItems with onSelectedIntent are never enabled (flutter#121885) Fix PlatformMenuItems with onSelectedIntent are never enabled Roll Flutter Engine from e2bd89f97e7f to 1c4aab7da9f0 (15 revisions) (flutter#122078) Roll Flutter Engine from e2bd89f97e7f to 1c4aab7da9f0 (15 revisions) Roll Flutter Engine from 1c4aab7da9f0 to 4e6cbe0fc22d (4 revisions) (flutter#122094) Roll Flutter Engine from 1c4aab7da9f0 to 4e6cbe0fc22d (4 revisions) [integration_test] upgrade androidx test to 1.4.0 (flutter#109547) [integration_test] upgrade androidx test to 1.4.0 Revert "[integration_test] upgrade androidx test to 1.4.0 (flutter#109547)" (flutter#122106) Revert "[integration_test] upgrade androidx test to 1.4.0" c40f1893e [camera_android] Fix camera android deprecation warning for CamcorderProfile.get() (flutter/packages#3273) (flutter#122104) Roll Packages from 789e3a72c9d4 to c40f1893ef38 (1 revision) Roll Flutter Engine from 4e6cbe0fc22d to b3985c31295f (2 revisions) (flutter#122109) Roll Flutter Engine from 4e6cbe0fc22d to b3985c31295f (2 revisions) 811dcfb46 Roll Skia from 5ff664171376 to ec96c2172789 (4 revisions) (flutter/engine#40120) (flutter#122121) Roll Flutter Engine from b3985c31295f to 811dcfb46101 (1 revision) Roll Flutter Engine from 811dcfb46101 to 8510f554d3fa (3 revisions) (flutter#122126) Roll Flutter Engine from 811dcfb46101 to 8510f554d3fa (3 revisions) Reland: Removes single window assumptions from `flutter_test` (flutter#122060) Reland: Removes single window assumptions from `flutter_test` [web] Ensure CanvasKit is served from the correct location (flutter#121902) [web] Ensure CanvasKit is served from the correct location Roll Flutter Engine from 8510f554d3fa to 89627fda3055 (5 revisions) (flutter#122135) Roll Flutter Engine from 8510f554d3fa to 89627fda3055 (5 revisions) Use tearoffs (flutter#122091) Use tearoffs in TestRenderingFlutterBinding Add printing on failure to web hotrestart tests (flutter#122115) cc0fdef39 [web][felt] Add 'archive' target + 'profile' mode (flutter/engine#40060) (flutter#122140) Roll Flutter Engine from 89627fda3055 to cc0fdef39f25 (1 revision) delete `FlutterCommand.intArg()`, which is not used anywhere in the codebase (flutter#122124) Delete `FlutterCommand.intArg()`, which is not used anywhere Marks Mac_ios hot_mode_dev_cycle_ios__benchmark to be flaky (flutter#120809) Marks Mac_ios hot_mode_dev_cycle_ios__benchmark to be flaky Fix typo in `integrationDriver()` function (flutter#115012) Fix typo in `integrationDriver()` function Roll Flutter Engine from cc0fdef39f25 to ab28c582f4f4 (4 revisions) (flutter#122152) Roll Flutter Engine from cc0fdef39f25 to ab28c582f4f4 (4 revisions) Fix DomCSSStyleSheetExtension to work with dart2wasm. (flutter#122154) Fix DomCSSStyleSheetExtension to work with dart2wasm. Roll Flutter Engine from ab28c582f4f4 to 694a14ca6e15 (3 revisions) (flutter#122158) Roll Flutter Engine from ab28c582f4f4 to 694a14ca6e15 (3 revisions) Revert "Roll Flutter Engine from ab28c582f4f4 to 694a14ca6e15 (3 revisions) (flutter#122158)" (flutter#122217) This reverts commit 5a279ed. ModalBottomSheetRoute: Remove gap at screen bottom with `useSafeArea: true` (flutter#122118) ModalBottomSheetRoute: Remove gap at screen bottom with `useSafeArea: true` Revert "Reland: Removes single window assumptions from `flutter_test` (flutter#122060)" (flutter#122193) Revert "Reland: Removes single window assumptions from `flutter_test`" Clear _scribbleCacheKey when connection closes (flutter#122145) Clear _scribbleCacheKey when connection closes Roll Packages from c40f1893ef38 to a4d3d16029a7 (11 revisions) (flutter#122191) Roll Packages from c40f1893ef38 to a4d3d16029a7 (11 revisions) Run plugin_test_macos on Intel machines (flutter#122212) Run plugin_test_macos on Intel machines Funnel devicelab tests through utils process methods (flutter#122161) Funnel devicelab tests through utils process methods Add support for iOS UndoManager (flutter#98294) Add support for iOS UndoManager Revert "[web:tools] always use CanvasKit from the cache when building web apps (flutter#93002)" (flutter#117693) Revert "[web:tools] always use CanvasKit from the cache when building web apps (flutter#93002)" Create configOnly flag for android (flutter#121904) Create configOnly flag for android Update cmake version to the latest in infra/3pp/tools (flutter#122147) Update cmake version to the latest in infra/3pp/tools SystemUiOverlayStyle, add examples and improve documentation (flutter#122187) SystemUiOverlayStyle, add two examples and improve documentation Use type promotion instead of access through a map (flutter#122178) Use variable instead of multiple accesses through a map Add one DefaultTextStyle example (flutter#122182) Add one DefaultTextStyle example Marks Windows plugin_test to be flaky (flutter#122201) Marks Windows plugin_test to be flaky No friction factor on macOS overscroll ease (flutter#122143) No friction factor on macOS overscroll ease [Impeller] Temporary flag flip for devicelab tests to use Impeller. (flutter#122224) [Impeller] Temporary flag flip for devicelab tests to use Impeller. Revert "[Impeller] Temporary flag flip for devicelab tests to use Impeller. (flutter#122224)" (flutter#122236) This reverts commit 21b8b72. Roll Flutter Engine from ab28c582f4f4 to 098fd5884e55 (15 revisions) (flutter#122234) Roll Flutter Engine from ab28c582f4f4 to 098fd5884e55 (15 revisions) Fix GestureRecognizer allowedButtonsFilter. (flutter#122227) fix a scroll bar stutter bug (flutter#121786) Fix a scrolling stutter caused by dragging scrollbar Enable plugin_test_linux (flutter#120912) Enable plugin_test_linux Reland (2): Removes single window assumptions from `flutter_test` (flutter#122233) Reland (2): Removes single window assumptions from `flutter_test` Roll Flutter Engine from 098fd5884e55 to 347a4f3b6892 (6 revisions) (flutter#122254) Roll Flutter Engine from 098fd5884e55 to 347a4f3b6892 (6 revisions) Roll Flutter Engine from 347a4f3b6892 to a996e1d70af2 (3 revisions) (flutter#122260) Roll Flutter Engine from 347a4f3b6892 to a996e1d70af2 (3 revisions) Roll Flutter Engine from a996e1d70af2 to 7e1b3a98d114 (2 revisions) (flutter#122262) Roll Flutter Engine from a996e1d70af2 to 7e1b3a98d114 (2 revisions) 78ff6a093 [Impeller][Compute] Guard subgroups with feature detection (flutter/engine#40159) (flutter#122265) Roll Flutter Engine from 7e1b3a98d114 to 78ff6a093411 (1 revision) Roll Flutter Engine from 78ff6a093411 to 54066d919a20 (2 revisions) (flutter#122272) Roll Flutter Engine from 78ff6a093411 to 54066d919a20 (2 revisions) a82207cee Roll Skia from 40065b435865 to e9471b0a9286 (1 revision) (flutter/engine#40172) (flutter#122288) Roll Flutter Engine from 54066d919a20 to a82207cee323 (1 revision) Roll Packages from a4d3d16029a7 to 3b29183e6f09 (10 revisions) (flutter#122304) Roll Packages from a4d3d16029a7 to 3b29183e6f09 (10 revisions) Roll Flutter Engine from a82207cee323 to 354503a6a545 (2 revisions) (flutter#122308) Roll Flutter Engine from a82207cee323 to 354503a6a545 (2 revisions) Feat : `TextField` cursor color matching M2 and M3 Spec in error state (flutter#119225) According to Material specs, cursor should be red in error state. Reland "Update test font (flutter#121306)" (flutter#122068) Reland "Update test font (flutter#121306)" Remove references to BindingBase.window (flutter#122119) Remove references to BindingBase.window Enable invalid_case_patterns lint (flutter#122318) Enable invalid_case_patterns lint Roll Flutter Engine from 354503a6a545 to 852b52c364e6 (6 revisions) (flutter#122326) Roll Flutter Engine from 354503a6a545 to 852b52c364e6 (6 revisions) Improve Dart plugin registration handling (flutter#122046) Improve Dart plugin registration handling Roll Flutter Engine from 852b52c364e6 to 832298bdd707 (3 revisions) (flutter#122330) Roll Flutter Engine from 852b52c364e6 to 832298bdd707 (3 revisions) [flutter_tools] Add namespace getter in Android project; use namespace as fallback (flutter#121416) [flutter_tools] Add namespace getter in Android project; use namespace as fallback Updates `flutter/test/cupertino` to no longer use `TestWindow` (flutter#122325) Updates `flutter/test/cupertino` to no longer use `TestWindow` Updates `flutter/test/gestures` to no longer reference `TestWindow` (flutter#122327) Updates `flutter/test/gestures` to no longer reference `TestWindow` Updates `flutter_localizations/test` to stop using `TestWindow` (flutter#122321) Updates `flutter_localizations/test` to stop using `TestWindow` fix devtool instructional messages after `flutter build ... --analyze-size `. Fixes flutter#122229 (flutter#122230) fix devtool instructional messages after `flutter build ... --analyze-size `. Fixes flutter#122229 Roll Flutter Engine from 832298bdd707 to 15a9ec7291fd (3 revisions) (flutter#122335) Roll Flutter Engine from 832298bdd707 to 15a9ec7291fd (3 revisions) Roll Flutter Engine from 15a9ec7291fd to 71c81ef9dc90 (3 revisions) (flutter#122343) Roll Flutter Engine from 15a9ec7291fd to 71c81ef9dc90 (3 revisions) Remove another reference to BindingBase.window (flutter#122341) Remove another reference to BindingBase.window [Impeller] Temporary flag flip for devicelab tests to use Impeller redux. (flutter#122340) Updates `flutter/test/rendering` to no longer use `TestWindow` (flutter#122347) Updates `flutter/test/rendering` to no longer use `TestWindow` Document on ScrollPhysics the requirement to override applyTo (flutter#121850) Document on ScrollPhysics the requirement to override applyTo Remove single view assumption from TestViewConfiguration (flutter#122352) Remove single view assumption from TestViewConfiguration Roll Flutter Engine from 71c81ef9dc90 to e323b11a5659 (4 revisions) (flutter#122359) Roll Flutter Engine from 71c81ef9dc90 to e323b11a5659 (4 revisions) Roll Flutter Engine from e323b11a5659 to e9ca7b2c457b (2 revisions) (flutter#122361) Roll Flutter Engine from e323b11a5659 to e9ca7b2c457b (2 revisions) Revert PRs relating to single window assumption (flutter#122369) * Revert "Remove references to BindingBase.window (flutter#122119)" This reverts commit c7681f0. * Revert "Remove another reference to BindingBase.window (flutter#122341)" This reverts commit 6ec4445. * Revert "Reland (2): Removes single window assumptions from `flutter_test` (flutter#122233)" This reverts commit eb3d317. * Revert "Remove single view assumption from TestViewConfiguration (flutter#122352)" This reverts commit 927289f. * Revert "Updates `flutter/test/cupertino` to no longer use `TestWindow` (flutter#122325)" This reverts commit 67e17e4. * Revert "Updates `flutter/test/gestures` to no longer reference `TestWindow` (flutter#122327)" This reverts commit c2a5111. * Revert "Updates `flutter/test/rendering` to no longer use `TestWindow` (flutter#122347)" This reverts commit 28b65e0. * Revert "Updates `flutter_localizations/test` to stop using `TestWindow` (flutter#122321)" This reverts commit 01367d5. Roll Packages from 3b29183e6f09 to b6668e4ef35f (4 revisions) (flutter#122403) Roll Packages from 3b29183e6f09 to b6668e4ef35f (4 revisions) Revert "[Impeller] Temporary flag flip for devicelab tests to use Impeller redux. (flutter#122340)" (flutter#122362) Revert "[Impeller] Temporary flag flip for devicelab tests to use Impeller redux." Adjust the minimum sdk version in the synthetic pkg for new Dart requirements (flutter#122380) Manually landing to fix the Engine rolls. The change will not affect Google Testing. Fix Gradle 7 warnings that are now errors in Gradle 8 (flutter#121958) Fix Gradle 7 warnings that are now errors in Gradle 8 SelectionChangedCause for iOS keyboard-select (flutter#122144) SelectionChangedCause for iOS keyboard-select Reland "Remove single view assumption from TestViewConfiguration (flutter#122352)" (flutter#122414) Reland "Remove single view assumption from TestViewConfiguration (flutter#122352)" Introduce the PipelineOwner tree (flutter#122231) Introduce the PipelineOwner tree Revert "Introduce the PipelineOwner tree (flutter#122231)" (flutter#122425) This reverts commit f73c358. Clean up scrollable.dart for 2D (flutter#122357) Clean up scrollable.dart for 2D Constrain date picker to max width to avoid bending outwards (flutter#120827) Constrain date picker to max width to avoid bending outwards Update sdk version in temp testing package (flutter#122423) Manual roll Flutter Engine from e9ca7b2c457b to 7572fe5b9226 (16 revisions) (flutter#122430) Manual roll Flutter Engine from e9ca7b2c457b to 7572fe5b9226 (16 revisions)
Introduce the PipelineOwner tree
This reverts commit 670f9d2.
Part of #121573.
Implements the PipelineOwner tree as specified in https://flutter.dev/go/multiple-views.