[Cupertino] More consistent of rendering of CupertinoActivityIndicator

[cbracken]

Golden tests for CupertinoActivityIndicator have been fairly flaky. See example failure here:
https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20framework_tests_libraries/28008/overview

Failing tests:
/Volumes/Work/s/w/ir/x/w/flutter/packages/flutter/test/cupertino/activity_indicator_test.dart: Activity indicator 0% in progress
/Volumes/Work/s/w/ir/x/w/flutter/packages/flutter/test/cupertino/activity_indicator_test.dart: Activity indicator 100% in progress
/Volumes/Work/s/w/ir/x/w/flutter/packages/flutter/test/cupertino/activity_indicator_test.dart: Activity indicator 30% in progress
/Volumes/Work/s/w/ir/x/w/flutter/packages/flutter/test/cupertino/activity_indicator_test.dart: Activity indicator dark mode

The previous implementation has two issues that can result in inconsistent rendering:

• Because we don't snap the centre of the indicator to a pixel boundary, we can end up with inconsistent antialiasing in Skia/Impeller drawing. We now snap to a pixel boundary which avoids introducing error due to floating point differences in the centre point between runs.
• Any floating point error in roration accumulates over time, since we build each rotation on the previous one. We now apply the cumulative rotation starting from the base co-ordinate system on each frame by applying canvas.save() and canvas.restore() so our rotation is the result of a single calculation rather than additional calculation with additional fp error on each rotation. The cost of save/restore are a single push/pop of a 4x4 matrix onto/off the stack, which is negligible.

No test changes because this is covered by existing tests; it just reduces flakiness.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the AI contribution guidelines and understand my responsibilities, or I am not using AI tools.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• I followed the breaking change policy and added Data Driven Fixes where supported.
• All existing and new tests are passing.

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-assist bot 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.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[cbracken]

I think it would be useful to support a tolerance for local golden test comparisons. Right now, it looks to me like we're looking for exact pixel values.

[cbracken]

Save the original unrotated state at the top.

Then we need to calculate the full rotation for this tick before drawing instead of applying a rotation at the end to prep for the next frame. Doing a single floating point calculation avoids building up small floating point error over time through repeated addition.

Then we restore back to the unrotated state so we can do it again next frame and not accumulate error.

[gemini-code-assist]

Code Review

This pull request updates the _CupertinoActivityIndicatorPainter to round translation coordinates to the nearest double and switches from incremental to absolute rotation for drawing ticks. A review comment suggests hoisting redundant calculations out of the loop and using an explicit integer for the loop limit to improve efficiency and prevent potential floating-point precision issues.

[gemini-code-assist]

Code Review

This pull request updates the _CupertinoActivityIndicatorPainter to round translation coordinates to the nearest double and refactors the tick drawing loop to use absolute rotation within save and restore calls. The review feedback recommends hoisting the constant rotation step calculation out of the loop to avoid redundant divisions.

[cbracken]

Sent a test exemption request on Discord since this is covered by existing tests and designed to reduce sources of floating point error so the test is less flaky.

[cbracken]

@Piinks @dkwingsmt not up to date on what the material/cupertino code freeze is. Is there a link to an issue/doc that has info?

Either way, there's not really any rush on this. This is in theory a no-op change, but it should help reduce flakes on the bots / improve developer velocity.

[flutter-dashboard]

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #186234 at sha 36dd39b

[navaronbracke]

There is #184093 , which is also linked from a banner at the top of the repo in the Github UI (it is a dismissible announcement)

The design doc: https://docs.google.com/document/d/189AbzVGpxhQczTcdfJd13o_EL36t-M5jOEt1hgBIh7w/edit?pli=1&tab=t.0

And the umbrella issue: #101479

[dkwingsmt]

@cbracken Yes, it's described in the pinned issue: #184093

Simply put, we can continue the review and push the PR to completion, then we'll turn it into a draft before merging. When the code freeze is lifted (likely June) we'll publish a guidance on how to port this change to the new packages. Thanks for the contribution and I'll review it soon!

[cbracken]

There is #184093 , which is also linked from a banner at the top of the repo in the Github UI (it is a dismissible announcement)

Thanks! Sounds like something I likely dismissed at some point.

Might be useful to submit a patch to the cocoon GitHub bot to emit the relevant info in the output, similar to how our auto-roller bots emit instructions on how to operate them in the roll PR description. I can file an issue.

EDIT: I should probably read the bot output because it's there right at the bottom 😅

[stuartmorgan-g]

test-exempt: addresses test flake

[dkwingsmt]

LGTM! The points sound very reasonable. Thank you for the investigation.

[dkwingsmt]

Let me wait for a few days and see if others have some opinions before I mark this PR as ready-to-merge-after-unfreeze.

[cbracken]

Checking in -- are we still in code freeze?

[Piinks]

Yes. When the code freeze lifts, this will not land here, it will be ported over to the new cupertino_ui package in flutter/packages.

[dkwingsmt]

Thanks for your contribution! This PR has reached completion, however we're not able to merge this PR for the moment due to the Material/Cupertino code freeze (#184093). As soon as the decoupling is complete, we will post a way to port this PR to the new packages to land. In the meantime, I'll convert this PR to a draft and apply a label so that we can quickly find it when the freeze is lifted.

[flutter-dashboard]

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[Piinks]

I've marked this PR as not ready to port to flutter/packages yet.
We'll provide instructions to move this change over to material_ui/cupertino_ui once ready to receive PRs. Thank you!

[cbracken]

@Piinks @dkwingsmt Is this still blocked, or can I move this over to material_ui/cupertino_ui now?

[navaronbracke]

According to Kate's answer in flutter/packages#12228 (comment) we still have to wait a while.

[Piinks]

We'll provide instructions to move this change over to material_ui/cupertino_ui once ready to receive PRs.

I meant what I already said. :)
We will let you know when we are ready and provide instructions. Please refrain from pinging, we have so many PRs on hold. I cannot repeat myself on all of them.