Skip to content

Conversation

@justinmc
Copy link
Contributor

@justinmc justinmc commented Jun 25, 2025

A tricky bug was encountered in https://github.com/flutter/flutter/pull/170321/files#r2138619243 where mutating Paint caused paints to behave strangely.

Originally this PR just added something to the docs for paints to give people a heads up, but now it fixes the root cause. Yak shave 🪒 .

Fixes #171194

@justinmc justinmc self-assigned this Jun 25, 2025
@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Jun 25, 2025
@justinmc justinmc mentioned this pull request Jun 25, 2025
9 tasks
///
/// This matcher does not work well when [Paint] instances are mutated and
/// reused. Create a new instance of [Paint] for subsequent draw calls instead
/// of mutating and reusing one instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

reusing the paint is fine, but it is just our TestRecordingCanvas can't handle it. I think we should fix the TestRecordingCanvas instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, that should be doable. I'll take a look.

@github-actions github-actions bot added the f: material design flutter/packages/flutter/material repository. label Jun 25, 2025
@justinmc justinmc changed the title Docs: Add a warning about mutating Paint Fix mutating Paint bug Jun 25, 2025
@flutter-dashboard
Copy link

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.

This only fixes it for drawCircle. I'll have to implement all the other
canvas calls.
@github-actions github-actions bot removed the f: material design flutter/packages/flutter/material repository. label Jun 25, 2025
@justinmc
Copy link
Contributor Author

This will require adding similar changes for all of the other methods in Canvas. Moving to draft for now.

@justinmc justinmc marked this pull request as draft June 25, 2025 22:42
@justinmc justinmc marked this pull request as draft June 25, 2025 22:42
@justinmc justinmc requested a review from chunhtai July 14, 2025 22:10
@justinmc justinmc marked this pull request as ready for review July 14, 2025 22:10
@justinmc
Copy link
Contributor Author

This is ready for re-review. Now it solves the problem for every Canvas method that uses Paint.

@github-actions github-actions bot added the f: cupertino flutter/packages/flutter/cupertino repository label Jul 16, 2025
@github-actions github-actions bot added the f: material design flutter/packages/flutter/material repository. label Jul 16, 2025
@justinmc justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 17, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Jul 17, 2025
Merged via the queue into flutter:master with commit fa961b7 Jul 17, 2025
73 of 74 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 17, 2025
@justinmc justinmc deleted the mutable-paint branch July 17, 2025 20:18
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2025
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jul 18, 2025
flutter/flutter@9c626d9...440713c

