Mirror before scaling in _AnimatedIconPainter #93312

Amir-P · 2021-11-09T10:11:35Z

This PR attempts to fix an issue in _AnimatedIconPainter causing wrong positioning of _AnimatedIconData when matchTextDirection set true in layouts with RTL directionality. This can be fixed by doing mirror before scaling the canvas.

Code to reproduce:

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Directionality(
        textDirection: TextDirection.rtl,
        child: Scaffold(
          appBar: AppBar(
            leading: IconButton(
              icon: AnimatedIcon(
                icon: AnimatedIcons.arrow_menu,
                progress: AlwaysStoppedAnimation(0),
              ),
              onPressed: () {},
            ),
          ),
        ),
      ),
    );
  }
}

Before:

After:

Issues this PR attempts to fix:

Fixes #60521

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
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.
All existing and new tests are passing.

Piinks

Hi @Amir-P thanks for the contribution, and welcome!
Can you add another test that validates the positioning like from the example before/after images you shared?
Also, it looks like there are currently failing tests, can you take a look?

Amir-P · 2021-11-25T12:35:41Z

Hi @Amir-P thanks for the contribution, and welcome!

Can you add another test that validates the positioning like from the example before/after images you shared?

Also, it looks like there are currently failing tests, can you take a look?

Thanks! @Piinks
I've fixed the issue with private tests. ~~One of the checks (tree check) is still failing and I think that's not because of my changes.~~
Also I couldn't find a way to test the final position of drawn path. The position is determined by the transformations made to the canvas before drawing path and we have tests for that.
If you can guide me on writing tests for the position of path on canvas, please let me know.

Piinks

Also I couldn't find a way to test the final position of drawn path. The position is determined by the transformations made to the canvas before drawing path and we have tests for that.
If you can guide me on writing tests for the position of path on canvas, please let me know.

The type of test to add should take the code sample that illustrates your bug (above), and verify that the icon is placed in the right position now. This validates that it actually fixes the reported bug, and prevents it from regressing in the future.
Here is a similar test:

flutter/packages/flutter/test/material/app_bar_test.dart

