Fix RefreshIndicator performance issue

[christian-muertz]

Description

Fixes the performance issue described by #47664.

Related Issues

-Fixes #47664

Tests

I added the following tests: /

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I signed the CLA.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I updated/added relevant documentation (doc comments with ///).
• All existing and new tests are passing.
• The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

• No, no existing tests failed, so this is not a breaking change.
• Yes, this is a breaking change. If not, delete the remainder of this section.
  • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
  • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
  • I wrote a migration guide: Replace with a link to your migration guide

[fluttergithubbot]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

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

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[christian-muertz]

@googlebot I fixed it.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[dnfield]

Is there some benchmark that this will improve? If not, we should write one before landing this.

If we don't have a benchmark for it, it will very easily get regressed (it will also be harder to tell if or how much this actually improves performance).

[LongCatIsLooong]

Hi @christian-muertz great job spotting this issue and thank you for the fix! However we can't merge the pull request without a valid test. I think a good test to add would be one that verifies RefreshIndicator's child does not re-layout excessively. Are you still interested in working on this PR? I would be happy to work with you on adding a test!

[christian-muertz]

Yes iam still interested. I will take a look at it again on friday!

[LongCatIsLooong]

Hi @christian-muertz, any updates on this?

[christian-muertz]

No, i currently have no idea how to test whether the child was relayout.

[LongCatIsLooong]

Does something like this make sense (the code snippet makes use of some global variables in refresh_indicator_test.dart)?

  testWidgets(
    'RefreshIndicator does not force child to relayout',
    (WidgetTester tester) async {
      refreshCalled = false;
      int layoutCount = 0;

      Widget layoutCallback(BuildContext context, BoxConstraints constraints) {
        layoutCount += 1;
        return ListView(
          physics: const AlwaysScrollableScrollPhysics(),
          children: <String>['A', 'B', 'C', 'D', 'E', 'F'].map<Widget>((String item) {
              return SizedBox(
                height: 200.0,
                child: Text(item),
              );
          }).toList(),
        );
      };
      await tester.pumpWidget(
        MaterialApp(
          home: RefreshIndicator(
            onRefresh: refresh,
            child: LayoutBuilder(builder: layoutCallback),
          ),
        ),
      );

      await tester.fling(find.text('A'), const Offset(0.0, 300.0), 1000.0);
      await tester.pump();

      await tester.pump(const Duration(seconds: 1)); // finish the scroll animation
      await tester.pump(const Duration(seconds: 1)); // finish the indicator settle animation
      await tester.pump(const Duration(seconds: 1)); // finish the indicator hide animation
      expect(refreshCalled, true);
      expect(layoutCount, 1);
  });

[christian-muertz]

Yes it does make sense. I would suggest not checking for it to refresh because that is not what the test is for. I will add a test like this in a few mins :D

[LongCatIsLooong]

I think there should be a return true; ?

Alternatively:

assert((_dragOffset == null) == (_mode == null));
assert((_isIndicatorAtTop == null) == (_mode == null));

[christian-muertz]

You are right. I'll add a return true . I think its easier to understand !

[LongCatIsLooong]

LGTM.

[LongCatIsLooong]

nit: our style guide prefers += over ++: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#prefer--over-

[LongCatIsLooong]

@dnfield it seems the change does prevent RefreshIndicator to re-layout its child when its state changes. Now that there's a test is a benchmark still required?

[dnfield]

LGTM

[dnfield]

Nope, this works too (we can test that we're not doing the extra work we don't need to do).