Skip to content

Conversation

@Yash-Dhrangdhariya
Copy link
Contributor

Description

This PR adds == and hashCode implementations to ScrollAwareImageProvider.

According to the ImageProvider documentation:

It should be immutable and implement the == operator and the hashCode getter.

ScrollAwareImageProvider did not previously override these, which could result in instances with identical configuration being treated as unequal. This behavior can negatively impact image caching and equality checks.

With this change, ScrollAwareImageProvider now conforms to the ImageProvider contract.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Sep 7, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly implements operator== and hashCode for ScrollAwareImageProvider, aligning it with the ImageProvider contract. This is an important fix that will improve image caching and equality checks. The added @immutable annotation is also appropriate. The new tests effectively validate the changes. I have one minor suggestion to further improve test coverage.

Comment on lines 98 to 108
testWidgets('Reflexivity: instance should be equal to itself', (WidgetTester tester) async {
final DisposableBuildContext context = await createContext(tester);

final ScrollAwareImageProvider<TestImageProvider> image =
ScrollAwareImageProvider<TestImageProvider>(
context: context,
imageProvider: TestImageProvider(testImage.clone()),
);

expect(image == image, isTrue);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The test coverage for equality is good. To make it more comprehensive, you could add a test case to verify that two ScrollAwareImageProvider instances are not equal if their context is different, while their imageProvider is the same. This would ensure all properties contributing to equality are also tested for inequality.

For example:

testWidgets('Instances with different contexts should not be equal', (WidgetTester tester) async {
  final DisposableBuildContext context1 = await createContext(tester);
  final DisposableBuildContext context2 = await createContext(tester);

  final TestImageProvider testImageProvider = TestImageProvider(testImage.clone());

  final ScrollAwareImageProvider<TestImageProvider> image1 =
      ScrollAwareImageProvider<TestImageProvider>(
        context: context1,
        imageProvider: testImageProvider,
      );

  final ScrollAwareImageProvider<TestImageProvider> image2 =
      ScrollAwareImageProvider<TestImageProvider>(
        context: context2,
        imageProvider: testImageProvider,
      );

  expect(image1 == image2, isFalse);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal but I guess I agree with Gemini here.

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!


expect(image1 == image2, isFalse);

addTearDown(testImage2.dispose);
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to immediately after testImage2 is created. That way, if there is an error before this line, it will still get disposed.

imageProvider: TestImageProvider(testImage2),
);

expect(image1 == image2, isFalse);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an expect for the hashCode too? Might as well.

Comment on lines 98 to 108
testWidgets('Reflexivity: instance should be equal to itself', (WidgetTester tester) async {
final DisposableBuildContext context = await createContext(tester);

final ScrollAwareImageProvider<TestImageProvider> image =
ScrollAwareImageProvider<TestImageProvider>(
context: context,
imageProvider: TestImageProvider(testImage.clone()),
);

expect(image == image, isTrue);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal but I guess I agree with Gemini here.

@Yash-Dhrangdhariya Yash-Dhrangdhariya force-pushed the fix/scrollawareimageprovider-equality branch from de7c407 to 29a9688 Compare September 23, 2025 17:34
@justinmc
Copy link
Contributor

@Yash-Dhrangdhariya Thanks for the quick response! I'll re-review in a bit, but FYI check out the failures here now.

@Yash-Dhrangdhariya Yash-Dhrangdhariya force-pushed the fix/scrollawareimageprovider-equality branch 4 times, most recently from 16b316b to 37b6841 Compare September 27, 2025 14:10
@Yash-Dhrangdhariya Yash-Dhrangdhariya force-pushed the fix/scrollawareimageprovider-equality branch from e5f80c5 to 7650f70 Compare October 11, 2025 07:56
@Yash-Dhrangdhariya
Copy link
Contributor Author

@justinmc Thanks for the heads-up! I’ve updated the PR— all test cases are passing.

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ksokolovskyi ksokolovskyi self-requested a review October 15, 2025 05:54
Copy link
Contributor

@ksokolovskyi ksokolovskyi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for your contribution!

lgtm_dash

@ksokolovskyi ksokolovskyi added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 15, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Oct 15, 2025
Merged via the queue into flutter:master with commit f22249b Oct 16, 2025
77 of 78 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 16, 2025
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 16, 2025
…10244)

Manual roll requested by tarrinneal@google.com

flutter/flutter@7cd821c...a873a27

2025-10-16 30870216+gaaclarke@users.noreply.github.com [tool] makes listing a shader also as an asset a build failure (flutter/flutter#176866)
2025-10-16 engine-flutter-autoroll@skia.org Roll Packages from d062181 to 835dccb (7 revisions) (flutter/flutter#177100)
2025-10-16 jason-simmons@users.noreply.github.com Handle the new location of Perfetto in create_updated_flutter_deps.py (flutter/flutter#177099)
2025-10-16 matt.kosarek@canonical.com Implement dialog windows for the win32 platform (flutter/flutter#176309)
2025-10-16 rmolivares@renzo-olivares.dev `SelectableRegion` should not show flutter rendered context menu when web context menu is enabled (flutter/flutter#176855)
2025-10-16 jason-simmons@users.noreply.github.com Manual roll Skia to 2d9df7c70b6f (flutter/flutter#177074)
2025-10-16 31685655+SalehTZ@users.noreply.github.com feat: add `OptionsViewOpenDirection.mostSpace` to `RawAutocomplete` (flutter/flutter#172997)
2025-10-16 43054281+camsim99@users.noreply.github.com [Android] Refactor `ImageReaderSurfaceProducer` restoration after app resumes (flutter/flutter#175937)
2025-10-15 34465683+rkishan516@users.noreply.github.com Refactor: migrate fade upwards page transition builder to widgets (flutter/flutter#175560)
2025-10-15 fluttergithubbot@gmail.com Marks Windows windowing_test to be unflaky (flutter/flutter#176701)
2025-10-15 72062416+Yash-Dhrangdhariya@users.noreply.github.com fix: 🐛 Add equality and hashCode implementations to ScrollAwareImageProvider (flutter/flutter#175038)
2025-10-15 mohellebiabdessalem@gmail.com Updates `sliver_tree.1.dart‎` to use `MediaQuery.widthOf(context)`  (flutter/flutter#176888)
2025-10-15 mdebbar@google.com [web] Fix focus issues in newer versions of Chrome (flutter/flutter#176938)
2025-10-15 jessiewong401@gmail.com [Android 16] Update `android_engine_vulkan_tests` to Test Against SDK 36 Emulator (flutter/flutter#176985)
2025-10-15 sokolovskyi.konstantin@gmail.com Fix key events interception by RadioGroup when no Radio is focused. (flutter/flutter#176335)
2025-10-15 21270878+elliette@users.noreply.github.com Update cherry-pick instructions to include instructions for pre-release CPs (flutter/flutter#177020)
2025-10-15 jason-simmons@users.noreply.github.com Manual roll Skia to c501c727a007 (flutter/flutter#177015)
2025-10-15 robert.ancell@canonical.com Update examples to latest Linux runner style (flutter/flutter#177033)
2025-10-15 59215665+davidhicks980@users.noreply.github.com [material/menu_anchor.dart] Create internal menu controller if external controller is changed to null. (flutter/flutter#176375)
2025-10-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix - TalkBack does not announce list information (#174374)" (flutter/flutter#177062)
2025-10-14 robert.ancell@canonical.com Implement Regular Windows for Linux (flutter/flutter#176187)
2025-10-14 31510811+jwlilly@users.noreply.github.com Fix - TalkBack does not announce list information (flutter/flutter#174374)

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 stuartmorgan@google.com,tarrinneal@google.com 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants