add @nonVirtual to RenderObject.attached, fix WidgetTester.hasRunningAnimations#186832
Conversation
github-actions
Bot
added
a: tests
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the attached property in RenderObject to be @nonVirtual and modifies WidgetTester.hasRunningAnimations to require a scheduled frame. It also adds a test case to verify animation state when transient callbacks do not schedule new frames. Feedback suggests reverting the use of the public owner getter to the private _owner field to strictly enforce the non-virtual constraint and recommends using idiomatic expect matchers instead of assert in the test code for better diagnostic output.
| bool get attached => _owner != null; | ||
| /// This becomes true during the call to [attach], and becomes false during the call to [detach]. | ||
| @nonVirtual | ||
| bool get attached => owner != null; |
There was a problem hiding this comment.
To strictly enforce the non-virtual nature of the attached property, it should continue to use the private field _owner instead of the public getter owner. Since owner is virtual and can be overridden by subclasses, using it here creates a loophole that allows subclasses to indirectly change the behavior of attached, which contradicts the intent of adding the @nonVirtual annotation. Using _owner ensures the property accurately reflects the internal lifecycle state managed by attach and detach.
bool get attached => _owner != null;References
- Avoid duplicating state and ensure that the source of truth for lifecycle state is the internal field managed by the lifecycle methods.
There was a problem hiding this comment.
intended escape hatch.
There was a problem hiding this comment.
Although I don't really have a use case for this escape hatch.
There was a problem hiding this comment.
I thought the right way to escape the attached is just override attached getter in the subclass? making owner null to make attached false seems like a side effect rather than intention
There was a problem hiding this comment.
From the docs I believe owner != null is necessary and sufficient for a RenderObject being attached:
/// The owner for this render object (null if unattached).
///
/// The entire render tree that this render object belongs to
/// will have the same owner.
PipelineOwner? get owner => _owner;
/// Whether the render tree this render object belongs to is attached to a [PipelineOwner].
///
/// This becomes true during the call to [attach].
///
/// This becomes false during the call to [detach].
bool get attached => _owner != null;
| assert(!tester.hasRunningAnimations); | ||
| tester.binding.scheduleFrameCallback((_) {}, scheduleNewFrame: false); | ||
| expect(tester.hasRunningAnimations, false); |
There was a problem hiding this comment.
In Flutter tests, it is more idiomatic to use expect with matchers like isFalse rather than using assert for preconditions or comparing against boolean literals. expect provides significantly better diagnostic information if the test fails and maintains consistency with the existing tests in this file (e.g., line 258).
| assert(!tester.hasRunningAnimations); | |
| tester.binding.scheduleFrameCallback((_) {}, scheduleNewFrame: false); | |
| expect(tester.hasRunningAnimations, false); | |
| expect(tester.hasRunningAnimations, isFalse); | |
| tester.binding.scheduleFrameCallback((_) {}, scheduleNewFrame: false); | |
| expect(tester.hasRunningAnimations, isFalse); |
References
- Code should follow the guidance described in the writing effective tests guide, which emphasizes the use of idiomatic matchers for clarity and better error reporting. (link)
| /// sufficient) condition to [hasRunningAnimations], as new frames can be | ||
| /// scheduled when widget calls [State.setState], even if there are no | ||
| /// transient callbacks scheduled. This is what [pumpAndSettle] uses. | ||
| bool get hasRunningAnimations => binding.hasScheduledFrame && binding.transientCallbackCount > 0; |
There was a problem hiding this comment.
I am a bit confused. why does hasRunningAnimations cares about hasScheduledFrame? It just won't build new frame, but the there is an animation running.
Not saying that this is a legit situation, just a question for my own understanding
There was a problem hiding this comment.
Animations should always schedule new frames otherwise it's not "running", as in, the user won't see UI changes until a new frame is presented (unless you're using shaders but that doesn't really count as "animation" by this getter anyways).
This getter does not account for animations driven by Timers but even those must schedule new frames.
Before #164034 transient callback count > 0 implies there is a new frame scheduled, but #164034 added a new flag to allow new callbacks to be added without scheduling a new frame, so now the flag is reporting false positives when I use OverlayPortal.childLayoutBuilder.
There was a problem hiding this comment.
The code here LGTM then.
I was looking at the https://github.com/flutter/flutter/pull/164034/changes#diff-4c298197a8aa7831a26eece096c8b8b07773ba1f5376d848ea4ef2924b606e9f.
Looks like the scheduleNewFrame is introduced for just for testing purposes?
There was a problem hiding this comment.
No it was a new mechanism that allows app devs to add "onNewFrame" callbacks. It's basically a user callback that gets called in handleBeginFrame without scheduling a new frame.
|
my last concern is here #186832 (comment) |
LongCatIsLooong
added
the
autosubmit
|
autosubmit label was removed for flutter/flutter/186832, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
auto-submit
Bot
removed
the
autosubmit
LongCatIsLooong
added
CICD
flutter-dashboard
Bot
removed
the
autosubmit
Roll Flutter from fc1ad955f164 to 8005793c3562 (40 revisions) flutter/flutter@fc1ad95...8005793 2026-07-18 [email protected] Remove `LineContents ` experimental AA line shader that is no longer used (flutter/flutter#189619) 2026-07-18 [email protected] Roll Skia from 9468e96cc40f to ba90f98535de (8 revisions) (flutter/flutter#189680) 2026-07-17 [email protected] Roll Dart SDK from 33e4b71e984b to 666e1e2133b7 (2 revisions) (flutter/flutter#189673) 2026-07-17 [email protected] [iOS] Fix potential use-after-free in a11y bridge channel handler (flutter/flutter#189637) 2026-07-17 [email protected] Migrate dev/tools skill validation to new dart_skills_lint API (flutter/flutter#189626) 2026-07-17 [email protected] Add validation for required fields during xcodebuild (flutter/flutter#187772) 2026-07-17 [email protected] Upgrade android_hardware_smoke_test CI to run instrumented tests (flutter/flutter#189390) 2026-07-17 [email protected] Roll Dart SDK from 0867cb1897b5 to 33e4b71e984b (1 revision) (flutter/flutter#189661) 2026-07-17 [email protected] Roll Skia from 702c3e790232 to 9468e96cc40f (1 revision) (flutter/flutter#189660) 2026-07-17 [email protected] Roll Packages from 9f95026 to 4fdc766 (11 revisions) (flutter/flutter#189659) 2026-07-17 [email protected] Roll Skia from 2e4a3ae035cd to 702c3e790232 (1 revision) (flutter/flutter#189655) 2026-07-17 [email protected] [macOS] Add FlutterPluginRegistrar.valuePublished(byPlugin:) just like iOS (flutter/flutter#189614) 2026-07-17 [email protected] Roll Dart SDK from c69d138c9646 to 0867cb1897b5 (2 revisions) (flutter/flutter#189649) 2026-07-17 [email protected] Roll Skia from 4f5aca109c87 to 2e4a3ae035cd (3 revisions) (flutter/flutter#189646) 2026-07-17 [email protected] [iOS] Fix missing nil checks and improve SemanticsObject bridge API (flutter/flutter#189630) 2026-07-17 [email protected] Roll Skia from 37c5e6b26aee to 4f5aca109c87 (19 revisions) (flutter/flutter#189638) 2026-07-17 [email protected] Roll Fuchsia Linux SDK from lLFbh5kFWbUGgC9Ek... to NL8xtzr8cxr5E8r8E... (flutter/flutter#189632) 2026-07-17 [email protected] Add blendMode parameter to RawImage and RenderImage (flutter/flutter#185938) 2026-07-17 [email protected] [web] Cache WASM network requests in flutter test (flutter/flutter#189623) 2026-07-17 [email protected] android_hardware_smoke_tests: Apply semantic line breaks to readme (flutter/flutter#189613) 2026-07-16 [email protected] ci(github-actions): resolve zizmor github-env findings in composite flutter actions (flutter/flutter#189602) 2026-07-16 [email protected] Remove obsolete packages analysis flag (flutter/flutter#189525) 2026-07-16 [email protected] Roll Dart SDK from e24870ff15bc to c69d138c9646 (2 revisions) (flutter/flutter#189597) 2026-07-16 [email protected] [fuchsia][iwyu] Remove transitive include of fuchsia.input.report. (flutter/flutter#188891) 2026-07-16 [email protected] Remove unnecessary value key hack in `key.dart` (flutter/flutter#189291) 2026-07-16 [email protected] Roll Dart SDK from 81306a2ed317 to e24870ff15bc (1 revision) (flutter/flutter#189569) 2026-07-16 [email protected] [Impeller] Fix atlas growth test (flutter/flutter#189533) 2026-07-16 [email protected] Roll Dart SDK from d402ff7c9c84 to 81306a2ed317 (4 revisions) (flutter/flutter#189558) 2026-07-16 [email protected] Move shared darwin plugin tests into own builder (flutter/flutter#189411) 2026-07-16 [email protected] Roll Skia from ab2410bc857c to 37c5e6b26aee (19 revisions) (flutter/flutter#189549) 2026-07-16 [email protected] [ios] Fix //flutter:unittests build for physical devices (flutter/flutter#189543) 2026-07-16 [email protected] Fix lower DragTarget not being recognized in overlapping targets (flutter/flutter#188979) 2026-07-16 [email protected] Relocate cupertino samples that were under widgets/ (flutter/flutter#188876) 2026-07-15 [email protected] [iOS] Migrate DisplayLinkManager to a shared instance (flutter/flutter#189492) 2026-07-15 [email protected] [Impeller] Call glfwTerminate during global test environment teardown if a playground test called glfwInit (flutter/flutter#189523) 2026-07-15 [email protected] Roll pub packages (flutter/flutter#189515) 2026-07-15 [email protected] Roll Packages from ad2eab1 to 9f95026 (8 revisions) (flutter/flutter#189509) 2026-07-15 [email protected] add `@nonVirtual` to `RenderObject.attached`, fix `WidgetTester.hasRunningAnimations` (flutter/flutter#186832) 2026-07-15 [email protected] Roll vulkan-deps to 0582f446e54a (flutter/flutter#188524) 2026-07-15 [email protected] Roll Dart SDK from 0c408ff6dce9 to d402ff7c9c84 (1 revision) (flutter/flutter#189497) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. ...
2 participants
attachedmust be set to true inattachand set to false indetach, subclasses must not override it to change the behavior. It is still possible to overrideownerto affectattachedbut that is also a bit dubious.#164034 added a
scheduleNewFrameflag to the add transient callback API which essentially allows the user to register a callback when a new frame is scheduled (by setting the flag to true). But it didn't update theWidgetTester.hasRunningAnimationsimplementation to reflect that change, as it allows transient callbacks to be scheduled without scheduling a new frame (so the API will report there are animations when there is none).Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.