Skip to content

Magnifier cleanup #143558

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.

Sign up for GitHub

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

Jump to bottom
Merged
Hixie merged 1 commit into from
Apr 3, 2024
Merged

Magnifier cleanup #143558

Hixie merged 1 commit into from
Apr 3, 2024

Conversation

@Hixie
Copy link
Contributor

@Hixie Hixie commented Feb 16, 2024

  • Introduces the ability to control whether RawMagnifier uses a clip on its decoration. (It still has to use a clip for its magnified image.)
  • Uses BlurStyle.outer to remove the clip for CupertinoMagnifier.
  • Many changes to the documentation around magnifiers and shadows.
  • Implements BoxShadow.copyWith which somehow we had never needed before.
  • Makes debugDisableShadows handle BlurStyle.outer correctly.
  • Adds MagnifierInfo.toString.
  • Aligns various operator ==s with the style guide.
  • Makes MagnifierDecoration a separate concept from Decoration, since it's not actually compatible anywhere.
  • Removes some dead code and makes other minor code simplifications.
  • Uses double syntax rather than integer syntax for various double literals for clarity.

I expect one minor golden image change (antialiasing change on Cupertino's magnifier test).

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository labels Feb 16, 2024
@Hixie Hixie marked this pull request as ready for review February 16, 2024 03:23
@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Feb 16, 2024
@Hixie
Copy link
Contributor Author

Hixie commented Feb 16, 2024

There were some other tests I forgot about that also changed a tiny bit. All the changes seem like improvements (less aggressive clipping, better antialiasing, etc).

@goderbauer goderbauer requested a review from justinmc February 26, 2024 22:17
bleroux
bleroux reviewed Feb 29, 2024
View reviewed changes
Copy link
Contributor

@bleroux bleroux left a comment

Choose a reason for hiding this comment

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

Reading this for educational purposes and spotted some small nits.
This PR is a great improvement. I had never work on the magnifier, so I will let @justinmc approved it.

packages/flutter/lib/src/material/magnifier.dart Outdated
Copy link
Contributor

@bleroux bleroux Feb 29, 2024

Choose a reason for hiding this comment

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

nit: coordinate

packages/flutter/lib/src/painting/shape_decoration.dart Outdated
Copy link
Contributor

@bleroux bleroux Feb 29, 2024

Choose a reason for hiding this comment

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

nit: redundant 'the'

packages/flutter/test/cupertino/magnifier_test.dart Outdated
Comment on lines 28 to 30
Copy link
Contributor

@bleroux bleroux Feb 29, 2024

Choose a reason for hiding this comment

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

Consider fixing the formatting from line 26 to 30.

justinmc
justinmc approved these changes Feb 29, 2024
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM with nits 👍 . Thanks for the cleanup.

packages/flutter/lib/src/cupertino/magnifier.dart Outdated
Copy link
Contributor

@justinmc justinmc Feb 29, 2024

Choose a reason for hiding this comment

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

Thanks for explaining this stuff in the docs, I remember it being very tricky when implemented.

packages/flutter/lib/src/cupertino/magnifier.dart Outdated
Copy link
Contributor

@justinmc justinmc Feb 29, 2024

Choose a reason for hiding this comment

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

Nit: Would this be any better as a "See also"?

Copy link
Contributor Author

@Hixie Hixie Mar 1, 2024

Choose a reason for hiding this comment

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

I tried both ways... I sort of felt like "see also" made it seem less immediately relevant, more of a "if you're browsing, you may also like these things" as opposed to "there's more to read on this right now, over there".

Copy link
Contributor

@justinmc justinmc Mar 6, 2024

Choose a reason for hiding this comment

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

Sounds good to me!

packages/flutter/lib/src/cupertino/magnifier.dart Outdated
Copy link
Contributor

@justinmc justinmc Feb 29, 2024

Choose a reason for hiding this comment

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

You've needed these two constants in your own code?

Copy link
Contributor Author

@Hixie Hixie Mar 1, 2024

Choose a reason for hiding this comment

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

@visibleForTesting doesn't really do anything. It's just sort of a way for us to forgive ourselves exposing suboptimal public APIs. It's best to just own that the thing is a public API.

Copy link
Contributor

@justinmc justinmc Mar 6, 2024

Choose a reason for hiding this comment

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

Should we mention that in the styleguide? I have definitely added a few visibleForTestings into the codebase 😁 .

Copy link
Contributor Author

@Hixie Hixie Mar 12, 2024

Choose a reason for hiding this comment

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

good idea, added a section on that.

packages/flutter/test/cupertino/magnifier_test.dart Outdated
Comment on lines +31 to +33
Copy link
Contributor

@justinmc justinmc Feb 29, 2024

Choose a reason for hiding this comment

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

Why are two pumps needed, and why 2 seconds?

Copy link
Contributor Author

@Hixie Hixie Mar 1, 2024

Choose a reason for hiding this comment

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

The first pump triggers the animations, the second ends them (two seconds is just "long enough to make sure all animations have finished"). This replaces "pumpAndSettle", which is generally a very sketchy way to do the same thing. ("pumpAndSettle" will fail to catch bugs where we require multiple frames to trigger or end an animation sequence. In general tests should never use "pumpAndSettle" unless they are verifying the return value.)

Copy link
Contributor

@justinmc justinmc Mar 6, 2024

Choose a reason for hiding this comment

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

Another thing to add to the styleguide? I have always tried to prefer pumpAndSettle because I thought it was actually more robust.

I feel like I'm confessing all my sins here on this review.

Copy link
Contributor Author

@Hixie Hixie Mar 12, 2024

Choose a reason for hiding this comment

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

It's already in the API docs for pumpAndSettle ("In general, it is better practice to figure out exactly why each frame is needed, and then to pump exactly as many frames as necessary. This will help catch regressions where, for instance, an animation is being started one frame later than it should."), but I'll add something to the style guide too. :-)