2025-07-18 [email protected] Add OS version support section to plugin docs (flutter/flutter#172316)
2025-07-18 [email protected] Roll Dart SDK from 7fcc5f48c335 to fe2232bd5b9f (2 revisions) (flutter/flutter#172339)
2025-07-18 [email protected] Refactor handling of frame waiting. (flutter/flutter#172277)
2025-07-18 [email protected] Roll pub packages (flutter/flutter#172332)
2025-07-17 [email protected] Refactor and forbid `base/exit.dart` outside of `lib/runner.dart` (flutter/flutter#171923)
2025-07-17 [email protected] Use UTC timezone for `stamp_command_test` to avoid local TZ flakes (flutter/flutter#172319)
2025-07-17 [email protected] [web] Add tests for unified platform view embedding behavior (flutter/flutter#172313)
2025-07-17 [email protected] Platform views shouldn't receive pointer events when not laid out (flutter/flutter#172043)
2025-07-17 [email protected] Fix Size.isEmpty description (flutter/flutter#172021)
2025-07-17 [email protected] feat: Add `maxCount` parameter to `Badge.count` constructor. (flutter/flutter#171054)
2025-07-17 [email protected] add `debugPaintTextLayoutBoxes` for debugging text layout (flutter/flutter#168876)
2025-07-17 [email protected] Roll Packages from 4a231ae to cb8fef6 (31 revisions) (flutter/flutter#172303)
2025-07-17 [email protected] Fix mutating Paint bug (flutter/flutter#171180)
2025-07-17 [email protected] No SystemContextMenu when readOnly is true (flutter/flutter#171242)
2025-07-17 [email protected] Add a getter to get the recorder used by painting context (flutter/flutter#170223)
2025-07-17 [email protected] [Impeller] Add missing SetPipeline call for the vertices uber pipeline in AtlasContents (flutter/flutter#172273)
2025-07-17 [email protected] Roll Dart SDK from 486f9c0663bc to 7fcc5f48c335 (1 revision) (flutter/flutter#172279)
2025-07-17 [email protected] Roll Skia from 3673a1f26a63 to 9587301e33bc (3 revisions) (flutter/flutter#172281)
2025-07-17 [email protected] Fix a race in FlutterEngineTest.CanLogToStdout (flutter/flutter#172025)

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.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
azatech pushed a commit to azatech/flutter that referenced this pull request Jul 28, 2025
A tricky bug was encountered in
https://github.com/flutter/flutter/pull/170321/files#r2138619243 where
mutating Paint caused
[paints](https://main-api.flutter.dev/flutter/flutter_test/paints.html)
to behave strangely.

Originally this PR just added something to the docs for `paints` to give
people a heads up, but now it fixes the root cause. Yak shave 🪒 .

Fixes flutter#171194
vashworth pushed a commit to vashworth/packages that referenced this pull request Jul 30, 2025
…r#9646)

flutter/flutter@9c626d9...440713c

2025-07-18 [email protected] Add OS version support section to plugin docs (flutter/flutter#172316)
2025-07-18 [email protected] Roll Dart SDK from 7fcc5f48c335 to fe2232bd5b9f (2 revisions) (flutter/flutter#172339)
2025-07-18 [email protected] Refactor handling of frame waiting. (flutter/flutter#172277)
2025-07-18 [email protected] Roll pub packages (flutter/flutter#172332)
2025-07-17 [email protected] Refactor and forbid `base/exit.dart` outside of `lib/runner.dart` (flutter/flutter#171923)
2025-07-17 [email protected] Use UTC timezone for `stamp_command_test` to avoid local TZ flakes (flutter/flutter#172319)
2025-07-17 [email protected] [web] Add tests for unified platform view embedding behavior (flutter/flutter#172313)
2025-07-17 [email protected] Platform views shouldn't receive pointer events when not laid out (flutter/flutter#172043)
2025-07-17 [email protected] Fix Size.isEmpty description (flutter/flutter#172021)
2025-07-17 [email protected] feat: Add `maxCount` parameter to `Badge.count` constructor. (flutter/flutter#171054)
2025-07-17 [email protected] add `debugPaintTextLayoutBoxes` for debugging text layout (flutter/flutter#168876)
2025-07-17 [email protected] Roll Packages from 4a231ae to cb8fef6 (31 revisions) (flutter/flutter#172303)
2025-07-17 [email protected] Fix mutating Paint bug (flutter/flutter#171180)
2025-07-17 [email protected] No SystemContextMenu when readOnly is true (flutter/flutter#171242)
2025-07-17 [email protected] Add a getter to get the recorder used by painting context (flutter/flutter#170223)
2025-07-17 [email protected] [Impeller] Add missing SetPipeline call for the vertices uber pipeline in AtlasContents (flutter/flutter#172273)
2025-07-17 [email protected] Roll Dart SDK from 486f9c0663bc to 7fcc5f48c335 (1 revision) (flutter/flutter#172279)
2025-07-17 [email protected] Roll Skia from 3673a1f26a63 to 9587301e33bc (3 revisions) (flutter/flutter#172281)
2025-07-17 [email protected] Fix a race in FlutterEngineTest.CanLogToStdout (flutter/flutter#172025)

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.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
ksokolovskyi pushed a commit to ksokolovskyi/flutter that referenced this pull request Aug 19, 2025
A tricky bug was encountered in
https://github.com/flutter/flutter/pull/170321/files#r2138619243 where
mutating Paint caused
[paints](https://main-api.flutter.dev/flutter/flutter_test/paints.html)
to behave strangely.

Originally this PR just added something to the docs for `paints` to give
people a heads up, but now it fixes the root cause. Yak shave 🪒 .

Fixes flutter#171194
mboetger pushed a commit to mboetger/flutter that referenced this pull request Sep 18, 2025
A tricky bug was encountered in
https://github.com/flutter/flutter/pull/170321/files#r2138619243 where
mutating Paint caused
[paints](https://main-api.flutter.dev/flutter/flutter_test/paints.html)
to behave strangely.

Originally this PR just added something to the docs for `paints` to give
people a heads up, but now it fixes the root cause. Yak shave 🪒 .

Fixes flutter#171194
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 12, 2025
lucaantonelli pushed a commit to lucaantonelli/flutter that referenced this pull request Nov 21, 2025
A tricky bug was encountered in
https://github.com/flutter/flutter/pull/170321/files#r2138619243 where
mutating Paint caused
[paints](https://main-api.flutter.dev/flutter/flutter_test/paints.html)
to behave strangely.

Originally this PR just added something to the docs for `paints` to give
people a heads up, but now it fixes the root cause. Yak shave 🪒 .

Fixes flutter#171194
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: tests "flutter test", flutter_test, or one of our tests f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mutating Paint breaks paints

3 participants