Line 566 in 0e2cc64

    
           testWidgets('AppBar endDrawer icon has default size', (WidgetTester tester) async {

Instead of checking the size (tester.getSize), you would want to check the position, using tester.getCenter or tester.getTopLeft and validate that the icon is in the right place now.

Amir-P · 2021-12-24T09:29:47Z

Instead of checking the size (tester.getSize), you would want to check the position, using tester.getCenter or tester.getTopLeft and validate that the icon is in the right place now.

This change does not effect the widget and it's position. You can check the AnimatedIcon widget's position and size in both RTL and LTR direction and see there is no difference. It's happening in the painting level on canvas. You can validate the behavior using this small test (Before and after my commit):

testWidgets('Direction has no effect on AnimatedIcon widget position',
      (WidgetTester tester) async {
    await tester.pumpWidget(
      const Directionality(
        textDirection: TextDirection.rtl,
        child: AnimatedIcon(
          progress: AlwaysStoppedAnimation<double>(0.0),
          icon: AnimatedIcons.arrow_menu,
        ),
      ),
    );
    final Rect rtlRect = tester.getRect(find.byType(AnimatedIcon));
    await tester.pumpWidget(
      const Directionality(
        textDirection: TextDirection.ltr,
        child: AnimatedIcon(
          progress: AlwaysStoppedAnimation<double>(0.0),
          icon: AnimatedIcons.arrow_menu,
        ),
      ),
    );
    final Rect ltrRect = tester.getRect(find.byType(AnimatedIcon));
    expect(rtlRect, ltrRect);
  });

@Piinks

Piinks

testWidgets('Direction has no effect on AnimatedIcon widget position',
      (WidgetTester tester) async {
    await tester.pumpWidget(
      const Directionality(
        textDirection: TextDirection.rtl,
        child: AnimatedIcon(
          progress: AlwaysStoppedAnimation<double>(0.0),
          icon: AnimatedIcons.arrow_menu,
        ),
      ),
    );
    final Rect rtlRect = tester.getRect(find.byType(AnimatedIcon));
    await tester.pumpWidget(
      const Directionality(
        textDirection: TextDirection.ltr,
        child: AnimatedIcon(
          progress: AlwaysStoppedAnimation<double>(0.0),
          icon: AnimatedIcons.arrow_menu,
        ),
      ),
    );
    final Rect ltrRect = tester.getRect(find.byType(AnimatedIcon));
    expect(rtlRect, ltrRect);
  });

I couldn't find this test in the framework, can you add it? Thanks!

Also, we'll still need a test that validates the very real use case you have in your before and after image above. Otherwise we may regress in the future. The current test validates that you have changed the order of operations, not that the visual difference above has changed.

If checking the position does not validate this, then it sounds like a golden file test is more appropriate. Can you add one? Thanks!

Amir-P · 2022-01-10T20:20:16Z

Can you please check the latest commit? Some of the tests are failing but I'm not sure why because I didn't make any changes. Also I guess we need to upload those golden file images which I don't have access to. @Piinks

skia-gold · 2022-01-10T20:22:24Z

Gold has detected about 6 new digest(s) on patchset 3.
View them at https://flutter-gold.skia.org/cl/github/93312

Piinks

If you visit https://flutter-gold.skia.org/cl/github/93312 you can see the images your test generated for each platform, I can approve them when this is ready to land. You may want to add a RepaintBoundary around the icon widget, right now the images are large white squares with a tiny icon in the top left corner. :)

It looks like the file with the golden file test needs a tag added at the top to pass analysis, the other failing checks should be fixed by updating your branch.

For more on golden file testing, check out this wiki page:
https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter

Piinks · 2022-01-10T20:47:12Z

packages/flutter/test/material/animated_icons_test.dart

Nit: Can you revert this style change?

Piinks · 2022-01-10T20:47:24Z

packages/flutter/test/material/animated_icons_test.dart

Nit: This should be one line

You mean to remove the break line after ...widget', ?

Piinks · 2022-01-10T20:47:51Z

packages/flutter/test/material/animated_icons_test.dart

Nit: revert this, these style changes don't match the style guide.

Amir-P · 2022-01-10T21:19:58Z

If you visit https://flutter-gold.skia.org/cl/github/93312 you can see the images your test generated for each platform, I can approve them when this is ready to land. You may want to add a RepaintBoundary around the icon widget, right now the images are large white squares with a tiny icon in the top left corner. :)

It looks like the file with the golden file test needs a tag added at the top to pass analysis, the other failing checks should be fixed by updating your branch.

For more on golden file testing, check out this wiki page: https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter

Hope I got it right this time🤞🏻. @Piinks

skia-gold · 2022-01-10T21:22:24Z

Gold has detected about 7 new digest(s) on patchset 4.
View them at https://flutter-gold.skia.org/cl/github/93312

Piinks · 2022-01-10T22:56:00Z

Coming along nicely! Thanks for the updates! It looks like you still need to update your PR with the latest from the master branch. :)

skia-gold · 2022-01-11T07:02:24Z

Gold has detected about 4 new digest(s) on patchset 4.
View them at https://flutter-gold.skia.org/cl/github/93312

Amir-P · 2022-01-11T07:30:31Z

Coming along nicely! Thanks for the updates! It looks like you still need to update your PR with the latest from the master branch. :)

Done. @Piinks

flutter-dashboard · 2022-01-11T16:13:50Z

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 #93312 at sha 0cb88c3

Piinks

Golden file images are approved, and this LGTM! Thank you!

Piinks · 2022-01-22T00:05:12Z

This will need a secondary review, I've reached out to the team for one. Meanwhile, can you take a look at the merge conflict here?

skia-gold · 2022-01-22T06:07:24Z

Gold has detected about 4 new digest(s) on patchset 5.
View them at https://flutter-gold.skia.org/cl/github/93312

Amir-P · 2022-01-22T06:29:18Z

Done @Piinks .

Hixie · 2022-01-26T21:28:23Z

packages/flutter/test/material/animated_icons_test.dart

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

+@Tags(<String>['reduced-test-set'])


what does this do?

flutter.dev/go/reduce-ci-tests
https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter#reduced-test-set-tag