packages/flutter/lib/src/material/magnifier.dart Outdated
Copy link
Contributor

@justinmc justinmc Feb 29, 2024

Choose a reason for hiding this comment

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

Any value in using a macro here for deduplicating the docs on CupertinoMagnifier.shadows? If they're not subtly different.

Copy link
Contributor Author

@Hixie Hixie Mar 1, 2024

Choose a reason for hiding this comment

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

they're actually more than just subtly different, they're sort of opposites of each other (at least in terms of defaults).

(Also and this is maybe more of a dubious point but so much of what I fixed in this PR was overzealous use of templates making the docs actively worse that I'm kind of reluctant to use them at all!)

packages/flutter/test/painting/decoration_test.dart Outdated
Copy link
Contributor

@justinmc justinmc Feb 29, 2024

Choose a reason for hiding this comment

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

I never thought of testing theses kinds of methods but I should 👍

packages/flutter/lib/src/widgets/magnifier.dart Outdated
Copy link
Contributor

@justinmc justinmc Feb 29, 2024

Choose a reason for hiding this comment

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

Good idea 👍

packages/flutter/lib/src/widgets/magnifier.dart Outdated
Copy link
Contributor

@justinmc justinmc Feb 29, 2024

Choose a reason for hiding this comment

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

I'll miss "donut" but that does sound better and more generic 🍩

Copy link
Contributor Author

@Hixie Hixie Mar 1, 2024

Choose a reason for hiding this comment

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

well also it stopped looking like a donut once i made it use an infinite rectangle instead of just growing the path to just enough to cover everything. :-)

@Hixie
Copy link
Contributor Author

Hixie commented Mar 1, 2024

Reading this for educational purposes and spotted some small nits.

Thanks for your comments @bleroux! Good catches!

@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@Hixie
Copy link
Contributor Author

Hixie commented Mar 5, 2024

Blocked on #144643

@justinmc
Copy link
Contributor

justinmc commented Mar 6, 2024

What is breaking because of #144643? I just noticed that a golden was recently disabled for the magnifier: #144350 (maybe unrelated but just flagging it).

@Hixie
Copy link
Contributor Author

Hixie commented Mar 6, 2024 •
edited
Loading

What is breaking because of #144643? I just noticed that a golden was recently disabled for the magnifier: #144350 (maybe unrelated but just flagging it).

The new code path triggers a bug in Impeller in one of the golden images. Basically a part of the image is missing.

edit: looks like the disabled test is unaffected by this PR

@flutter-dashboard

This comment was marked as off-topic.

Sign in to view
@Hixie

This comment was marked as outdated.

Sign in to view
@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@Hixie

This comment was marked as outdated.

Sign in to view
@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@Hixie

This comment was marked as outdated.

Sign in to view
@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@Hixie

This comment was marked as outdated.

Sign in to view
@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@Hixie Hixie force-pushed the magnifier branch 3 times, most recently from 7ff81d0 to 3422b23 Compare March 27, 2024 23:38
@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@Hixie

This comment was marked as outdated.

Sign in to view
@Hixie
Copy link
Contributor Author

Hixie commented Mar 29, 2024

The problems above were caused by my misunderstanding gold. Submitted a new commit to hopefully get new results.