One of the velocity projects over the summer involved removing unit test runs on windows and mac machines. Just doing so though would remove our ability to provide a golden file test for every platform someone might be contributing to Flutter on, so we created a reduced test set for windows and mac. The analyzer will ensure now that all test files that contain a golden file test are included in the reduced test set.

Hixie

…r#93312)

@NATHANIELCROSBY1

@NATHANIELCROSBY1 NATHANIELCROSBY1 create … 609e127 13 days ago Git stats 27,477 commits Files Type Name Latest commit message Commit time .github Bump github/codeql-action from 1.0.26 to 1.0.31 (flutter#97820) 14 days ago bin Roll Engine from 2a4709a to 0712096 (1 revision) (flutter#9… 13 days ago dev Include -isysroot -arch and -miphoneos-version-min when creating dumm… 15 days ago examples Clean up the bindings APIs. (flutter#89451) 16 days ago packages Report progress on Dismissible update callback (flutter#95504) 14 days ago .ci.yaml Marks Linux_android opacity_peephole_fade_transition_text_perf__e2e_s… 15 days ago .cirrus.yml Pin dependencies in docker file. (flutter#97466) 18 days ago .gitattributes Add pre-stable support for create on Windows (flutter#51895) 2 years ago .gitignore Add macOS ephemeral to gitignore (flutter#96397) 20 days ago AUTHORS Mirror before scaling in _AnimatedIconPainter (flutter#93312) 24 days ago CODEOWNERS [codeowners] Remove *_builders.json ownership (flutter#91691) 4 months ago CODE_OF_CONDUCT.md Update CODE_OF_CONDUCT.md (flutter#94583) 3 months ago CONTRIBUTING.md Links How to contribute to Flutter YouTube video (flutter#96313) last month LICENSE License update (flutter#45373) 2 years ago PATENT_GRANT Rename patent file (flutter#38686) 3 years ago README.md Update README (flutter#97271) 24 days ago TESTOWNERS Add benchmarks to measure impact of alpha saveLayers in DisplayLists (f… 23 days ago analysis_options.yaml Enable no_leading_underscores_for_local_identifiers (flutter#96422) 29 days ago dartdoc_options.yaml Eliminate uses of pub executable in docs publishing and sample analys… 6 months ago flutter_console.bat License update (flutter#45373) 2 years ago git clone create 13 days ago README.md Flutter logo Build Status - Cirrus Discord badge Twitter handle Flutter is Google's SDK for crafting beautiful, fast user experiences for mobile, web, and desktop from a single codebase. Flutter works with existing code, is used by developers and organizations around the world, and is free and open source. Documentation Install Flutter Flutter documentation Development wiki Contributing to Flutter For announcements about new releases, follow the [email protected] mailing list. Our documentation also tracks breaking changes across releases. Terms of service The Flutter tool may occasionally download resources from Google servers. By downloading or using the Flutter SDK you agree to the Google Terms of Service: https://policies.google.com/terms For example, when installed from GitHub (as opposed to from a prepackaged archive), the Flutter tool will download the Dart SDK from Google servers immediately when first run, as it is used to execute the flutter tool itself. This will also occur when Flutter is upgraded (e.g. by running the flutter upgrade command). About Flutter We think Flutter will help you create beautiful, fast apps, with a productive, extensible and open development model, whether you're targeting iOS or Android, web, Windows, macOS, Linux or embedding it as the UI toolkit for a platform of your choice. Beautiful user experiences We want to enable designers to deliver their full creative vision without being forced to water it down due to limitations of the underlying framework. Flutter's layered architecture gives you control over every pixel on the screen and its powerful compositing capabilities let you overlay and animate graphics, video, text, and controls without limitation. Flutter includes a full set of widgets that deliver pixel-perfect experiences whether you're building for iOS (Cupertino) or Android (Material), along with support for customizing or creating entirely new visual components. Reflectly hero image Fast results Flutter is fast. It's powered by the same hardware-accelerated 2D graphics library that underpins Chrome and Android: Skia. We architected Flutter to support glitch-free, jank-free graphics at the native speed of your device. Flutter code is powered by the world-class Dart platform, which enables compilation to 32-bit and 64-bit ARM machine code for iOS and Android, as well as JavaScript for the web and Intel x64 for desktop devices. Dart diagram Productive development Flutter offers stateful hot reload, allowing you to make changes to your code and see the results instantly without restarting your app or losing its state. Hot reload animation Extensible and open model Flutter works with any development tool (or none at all), and also includes editor plug-ins for both Visual Studio Code and IntelliJ / Android Studio. Flutter provides tens of thousands of packages to speed your development, regardless of your target platform. And accessing other native code is easy, with support for both FFI and platform-specific APIs. Flutter is a fully open-source project, and we welcome contributions. Information on how to get started can be found in our contributor guide.

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Nov 9, 2021

google-cla bot added the cla: yes label Nov 9, 2021

HansMuller requested a review from Piinks November 19, 2021 23:00

Piinks reviewed Nov 24, 2021

View reviewed changes

Amir-P force-pushed the fix_60521 branch from 26a9c12 to f0c7e3c Compare November 25, 2021 11:05

Amir-P requested a review from Piinks November 26, 2021 08:58

Piinks requested changes Dec 23, 2021

View reviewed changes

Piinks reviewed Jan 4, 2022

View reviewed changes

Amir-P requested a review from Piinks January 10, 2022 20:20

Piinks reviewed Jan 10, 2022

View reviewed changes

Amir-P requested a review from Piinks January 10, 2022 21:20

Amir-P added 4 commits January 11, 2022 10:22

Fixed flutter#60521

7add09b

Update animated_icons_private_test.dart.tmpl

18a364b

Added golden file tests

fd62ff3

Wrapped icons in RepaintBoundary, fixed style issues

0cb88c3

Amir-P force-pushed the fix_60521 branch from 6f3f656 to 0cb88c3 Compare January 11, 2022 06:55

flutter-dashboard bot added the will affect goldens Changes to golden files label Jan 11, 2022

Piinks approved these changes Jan 22, 2022

View reviewed changes

Merge branch 'master' into fix_60521

f1555e3

Hixie reviewed Jan 26, 2022

View reviewed changes

Hixie approved these changes Jan 26, 2022

View reviewed changes

Hixie added the waiting for tree to go green label Jan 26, 2022

fluttergithubbot merged commit 42a8122 into flutter:master Jan 26, 2022

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 26, 2022

42a8122 Mirror before scaling in _AnimatedIconPainter (flutter/flutte…

c839e23

…r#93312)

engine-flutter-autoroll mentioned this pull request Jan 26, 2022

Roll Flutter from 32122480b5cf to f15dd780d4c5 (2 revisions) flutter/packages#564

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 4, 2022

42a8122 Mirror before scaling in _AnimatedIconPainter (flutter/flutte…

9014d5d

…r#93312)

NATHANIELCROSBY1 mentioned this pull request Feb 19, 2022

tyetech NATHANIELCROSBY1/flutter#1

Open

8 tasks

clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022

Mirror before scaling in _AnimatedIconPainter (flutter#93312)

cc5046d

Mirror before scaling in _AnimatedIconPainter #93312

Mirror before scaling in _AnimatedIconPainter #93312

Uh oh!

Conversation

Amir-P commented Nov 9, 2021 • edited by Piinks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code to reproduce:

Issues this PR attempts to fix:

Pre-launch Checklist

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Amir-P commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Amir-P commented Dec 24, 2021

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Amir-P commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skia-gold commented Jan 10, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amir-P commented Jan 10, 2022

Uh oh!

skia-gold commented Jan 10, 2022

Uh oh!

Piinks commented Jan 10, 2022

Uh oh!

skia-gold commented Jan 11, 2022

Uh oh!

Amir-P commented Jan 11, 2022

Uh oh!

flutter-dashboard bot commented Jan 11, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks commented Jan 22, 2022

Uh oh!

skia-gold commented Jan 22, 2022

Uh oh!

Amir-P commented Jan 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Amir-P commented Nov 9, 2021 •

edited by Piinks

Loading

Amir-P commented Nov 25, 2021 •

edited

Loading

Amir-P commented Jan 10, 2022 •

edited

Loading