@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@Hixie
Magnifier cleanup
320d046 
- Introduces the ability to control whether RawMagnifier uses a clip on its decoration. (It still has to use a clip for its magnified image.)
- Uses `BlurStyle.outer` to remove the clip for CupertinoMagnifier.
- Many changes to the documentation around magnifiers and shadows.
- Implements `BoxShadow.copyWith` which somehow we had never needed before.
- Makes `debugDisableShadows` handle `BlurStyle.outer` correctly.
- Adds `MagnifierInfo.toString`.
- Aligns various `operator ==`s with the style guide.
- Makes MagnifierDecoration a separate concept from Decoration, since it's not actually compatible anywhere.
- Removes some dead code and makes other minor code simplifications.
- Uses double syntax rather than integer syntax for various double literals for clarity.
@Hixie Hixie mentioned this pull request Mar 29, 2024
Closed
@flutter-dashboard
Copy link

flutter-dashboard bot commented Mar 29, 2024

Golden file changes are available for triage from new commit, Click here to view.

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 #143558 at sha 320d046

@Hixie Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 29, 2024
justinmc
justinmc reviewed Apr 3, 2024
View reviewed changes
packages/flutter/test/widgets/magnifier_test.dart
top: magnifierPosition.dy + magnifierFocalPoint.dy,
child: Container(
color: Colors.pink,
color: Colors.black,
Copy link
Contributor

@justinmc justinmc Apr 3, 2024

Choose a reason for hiding this comment

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

Were these color changes just for better visibility?

Copy link
Contributor Author

@Hixie Hixie Apr 3, 2024

Choose a reason for hiding this comment

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

yeah. i was fighting this particular golden image test for weeks and needed to sanity check that the image was actually updating (it wasn't! until it was).

@Hixie Hixie merged commit 13bec28 into flutter:master Apr 3, 2024
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 4, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 4, 2024
@engine-flutter-autoroll
13bec28 Magnifier cleanup (flutter/flutter#143558)
b1e1fe7
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Apr 4, 2024
@engine-flutter-autoroll
Roll Flutter from e868e2b to ac2ca93 (25 revisions) (#6460)
b966e74 
flutter/flutter@e868e2b...ac2ca93

2024-04-04 [email protected] Roll Flutter Engine from aadf522e3c98 to 4303dc0d7a73 (1 revision) (flutter/flutter#146262)
2024-04-04 [email protected] Roll Flutter Engine from c57c8665b4eb to aadf522e3c98 (1 revision) (flutter/flutter#146261)
2024-04-04 [email protected] Roll Flutter Engine from ca7596642cfd to c57c8665b4eb (1 revision) (flutter/flutter#146259)
2024-04-04 [email protected] Roll Flutter Engine from 83c037c449b5 to ca7596642cfd (1 revision) (flutter/flutter#146258)
2024-04-04 [email protected] Roll Flutter Engine from 614154012e93 to 83c037c449b5 (1 revision) (flutter/flutter#146255)
2024-04-04 [email protected] Roll Flutter Engine from 41da00ac46bc to 614154012e93 (1 revision) (flutter/flutter#146250)
2024-04-04 [email protected] Roll Flutter Engine from 18fdcad40332 to 41da00ac46bc (6 revisions) (flutter/flutter#146246)
2024-04-04 [email protected] Roll pub packages (flutter/flutter#146245)
2024-04-03 [email protected] Add tests for theme_extension.1.dart API example. (flutter/flutter#145819)
2024-04-03 [email protected] Roll Flutter Engine from 349608d2b008 to 18fdcad40332 (6 revisions) (flutter/flutter#146240)
2024-04-03 [email protected] Add `missing_code_block_language_in_doc_comment` lint. (flutter/flutter#145354)
2024-04-03 [email protected] Magnifier cleanup (flutter/flutter#143558)
2024-04-03 [email protected] Set up Kotlin linting step in ci with ktlint (flutter/flutter#143478)
2024-04-03 [email protected] Roll Flutter Engine from 1a9e48ab1c9a to 349608d2b008 (1 revision) (flutter/flutter#146230)
2024-04-03 [email protected] Update ownership to GitHub handles (flutter/flutter#146221)
2024-04-03 [email protected] Roll Flutter Engine from d065763b1a63 to 1a9e48ab1c9a (1 revision) (flutter/flutter#146226)
2024-04-03 [email protected] Refactor fuchsia_precache (flutter/flutter#145978)
2024-04-03 [email protected] Roll Flutter Engine from b1c23addaec5 to d065763b1a63 (1 revision) (flutter/flutter#146218)
2024-04-03 [email protected] `computeDryBaseline` for rendering / widgets RenderBoxes (flutter/flutter#146143)
2024-04-03 [email protected] Roll Flutter Engine from 7b28ae1d15cb to b1c23addaec5 (1 revision) (flutter/flutter#146214)
2024-04-03 [email protected] Fix TextStyle.lerp() to properly interpolate text shadows (flutter/flutter#145666)
2024-04-03 [email protected] Renderflex cross intrinsic size with baseline alignment (flutter/flutter#146185)
2024-04-03 [email protected] Roll Flutter Engine from 56fa2c33a5f7 to 7b28ae1d15cb (1 revision) (flutter/flutter#146208)
2024-04-03 [email protected] Refactor customer_testing (flutter/flutter#145911)
2024-04-03 [email protected] Add `DropdownMenu` cursor behavior sample (flutter/flutter#146133)

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],[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
@HansMuller HansMuller mentioned this pull request Apr 5, 2024
Closed
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
@engine-flutter-autoroll @TecHaxter
Roll Flutter from e868e2b to ac2ca93 (25 revisions) (flutter#6460)
0de3376 
flutter/flutter@e868e2b...ac2ca93

2024-04-04 [email protected] Roll Flutter Engine from aadf522e3c98 to 4303dc0d7a73 (1 revision) (flutter/flutter#146262)
2024-04-04 [email protected] Roll Flutter Engine from c57c8665b4eb to aadf522e3c98 (1 revision) (flutter/flutter#146261)
2024-04-04 [email protected] Roll Flutter Engine from ca7596642cfd to c57c8665b4eb (1 revision) (flutter/flutter#146259)
2024-04-04 [email protected] Roll Flutter Engine from 83c037c449b5 to ca7596642cfd (1 revision) (flutter/flutter#146258)
2024-04-04 [email protected] Roll Flutter Engine from 614154012e93 to 83c037c449b5 (1 revision) (flutter/flutter#146255)
2024-04-04 [email protected] Roll Flutter Engine from 41da00ac46bc to 614154012e93 (1 revision) (flutter/flutter#146250)
2024-04-04 [email protected] Roll Flutter Engine from 18fdcad40332 to 41da00ac46bc (6 revisions) (flutter/flutter#146246)
2024-04-04 [email protected] Roll pub packages (flutter/flutter#146245)
2024-04-03 [email protected] Add tests for theme_extension.1.dart API example. (flutter/flutter#145819)
2024-04-03 [email protected] Roll Flutter Engine from 349608d2b008 to 18fdcad40332 (6 revisions) (flutter/flutter#146240)
2024-04-03 [email protected] Add `missing_code_block_language_in_doc_comment` lint. (flutter/flutter#145354)
2024-04-03 [email protected] Magnifier cleanup (flutter/flutter#143558)
2024-04-03 [email protected] Set up Kotlin linting step in ci with ktlint (flutter/flutter#143478)
2024-04-03 [email protected] Roll Flutter Engine from 1a9e48ab1c9a to 349608d2b008 (1 revision) (flutter/flutter#146230)
2024-04-03 [email protected] Update ownership to GitHub handles (flutter/flutter#146221)
2024-04-03 [email protected] Roll Flutter Engine from d065763b1a63 to 1a9e48ab1c9a (1 revision) (flutter/flutter#146226)
2024-04-03 [email protected] Refactor fuchsia_precache (flutter/flutter#145978)
2024-04-03 [email protected] Roll Flutter Engine from b1c23addaec5 to d065763b1a63 (1 revision) (flutter/flutter#146218)
2024-04-03 [email protected] `computeDryBaseline` for rendering / widgets RenderBoxes (flutter/flutter#146143)
2024-04-03 [email protected] Roll Flutter Engine from 7b28ae1d15cb to b1c23addaec5 (1 revision) (flutter/flutter#146214)
2024-04-03 [email protected] Fix TextStyle.lerp() to properly interpolate text shadows (flutter/flutter#145666)
2024-04-03 [email protected] Renderflex cross intrinsic size with baseline alignment (flutter/flutter#146185)
2024-04-03 [email protected] Roll Flutter Engine from 56fa2c33a5f7 to 7b28ae1d15cb (1 revision) (flutter/flutter#146208)
2024-04-03 [email protected] Refactor customer_testing (flutter/flutter#145911)
2024-04-03 [email protected] Add `DropdownMenu` cursor behavior sample (flutter/flutter#146133)

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],[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
@antholeole
Copy link
Contributor

antholeole commented Jun 26, 2024

Thanks for this PR, and thanks for reverse engineering some of the details I didn't understand. I must confess that some of my sins in the loupe haunt me - in two-year retrospect, I would have done a lot differently - I guess they call that "learning" 😄.

@justinmc
Copy link
Contributor

justinmc commented Jul 12, 2024

No worries! The magnifier has served us pretty well over the last few years. If it weren't for you, it would probably still be in my backlog.

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

Reviewers

@justinmc justinmc justinmc approved these changes

@bleroux bleroux bleroux left review comments

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Hixie @justinmc @antholeole @bleroux